Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
5efb9b8
Add estimates for required name truncation to prevent game info strin…
slurmlord Jun 20, 2025
ca773e0
Rename index var to si to avoid colliding with i.
slurmlord Jun 20, 2025
b7eafeb
Move name truncation to separate method and make 2 serialization pass…
slurmlord Jun 20, 2025
d5a4150
Split Player name list creation and truncation into separate functions.
slurmlord Jun 20, 2025
1ae9876
Add removeLastNChars method to AsciiString.
slurmlord Jul 1, 2025
cdbf4b7
Ensure game options are null terminated.
slurmlord Jul 1, 2025
aab7f02
Ensure we accept the maximum length game options.
slurmlord Jul 1, 2025
f8600c6
Fairer truncation algo, rewarding short names.
slurmlord Jul 1, 2025
ca15e22
VC6 compat.
slurmlord Jul 1, 2025
60d1b56
Rename removeLastNChars to truncate.
slurmlord Jul 2, 2025
c83bae0
Merge remote-tracking branch 'origin/main' into fix-lobby-hang
slurmlord Jul 2, 2025
87b552e
Improve truncation target length selection and move away from std::pair.
slurmlord Jul 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions GeneralsMD/Code/GameEngine/Include/Common/AsciiString.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,12 @@ class AsciiString
*/
void removeLastChar();

/**
Remove the final N characters in the string. If the string is empty,
do nothing.
*/
void removeLastNChars(UnsignedInt chars);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should then also be replicated in UnicodeString, DisplayString.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking of doing that in a separate PR, and at the same time replace the cases where removeLastChar being used in a loop with one call to truncate instead. Also replicating to Generals, while the issue in this PR is ZH-only.
Does that sound like a reasonable approach?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. How about we do that other change before this one? Then is clean.


