WebSockets client: handle TCP fragmentation and simplify code

[argosphil]

Fixes #9052.

What does this PR do?

This handles TCP fragmentation (TCP bytes being delivered in smaller chunks than they were sent) in the WebSockets client. It's different from WebSockets fragmentation, where one data chunk is split between several WebSockets frames, which may or may not be sent as separate TCP fragments.

• Code changes

How did you verify your code works?

I ran the Autobahn test suite. This isn't sufficient as it doesn't appear to split packets into individual octets.

• I checked the lifetime of memory allocated to verify it's (1) freed and (2) only freed when it should be [not yet!]
• I included a test for the new code, or an existing test covers it [kinda]
• JSValue used outside outside of the stack is either wrapped in a JSC.Strong or is JSValueProtect'ed
• I wrote TypeScript/JavaScript tests and they pass locally (bun-debug test test-file-name.test) [more tests needed]

[Jarred-Sumner]

Thank you for this, but I think we should avoid the extra memory allocations when possible. We only need to dynamically allocate when we don't receive enough data, and that can go into a single ArrayList which is reused. Additionally, we can use bun.copy to handle copying potentially overlapping data efficiently (without temporary allocations)

Also, when making a slice have length zero, its best to use either &.{} or "" instead of slice = slice[0..0] because slice[0..0] will preserve the original pointer which makes it easier to turn into a UAF later on.

[argosphil]

Thank you for this, but I think we should avoid the extra memory allocations when possible.

We only need to dynamically allocate when we don't receive enough data,

That's what the existing PR does. The fast path is fundamentally unchanged (it checks a slice for .len > 0 rather than checking the initial_handler pointer for != null).

However, if we do hit the slow path, there are some unnecessary allocations (we can avoid the extra allocation entirely or do it on the stack). I'll get rid of them.

and that can go into a single ArrayList which is reused.

I'm not sure I see why an ArrayList would be beneficial.

Additionally, we can use bun.copy to handle copying potentially overlapping data efficiently (without temporary allocations)
Also, when making a slice have length zero, its best to use either &.{} or "" instead of slice = slice[0..0] because slice[0..0] will preserve the original pointer which makes it easier to turn into a UAF later on.

Thanks, I'll make those changes!

ETA: changes made. Not using an ArrayList for now, as it doesn't appear to be needed.

[lithdew]

Unsure if the intent is to have all the refactors previously discussed for the websocket client to be done in this PR, but I feel the logic here is a bit messy/expensive as the websocket frame parser by itself shouldn't ever need any allocations in any fast/slow path. The only allocations ever needed is in the buffering of data in the receive_buffer of the client.

Once I get some freelance work done in roughly a few weeks, I can get to work on implementing an allocation-free standalone version of the frame parser w/ tests in a separate file in the websockets dir. This should help fully isolate concerns around allocations away to the client and away from the frame parser itself (and make the implementation extremely efficient IMO).

If you have time and want to get this done ASAP though @argosphil, I can point to some reference code on Discord that an implementation can be based off of as a start.

[argosphil]

Unsure if the intent is to have all the refactors previously discussed for the websocket client to be done in this PR,

It isn't, it's just to have as little code as possible in the rarely-exercised path of frame fragmentation.

but I feel the logic here is a bit messy/expensive as the websocket frame parser by itself shouldn't ever need any allocations in any fast/slow path. The only allocations ever needed is in the buffering of data in the receive_buffer of the client.

What about the initial data, which can have any length and needs to be buffered? I feel it's best to piggyback onto that rather than having yet another slow code path for the (fixed-length, for now, admittedly) control frame fragmentation logic.

Once I get some freelance work done in roughly a few weeks, I can get to work on implementing an allocation-free standalone version of the frame parser w/ tests in a separate file in the websockets dir. This should help fully isolate concerns around allocations away to the client and away from the frame parser itself (and make the implementation extremely efficient IMO).

I think the fragmented control frame case is really rare, while the initial data case is somewhat more common. Handling both through dynamic allocation seems sensible to me, since it's needed in the common case and it makes the rare case use code which is exercised at least once in a while.

As for efficiency, I think the additional icache pressure of having extra code outweighs the benefit of avoiding dynamic allocation in the fragmented control frame case. The initial data case might be another question entirely.

If you have time and want to get this done ASAP though @argosphil, I can point to some reference code on Discord that an implementation can be based off of as a start.

I'd love that, feel free to message me in whichever way is most convenient to you.