Large messages can clog ReliableOrderedChannel indefinitely

[sherief]

This occurred on a configuration with .maxPacketSize == 8192. A message was generated, which serialized to 119500 bits. In Connection::GeneratePacket(), the availableBits is 65520, and as that makes the call to ReliableOrderedChannel::GetMessagesToSend(), the condition at https://github.com/networkprotocol/yojimbo/blob/master/yojimbo.cpp#L1828 never fires since entry->messageBits is 119500 and so availableBits >= (int) entry->measuredBits always evaluates to false.

What do you think the behavior should be here? That packet is too large, but an early error would be much preferred to a quiet clog of the channel for the rest of the connection's lifetime.

[gafferongames]

I agree that an early out with an error (assert) is appropriate, since this is on send, and you should configure the channel size to be larger in this case. -- cheers

[dbechrd]

I just encountered this exact same problem and it took me quite awhile in the debugger looking through the yojimbo internals to figure out why my packet just silently disappeared on a reliable channel. This definitely needs an assert or some kind of obvious error. How can I even preventatively check for this before calling SendMessage()? I don't see any obvious way in my Serialize() function to know when I've serialized too much data, i.e. such that I can return false.

[dbechrd]

I was reading the code a bit more and just noticed this:

#if YOJIMBO_DEBUG_MESSAGE_BUDGET
            if ( channelConfig.packetBudget > 0 )
            {
                yojimbo_assert( stream.GetBitsProcessed() - startBits <= channelConfig.packetBudget * 8 );
            }
#endif // #if YOJIMBO_DEBUG_MESSAGE_BUDGET

But it doesn't run in my case since packetBudget is -1 by default.

I don't think @sherief 's commit is quite the correct, because they assert on availableBits, which can legitimately run out if you put enough messages into the packet. A better assert would be something akin to:

if (numMessageIds == 0) {
    // Increase your max packet size!
    yojimbo_assert( entry->measuredBits <= availableBits );
}

This assumes that yojimbo attempts to send the messages in the order they're submitted to the queue, such that the message that's too big will eventually make its way to the front of the queue and hit the assert. That seems like a reasonable assumption to me, but someone would have to sanity check it.