/**
Analogous to sprintf() -- this formats a string according to the
given sprintf-style format string (and the variable argument list)
Expand Down
11 changes: 9 additions & 2 deletions GeneralsMD/Code/GameEngine/Source/Common/System/AsciiString.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,15 +331,22 @@ void AsciiString::toLower()

// -----------------------------------------------------
void AsciiString::removeLastChar()
{
removeLastNChars(1U);
}

// -----------------------------------------------------
void AsciiString::removeLastNChars(UnsignedInt chars)
{
validate();
if (m_data)
if (m_data && chars > 0)
{
int len = strlen(peek());
if (len > 0)
{
ensureUniqueBufferOfSize(len+1, true, NULL, NULL);
peek()[len - 1] = 0;
chars = chars > len ? len : chars;
peek()[len - chars] = 0;
}
}
validate();
Expand Down
141 changes: 120 additions & 21 deletions GeneralsMD/Code/GameEngine/Source/GameNetwork/GameInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@
#include "GameNetwork/LANAPI.h" // for testing packet size
#include "GameNetwork/LANAPICallbacks.h" // for testing packet size
#include "strtok_r.h"
#include <algorithm>
#include <utility>

#ifdef RTS_INTERNAL
// for occasional debugging...
Expand Down Expand Up @@ -897,7 +899,89 @@ Bool GameInfo::isSandbox(void)

static const char slotListID = 'S';

AsciiString GameInfoToAsciiString( const GameInfo *game )
static AsciiStringVec BuildPlayerNames(const GameInfo& game)
{
AsciiStringVec playerNames;
playerNames.reserve(MAX_SLOTS);

for (Int i = 0; i < MAX_SLOTS; ++i)
{
const GameSlot* slot = game.getConstSlot(i);
if (slot->isHuman())
{
playerNames.push_back(WideCharStringToMultiByte(slot->getName().str()).c_str());
}
else
{
playerNames.push_back(AsciiString::TheEmptyString);
}
}

return playerNames;
}

static Bool TruncatePlayerNames(AsciiStringVec& playerNames, UnsignedInt truncateAmount)
{
// wont truncate any name to below this length
const Int MinimumNameLength = 2;
UnsignedInt availableForTruncation = 0;

// make length+index pairs for the player names
std::vector<std::pair<Int, size_t> > lengthIndex;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not use std::pair (or std::tuple). Prefer using a struct, because that can be given proper names for the fields.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, it felt a bit dirty. Much better now :)

lengthIndex.reserve(playerNames.size());
for (size_t pi = 0; pi < playerNames.size(); ++pi)
{
Int playerNameLength = playerNames[pi].getLength();
lengthIndex.push_back(std::make_pair(playerNameLength, pi));
availableForTruncation += std::max(0, playerNameLength - MinimumNameLength);
}

if (truncateAmount > availableForTruncation)
{
DEBUG_LOG(("TruncatePlayerNames - Requested to truncate %u chars from player names, but only %u were available for truncation.\n", truncateAmount, availableForTruncation));
return false;
}

// sort based on length in descending order
std::sort(lengthIndex.begin(), lengthIndex.end(), std::greater<std::pair<Int, size_t> >());

// determine how long each of the player names should be
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This loop does not yet look optimal. It goes one char at a time, over all players.

It looks like it should be possible to skip ahead multiple characters by peeking into the next player name (if it is not the last one).

longplayername
dude
    ^ we already know that the first name is 10 characters longer than all others

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was a bit afraid of making it too complicated again, but the latest iteration will do greater steps in the target length. It's a bit more complex, but perhaps manageable?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In inquired with Chat GPT about it. Can you check?

Chat GPT

Great! To improve performance, we can avoid repeated scanning and dynamic target recomputation by switching to a leveling algorithm, inspired by how you might "flatten terrain" from the top down.

✅ Optimal Fair Truncation Algorithm — O(n log n)

📌 Key Ideas:

  1. Sort strings by length descending.

  2. Flatten in levels: repeatedly lower the longest group of strings to the height of the next group.

  3. For each level:

    • Compute how many characters you'd save by flattening all strings in that level.
    • If you can afford it, flatten and continue.
    • If not, stop and perform partial truncation evenly on that level.

This avoids the repeated loop-reset logic of your original code.

🔧 Optimized Implementation

#include <vector>
#include <string>
#include <algorithm>
#include <cassert>
#include <iostream>

using namespace std;

// Alias types for clarity
using AsciiStringVec = std::vector<std::string>;
using Int = int;
using UnsignedInt = unsigned int;
using Bool = bool;

struct LengthIndexPair {
    Int Length;
    size_t Index;
};

static Bool TruncatePlayerNames(AsciiStringVec& playerNames, UnsignedInt truncateAmount) {
    constexpr Int MinimumNameLength = 2;
    const size_t n = playerNames.size();
    if (n == 0 || truncateAmount == 0) return true;

    // Prepare length/index pairs and calculate available truncation budget
    vector<LengthIndexPair> entries(n);
    UnsignedInt availableForTruncation = 0;

    for (size_t i = 0; i < n; ++i) {
        Int len = static_cast<Int>(playerNames[i].size());
        entries[i] = { len, i };
        availableForTruncation += max(0, len - MinimumNameLength);
    }

    if (truncateAmount > availableForTruncation) {
        cerr << "Requested to truncate more than allowed.\n";
        return false;
    }

    // Sort by descending length
    sort(entries.begin(), entries.end(), [](const auto& a, const auto& b) {
        return a.Length > b.Length;
    });

    // Process in levels
    size_t i = 0;
    while (i < n && truncateAmount > 0) {
        Int currentLength = entries[i].Length;

        // Find the next group with a smaller length
        size_t j = i + 1;
        while (j < n && entries[j].Length == currentLength) ++j;

        // Determine target level
        Int nextLength = (j < n) ? entries[j].Length : MinimumNameLength;
        Int maxReduction = currentLength - max(nextLength, MinimumNameLength);
        size_t groupSize = j;

        UnsignedInt cost = groupSize * maxReduction;

        if (cost <= truncateAmount) {
            // Flatten whole group
            for (size_t k = 0; k < groupSize; ++k)
                entries[k].Length -= maxReduction;
            truncateAmount -= cost;
            i = j; // move to next group
        } else {
            // Partial truncation to equal target
            Int cut = truncateAmount / groupSize;
            Int extra = truncateAmount % groupSize;

            for (size_t k = 0; k < groupSize; ++k) {
                entries[k].Length -= cut;
                if (extra > 0) {
                    entries[k].Length -= 1;
                    --extra;
                }
            }
            truncateAmount = 0;
            break;
        }
    }

    // Apply truncation
    for (const auto& entry : entries) {
        Int newLength = max(entry.Length, MinimumNameLength);
        if ((Int)playerNames[entry.Index].size() > newLength) {
            playerNames[entry.Index].resize(newLength);
        }
    }

    return true;
}

✅ Time Complexity

  • Sorting: O(n log n)
  • Leveling: O(n) (each index touched once)
  • Total: O(n log n) — optimal for this problem

🧪 Example

Input:

playerNames = {"Jonathan", "Tom", "Christopher", "Al", "Dan"};
truncateAmount = 10;

After truncation:

  • Long names like "Jonathan" and "Christopher" will lose more characters than "Tom", "Dan", and "Al".
  • But none of the shorter names will be longer than truncated longer names.

Let’s compare the performance characteristics of your original implementation and the optimized one I provided, to show exactly why the latter is more efficient.

🧠 1. Your Original Implementation

🔁 Key Pattern:

while (truncateAmount > 0)
{
    for (size_t i = 0; i < lengthIndex.size(); ++i)
    {
        ...
        if (condition) {
            i = -1; // restart loop
            continue;
        }
    }
}

❌ Performance Implications:

  • The outer while loop may execute multiple full scans of the list (O(n) per scan).

  • The inner for loop is restarted (i = -1) every time currentTargetLength changes. This is a valid but costly retry strategy.

  • In the worst case, the target length changes frequently (e.g., strings with many different lengths), resulting in O(n²) time complexity.

🔄 Example Worst-Case Behavior:

If you have n strings with distinct lengths, it will:

  • Attempt to lower the longest string to the second longest.

  • Then restart and lower both to the third longest.

  • And so on…

That’s O(n²) operations in the worst case.

⚡ 2. Optimized Implementation

✅ Key Strategy:

  • Uses a leveling approach: rather than decrementing string lengths one-by-one, it processes all strings of the same length together.

  • Each “level” flattens a group of strings down to the next shorter length (or as far as allowed).

  • The algorithm processes each string exactly once in a sorted list.

📈 Performance Characteristics:

  • Sort once: O(n log n)

  • Single pass leveling: O(n)

  • Total time complexity: O(n log n)

  • No backtracking, no restarting loops, no wasted work.

💡 Analogy

Imagine you have towers of blocks (string lengths) and you want to remove N blocks total.

Your Approach:

"Try removing blocks from the tallest towers one by one. If we can't balance, restart the process with a new target height."

Problem: You often keep re-checking previous towers after recalculating your target — redundant work.

Optimized Approach:

"Sort the towers by height, and flatten them level by level, always removing as many blocks as possible at once."

Benefit: You never redo work; every block removal counts.

🧪 Example Timing Difference (Theoretical)

Strings Count Your Impl (Worst Case) Optimized Impl
1000 O(1,000,000) steps O(10,000) steps
10,000 O(100,000,000) steps O(140,000) steps

Int currentTargetLength = lengthIndex[0].first - 1;
while (truncateAmount > 0)
{
currentTargetLength = std::min<Int>(lengthIndex[0].first - 1, currentTargetLength);
for (size_t i = 0; i < lengthIndex.size(); ++i)
{
if (lengthIndex[i].first > currentTargetLength)
{
Int truncateCurrent = std::min<Int>(truncateAmount, lengthIndex[i].first - currentTargetLength);
lengthIndex[i].first -= truncateCurrent;
truncateAmount -= truncateCurrent;
}

if (truncateAmount == 0)
{
break;
}
}
}

// truncate each name to its new length
for (size_t ti = 0; ti < lengthIndex.size(); ++ti)
{
int charsToRemove = playerNames[lengthIndex[ti].second].getLength() - lengthIndex[ti].first;
if (charsToRemove > 0)
{
DEBUG_LOG(("TruncatePlayerNames - truncating '%s' by %d chars to ", playerNames[lengthIndex[ti].second].str(), charsToRemove));
playerNames[lengthIndex[ti].second].removeLastNChars(charsToRemove);
DEBUG_LOG(("'%s' (target length=%d).\n", playerNames[lengthIndex[ti].second].str(), lengthIndex[ti].first));
}
}

return true;
}

AsciiString GameInfoToAsciiString(const GameInfo *game, const AsciiStringVec& playerNames)
{
if (!game)
return AsciiString::TheEmptyString;
Expand All @@ -923,7 +1007,7 @@ AsciiString GameInfoToAsciiString( const GameInfo *game )
newMapName.concat(token);
mapName.nextToken(&token, "\\/");
}
DEBUG_LOG(("Map name is %s\n", mapName.str()));
DEBUG_LOG(("Map name is %s\n", newMapName.str()));
}

AsciiString optionsString;
Expand All @@ -941,23 +1025,13 @@ AsciiString GameInfoToAsciiString( const GameInfo *game )
AsciiString str;
if (slot && slot->isHuman())
{
AsciiString tmp; //all this data goes after name
tmp.format( ",%X,%d,%c%c,%d,%d,%d,%d,%d:",
slot->getIP(), slot->getPort(),
(slot->isAccepted()?'T':'F'),
(slot->hasMap()?'T':'F'),
str.format( "H%s,%X,%d,%c%c,%d,%d,%d,%d,%d:",
playerNames[i].str(), slot->getIP(),
slot->getPort(), (slot->isAccepted() ? 'T' : 'F'),
(slot->hasMap() ? 'T' : 'F'),
slot->getColor(), slot->getPlayerTemplate(),
slot->getStartPos(), slot->getTeamNumber(),
slot->getNATBehavior() );
//make sure name doesn't cause overflow of m_lanMaxOptionsLength
int lenCur = tmp.getLength() + optionsString.getLength() + 2; //+2 for H and trailing ;
int lenRem = m_lanMaxOptionsLength - lenCur; //length remaining before overflowing
int lenMax = lenRem / (MAX_SLOTS-i); //share lenRem with all remaining slots
AsciiString name = WideCharStringToMultiByte(slot->getName().str()).c_str();
while( name.getLength() > lenMax )
name.removeLastChar(); //what a horrible way to truncate. I hate AsciiString.

str.format( "H%s%s", name.str(), tmp.str() );
slot->getNATBehavior());
}
else if (slot && slot->isAI())
{
Expand Down Expand Up @@ -989,13 +1063,38 @@ AsciiString GameInfoToAsciiString( const GameInfo *game )
}
optionsString.concat(';');

DEBUG_ASSERTCRASH(!TheLAN || (optionsString.getLength() < m_lanMaxOptionsLength),
("WARNING: options string is longer than expected! Length is %d, but max is %d!\n",
optionsString.getLength(), m_lanMaxOptionsLength));

return optionsString;
}

AsciiString GameInfoToAsciiString(const GameInfo* game)
{
if (!game)
{
return AsciiString::TheEmptyString;
}

AsciiStringVec playerNames = BuildPlayerNames(*game);
AsciiString infoString = GameInfoToAsciiString(game, playerNames);

// TheSuperHackers @bugfix Safely truncate the game info string by
// stripping characters off of player names if the overall length is too large.
if (TheLAN && (infoString.getLength() > m_lanMaxOptionsLength))
{
const UnsignedInt truncateAmount = infoString.getLength() - m_lanMaxOptionsLength;
if (!TruncatePlayerNames(playerNames, truncateAmount))
{
DEBUG_ASSERTCRASH(infoString.getLength() <= m_lanMaxOptionsLength,
("WARNING: options string is longer than expected! Length is %d, but max is %d. Attempted to truncate player names by %u characters, but was unsuccessful!\n",
infoString.getLength(), m_lanMaxOptionsLength, truncateAmount));
return AsciiString::TheEmptyString;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}

infoString = GameInfoToAsciiString(game, playerNames);
}

return infoString;
}

static Int grabHexInt(const char *s)
{
char tmp[5] = "0xff";
Expand Down
2 changes: 1 addition & 1 deletion GeneralsMD/Code/GameEngine/Source/GameNetwork/LANAPI.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -837,7 +837,7 @@ void LANAPI::RequestGameStartTimer( Int seconds )

void LANAPI::RequestGameOptions( AsciiString gameOptions, Bool isPublic, UnsignedInt ip /* = 0 */ )
{
DEBUG_ASSERTCRASH(gameOptions.getLength() < m_lanMaxOptionsLength, ("Game options string is too long!"));
DEBUG_ASSERTCRASH(gameOptions.getLength() <= m_lanMaxOptionsLength, ("Game options string is too long!"));

if (!m_currentGame)
return;
Expand Down
1 change: 1 addition & 0 deletions GeneralsMD/Code/GameEngine/Source/GameNetwork/LANAPIhandlers.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,7 @@ void LANAPI::handleRequestGameInfo( LANMessage *msg, UnsignedInt senderIP )

AsciiString gameOpts = GameInfoToAsciiString(m_currentGame);
strncpy(reply.GameInfo.options,gameOpts.str(),m_lanMaxOptionsLength);
reply.GameInfo.options[m_lanMaxOptionsLength] = 0;
wcsncpy(reply.GameInfo.gameName, m_currentGame->getName().str(), g_lanGameNameLength);
reply.GameInfo.gameName[g_lanGameNameLength] = 0;
reply.GameInfo.inProgress = m_currentGame->isGameInProgress();
Expand Down
Loading