Split and Send Conveniences.

[mukunda-]

From what I understand vaguely by looking at the code, you're expecting the user to handle constructing their split messages when they receive them. It'd be convenient if Chomp did that already: buffered incoming messages and only did the user callback when they have a complete packet.

One thing to also watch out with this is message ordering (forgive me if I missed something in the code, but I see no accommodation for it).

For example,

BNSendGameData( account, "1" )
BNSendGameData( account, "2" )

could easily appear on another's client with the "2" first, corrupting your data if you don't account for that quirk.

One way to handle it is to insert in the message prefix a serial number, a split index, and the total number of messages, so you know where to put things when buffering. It gets messy quick, and I have no idea why Blizzard can't guarantee message ordering already; it's like dealing with data without the TCP protocol.

[ghost]

I have no idea why Blizzard can't guarantee message ordering already

I haven't actually seen out-of-order messages received, but I'll admit I have not been testing BattleNet messages intensely since around when they were introduced. Have you actually seen this occurring lately, or was this a theoretical concern?

I know the in-game messages are a guaranteed order (...or at least they have been so far, I haven't extensively tested it in BfA...), but if the BattleNet ones aren't... Whoof, I'll definitely get on that issue.

It'd be convenient if Chomp did that already: buffered incoming messages and only did the user callback when they have a complete packet.

I'll add a per-prefix toggle for reassembly vs on-the-fly delivery in the near future.

The design as it is currently was largely based around the needs of RP profile addons, which can sometimes have an incredibly loooooong incoming queue, and getting to load bits of that data before the final message is received can be incredibly valuable. But I see the use of having Chomp assemble messages, too, in cases where the message is only valuable if the entire thing is present.

[mukunda-]

The way I see it, the RP profile addons still buffer everything first, but (at least with TRP) they split it into different parts that are sent (which are whole packets), so you receive some parts before the others. Having that in the Chomp layer would be really convenient, just like when you're using AceComm.

As for the out of order issue, it's definitely a problem in-game with both BNet whispers and BNet data. I've been doing a lot of tests lately to verify this kind of stuff, and you can see it easily by doing two or more calls to BNSendGameData at once and then watching the events. Another addon that has this problem is RP Connect, which sends your raid posts through a relay using BNet GameData, and often the messages for multipara emotes are out of order.

Also, this has been a problem for a long time, not just BFA. I first verified it with BNet whispers about a year ago, and then recently verified it for GameData.

[mukunda-]

Here's a test case for the out of order issue:

local received_index = nil

local frame = CreateFrame( "Frame" )
frame:RegisterEvent( "BN_CHAT_MSG_ADDON" )
frame:SetScript( "OnEvent", function( self, event, prefix, message, _, senderGameID )
    if prefix == "ORDERTEST" then
        message = tonumber(message)
        print( "got " .. message .. "!" )
        if message == 1 then
            received_index = message
        elseif received_index and message == received_index + 1 then
            received_index = message
        else
            print( "out of order error!" )
            received_index = math.max( received_index, message )
        end
    end
end)

function SlashCmdList.ORDERTEST(msg)
    local bnetGameID = select( 16, BNGetGameAccountInfoByGUID( UnitGUID("player") ))

    print( "Sending messages . . ." )
    for i = 1, 100 do
        BNSendGameData( bnetGameID, "ORDERTEST", tostring(i) )
    end
end
SLASH_ORDERTEST1 = "/ordertest"

Honestly I think this should really be fixed by the devs, but :man_shrugging:.

[ghost]

Damn, that's awful. I'm pretty convinced it did not used to be that way, but maybe I was just obscenely lucky. (Edit: Actually, I thought of something else. I've always delayed the sending of more than 2 messages -- it's one full message per second after the initial two-message burst, or I found it started dropping messages. That probably has saved me a lot of headache by reducing the odds of the issue occurring since I was always sending maximum-sized messages when possible.)

I'll look at adding a sequence marker of some sort for BattleNet messages in Chomp. I'm probably going to use a message marker, a 24-bit integer (session sequence), and two 12-bit integers (message sequence/message total), base64-encoded. That gives 16 million session sequences (which will wrap if it hits maximum, starting from a randomly-generated value -- this is to have some safety in case of unexpected UI reloads or relogs), and a maximum message sequence length of 4096 messages. I think those values give enough uniqueness and overhead.

So a message would look something like CHMPABCXABAC[...] with that scheme, prior to being processed by Chomp. Addons would see none of that, they'd just find their maximum message sizes reduced to 4066 bytes from 4078. Any concerns with this technique?

[mukunda-]

My input on those points:

Don't really need a random startup value. Reloads/relogs should take enough time for the queue to empty, or at least whatever messages on the line to be received before you send more.

Avoid base64 - that's just an extra library when you can just encode it in plain text for Lua to have an easy time with it (example: "23/1/3" for sequence/page/#pages). (and I think sequence of 2 digits 00-99 is more than enough)

As for addon prefixes, I saw that Chomp supports different prefixes for different parts of the message, I'd recommend not encouraging that practice, so it's easy to see what messages are from what addon or system.

[ghost]

Good points. I'll use hex digits, but I'm sticking to a guaranteed-size preamble. It'll be 12 hex digits, indicating the session sequence, message number, and total messages. I'm keeping the random startup value and modulo handling for now (it's easy to change later), but I'll think more about whether the consequences of not doing so are actually valid.

Use of multiple prefixes is definitely not encouraged. It's only supported because I have to deal with a legacy protocol from the TBC-era that did so and is implemented across multiple addons. I'll make that clearer in the documentation, and maybe un-document how to actually use multiple prefixes.

[mukunda-]

I see.

And I'm fine with however you want to implement it in the end - just my opinions and it's not like it's going to affect the user-side.

[ghost]

I'm mainly worried about an off-by-one issue on for the total message number, if the preamble isn't a fixed size. I could probably account for that in some other way, but I think I feel most comfortable this way.

For purposes of my implementation, are you planning to send a bunch of small order-essential messages over BattleNet? In the current implementation I'm testing, that would not be order-guaranteed by Chomp's parsing. Or is that just the test case for the issue, and your actual messages will only be multi-part if they exceed the maximum message length?

[mukunda-]

It's a tricky question. I want to say that it's fine so long as whole packets are coming in safely - not much you can do when it comes to the protocol above not accounting for the ordering quirk. But, I can imagine that in practical use cases, ordering of smaller individual messages is important too. I can also imagine this gets hairy quickly - you'd have to keep track of a lot more serials—per client, and per message channel (each channel would guarantee ordering of its different messages). Lots of dangers and gotchas there, but it's a headache that the lib user would totally like to avoid.

For one of my use case ideas, I was thinking of building a cross-faction/realm layer for TRP3, where I'd reroute invalid addon messages (targeting opposite faction) over bnet if it can (which Chomp's "smart" function does), and then feed the data to TRP3's event handlers manually on the other side. I believe this might be part of TRP3's core soon though.

[ghost]

I was talking to Ellypse about all this over the past few days, and my understanding is that, yes, TRP3 will probably end up adopting Chomp and getting BattleNet as a part of the package for doing so.

I modified your test script to add a delay between messages, and it looks like a delay of ~0.125 seconds between each message is necessary to avoid the out-of-order issue (though minor latency spikes can throw that off -- I saw the very rare out of order error at 125ms, and couldn't trigger any at 130ms after several runs).

If I had to guess, that makes it seem like Blizzard's Battle.net messaging servers are processing messages 8 times per second, and have multiple processes plucking any pending messages from the queues when that happens. The modified script is below (modify the DELAY and TOTAL_MSG variables for testing).

local DELAY = 0.130
local MSG_TOTAL = 100

local received_index = nil

local frame = CreateFrame( "Frame" )
frame:RegisterEvent( "BN_CHAT_MSG_ADDON" )
frame:SetScript( "OnEvent", function( self, event, prefix, message, _, senderGameID )
    if prefix == "ORDERTEST" then
        message = tonumber(message)
        print( "got " .. message .. "!" )
        if message == 1 then
            received_index = message
        elseif received_index and message == received_index + 1 then
            received_index = message
        else
            print( "out of order error!" )
            received_index = math.max( received_index, message )
        end
    end
end)

local i = 1
local SendTest
local bnetGameID
function SendTest()
    BNSendGameData( bnetGameID, "ORDERTEST", tostring(i) )
    if i < MSG_TOTAL then
        i = i + 1
        C_Timer.After(DELAY, SendTest)
    end
end
function SlashCmdList.ORDERTEST(msg)
    bnetGameID = select( 16, BNGetGameAccountInfoByGUID( UnitGUID("player") ))

    received_index = 0
    print( "Sending messages . . ." )
    i = 1
    C_Timer.After(DELAY, SendTest)
end
SLASH_ORDERTEST1 = "/ordertest"

There isn't much I can think of to do with this knowledge at the moment, but it might be useful in the future somehow. Chomp's pool tick rate is already at 200ms, so if you're sending full-size BattleNet messages, even without the ordering compensation, it should deliver in-order almost always. The first two messages in the sequence would still be a problem, though, regardless.

[mukunda-]

"almost always" :frowning_face:

Why or how it's plucking messages out of the queue randomly on Blizzard's end is boggling; now we have to suffer. I imagine that even if you wait 1000ms, clients on either side may get a latency spike and it will throw it off.

[ghost]

Yeah, it will always be possible to receive out-of-order messages, regardless of how much work you do with delays and other tactics, unless you actually buffer them on your end. Speaking of, that latest commit referenced adds the fullMsgOnly mode to the Smart handling, have a look and see if it fits your needs.

The needBuffer-only mode is mostly complete, but I need to add a status return of some sort to non-fullMsgOnly needBuffer handlers to inform them of the current queue state (first message, middle message, last message, or only message -- probably just going to add a final return value of "FIRST", "MIDDLE", "LAST", or "ONLY", I guess).

You may note some additional complexity. Due to the necessity of logged messages being split in unique ways that may alter their byte total (not splicing quoted-printable characters, nor UTF-8 byte sequences), the msgTotal count is now sometimes updated. The msgTotal count on messages n and n-1 should always be accurate, so it shouldn't ever erroneously finish processing at message n-1. I did test this, but it's always possible with something like this that some specific set of conditions I missed could throw it off.

[mukunda-]

I likely wouldn't be looking at internals anyway when the message magic is going on. I don't have a project to use this quite yet, but I'll let you know if I run into any troubles.

[ghost]

Alright. I'm going to mark this issue as closed for now, but feel free to file another issue if you run into any problems ever, of course. The remaining bits I noted I need to work on from here are now specifically covered by #6