[ZH] Prevent hang in network lobby with long player names

[slurmlord]

• Fixes Network Game Room hangs if 8 players with long nicknames join #79

The Problem

When many players with long names join a lobby, the host can hang. This is caused by an eternal loop while trying to truncate player names in order to fit the entire game info within a network packet (476 bytes, with 400 for the game info).
The eternal looping is a result of having overflowed the total packet size and thus requiring a player name to have negative length in order to fit within the packet. The truncation is done with a while loop checking if the string is longer than the required value, and if so removing the last character. Removing the last character on an AsciiString is a no-op, and thus the loop continues.

This name truncation is not implemented in Generals, and is therefore not affected by this issue either.

The Solution

Instead of attempting to truncate the player names "on the fly", first try to construct the game options string with full names. If the resulting string is within the size limits, use that string.
If the resulting string is too long, use the size difference to determine how much truncation needs to happen with the player names to fit.
Each player name is truncated one character at the time in a round-robin fashion until the truncation goal is met.

A lower limit of 2 characters for player names has been added, meaning that no name will be truncated shorter than that. If all names are truncated to 2 characters and the string is still too long, the serialization will fail and return an empty string for the game options.

[xezon]

The presented fix is too complicated.

Better approach (high level illustration):

std::vector<AsciiString> playerNames = BuildPlayerNames( playerNames );
AsciiString infoString = GameInfoToAsciiString( game, playerNames );

// TheSuperHackers @bugfix Safely truncate the game info string by
// scrapping characters off of player names if the overall length is too large.
if (infoString.length() > m_lanMaxOptionsLength)
{
  const UnsignedInt truncateAmount = infoString.length() - m_lanMaxOptionsLength;
  if (!TruncatePlayerNames( playerNames, truncateAmount ))
  {
    DEBUG_CRASH("Unable to scrap the required space off of player names");
    // abort or whatever.
  }
  infoString = GameInfoToAsciiString( game, playerNames );
}

This is a 2 pass build, but the second pass is an exceptional pass and not the norm. It is easier to understand and also easier to modify or remove afterwards.

Edit: Mockup improved.

[slurmlord]

Ah, didn't see the updated approach before pushing. I can split out the truncation from the building of the player name vector in a new iteration, it makes the separations of concerns even more clear.

[xezon]

should this be "scrapping" or "scraping"?

[xezon]

I suggest build the vector in this class and return it. Because right now this expectes passing the vector with the correct size which is not so nice.

[xezon]

This implementation is not fair to players that have short names :-)

Player names will be truncated like so

Start

TheDude
ThePrince
HolySmokesLongAssWastefulName

First iteration NOW

TheDud
ThePrinc
HolySmokesLongAssWastefulNam

First iteration MORE FAIR

TheDude
ThePrince
HolySmokesLongAssWastefulN

I think this should be implemented in a way that it aggressively starts truncating the long names first.

To do that, you can take the max length of all player names, and then use that as the cap to start truncating on the iterations. However that is a wasteful algorithm.

A more efficient way to implement it is to sort the names first (use a local array with indices to the vector) and then start truncating from longest to smallest names. Maybe it can be implemented without excessive loops even. Note that removeLastChar is also quite a wasteful operation.

[xezon]

One more consideration here is: Will a new implementation for the truncate be compatible with Retail game clients? To test this, compile with RTS_MULTI_INSTANCE and launch one client before this change and one after, then matchmake in LAN.

Edit: On the other hand, the original implemention will hang up, so perhaps it does not matter at all.

[Mauller]

The game already limits the number of characters a Lan Name can have to 11 characters, even though it should be 12+1 based on g_lanPlayerNameLength + 1 being the size of the character array for the player name

[xezon]

Null test is not necessary. Slot is not expected NULL.

[xezon]

So originally it would have returned the long string if it was unable to (theoretically) truncate. What happens if we return an empty string or a string that is longer than the cap?

[xezon]

Should this be <= instead ?

[xezon]

Perhaps we can do TheLAN && (infoString.getLength() < m_lanMaxOptionsLength), judging by the DEBUG_ASSERTCRASH below.

[xezon]

Perhaps we move this assert to where the new DEBUG_LOG is above?