SendAsync not sending all data fix plus various stabilizations and improvements #160

frg-kova · 2022-11-27T07:18:32Z

Main fix is for issue #159 where SendAsync wouldn't send all the data on a first call. In attempt to hunt down that bug, I've made quite a bit stabilization discoveries that helped to increase rarity of the main bug as well. They are:

Cleanup of reused pooled objects when entering pool

Guaranteed closed bi channel connection fix with socket close not to leave connection socket open and client in disconnected state. A real bug as well we've found.

ability to reset and reuse the writer as it often does not need to go into the pool but single instance can be reused

not doing infinite timeout (a preference, should be config option really)

allocating buffer to include header and avoid header alloc later

... and lots more comments on how code works as I learned it.

There's a lot! I'm open for a call if something needs explanation.

4real · 2022-12-20T14:02:09Z

My concern is the test suite is failing on my local computer, and not just a few, but most of them. (Whereas on the DR2 master, just a few fail.) That doesn't necessarily imply that your changes are bad, but might imply that the tests need to be updated.

Also, the branch needs to be merged with/rebased on master since now I've gotten around to preparing for a new release by merging PRs. But the higher prio is resolving the test suite failures.

An alternative to reduce complexity is to put your changes into multiple isolated PRs/branches.

frg-kova · 2022-12-20T19:12:59Z

I see how splitting all the changes into isolated small PRs is great and way to go. Having said that there's a lot and I do not have a few days to spend to pull them out one. My working env. is Unity so I cannot switch easily into it and out without spending some time which I don't have ATM.

4real · 2022-12-20T19:29:36Z

No worries, I started working on it.

A lot of thanks for everything you already did (in your free time!) - your code and documentation is very clear.

JamJar00 · 2022-12-20T21:30:34Z

I think part of the code is missing here. All you're doing is adding a random 4 byte padding to the start of every message, but then that 4 byte padding hasn't been taken into account in the deserialization Hence it's not passing any of the tests

Given this comment on the issue, I'm guessing that 4 byte padding is actually supposed to be for the length header on TCP messages? (I also vaguely recall that being a change from when I skimmed through this PR ages ago). If that is the case then this change is incompatible with all the other listeners except the bichannel listeners because none of them will be expecting that change so we should definitely not merge.

Let's get the rest of this PR recovered and then work out whether it's compatible of not though!

frg-kova · 2022-12-21T02:58:35Z

You're right I am working only with BiChannelConnection myself and I have tunnel vision there. Do all messages require headers? If not it's uncompatible, for that specific fix which is a minor thing.

The difference is in BiChannelClientConnection.SendMessageReliable, it used to be forced to expand by 4 bytes and using BufferLists. Receiving remained the same as it's just a matter of buffer space alloc not message structure.

Used to be this

            byte[] header = new byte[4];
            BigEndianHelper.WriteBytes(header, 0, message.Count);

            SocketAsyncEventArgs args = ObjectCache.GetSocketAsyncEventArgs();

            args.SetBuffer(null, 0, 0);
            args.BufferList = new List<ArraySegment<byte>>()
            {
                new ArraySegment<byte>(header),
                new ArraySegment<byte>(message.Buffer, message.Offset, message.Count)
            };

and now is
args.SetBuffer(message.Buffer, message.Offset, message.Count);

JamJar00 · 2022-12-22T19:24:02Z

Ah I think maybe I just missed the minified diff there.

I can't say I'm a big fan of the change in the protocol. It'll add 4 bytes to every UDP message we send and it'll break compatibility with all other listeners. The change has also only been made to the client side, it should also be applied to the server as well.

I do however think we should distil the changes here to the fixes you've identified, because that's all great investigation work and important to get in. If BufferList is causing us issues lets get rid of it, even if that means nother memory copy or something!

frg-kova · 2022-12-22T21:42:39Z

The BufferList isn't a problem, that change is only to get rid of the 4 byte alloc on every send (plus to make things more unified and use SetBuffer as everywhere else). Ignore the change since it breaks other listeners.

Ideally we pass in an argument for additional reserved header size if needed.

LittleCodingFox · 2023-03-27T07:15:30Z