Refactor the code handling WS data event to remove code duplication and fix the usages of pinfo->opcode in examples

[mathieucarbou]

this is a little refactoring following PR #385.

[Copilot]

Pull request overview

This PR refactors the WebSocket data event handling code to eliminate duplication by extracting repeated logic into a dedicated helper function. The refactoring addresses code that was duplicated across two locations where WebSocket data frames are processed.

Changes:

• Extracted duplicated buffer allocation and event handling logic into a new _handleDataEvent helper method
• Modernized memory management by replacing manual malloc/free calls with std::unique_ptr
• Consolidated error handling for allocation failures into the new helper function

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
src/AsyncWebSocket.h	Added declaration for the new _handleDataEvent private helper method
src/AsyncWebSocket.cpp	Replaced two instances of duplicated code with calls to _handleDataEvent and implemented the new helper method using modern C++ memory management

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[willmmiles]

Looks very good to me, though I haven't personally tested it. We could optimize further, and IMO an opt-out feature would be nice to have, but this is already a clean improvement.

Aside: frankly, seeing this make me wonder if there isn't some small value in doing a similar factor-out-to-function on some of the other handler paths, even though there would be only one call site for each. I find this actually makes the code easier to read, keeping the "what to do with a specific frame" in an easily identifiable section unmixed from the "how we find frames in the packet buffer" loop.

[mathieucarbou]

Aside: frankly, seeing this make me wonder if there isn't some small value in doing a similar factor-out-to-function on some of the other handler paths, even though there would be only one call site for each. I find this actually makes the code easier to read, keeping the "what to do with a specific frame" in an easily identifiable section unmixed from the "how we find frames in the packet buffer" loop.

yes I agree!

[mathieucarbou]

@willmmiles I added a commit based on our discussion here.

I tested sending a long message with websocat with openssl rand -hex 2000 | websocat ws://192.168.4.1/ws.

It turns out the copy will always be used as long as the tcp buffer is filled. But if the last frame is shorter, then the disconnect control frame can be put together by websocat or browser. So for the last frame processing, we can see that a backup is done, then last character restored, then the control frame for the disconnection is processed.

I also fixed another bug where the message_opcode was not initialized in the case of a short message where there is 1 segment 1 frame.

[mathieucarbou]

message_opcode was not properly initialized when the text/binary frame fitted in 1 tcp buffer

=> (datalen + _pinfo.index) == _pinfo.len)

[willmmiles]

@willmmiles I added a commit based on our discussion here.

I tested sending a long message with websocat with openssl rand -hex 2000 | websocat ws://192.168.4.1/ws.

It turns out the copy will always be used as long as the tcp buffer is filled. But if the last frame is shorter, then the disconnect control frame can be put together by websocat or browser. So for the last frame processing, we can see that a backup is done, then last character restored, then the control frame for the disconnection is processed.

I also fixed another bug where the message_opcode was not initialized in the case of a short message where there is 1 segment 1 frame.

The way websocat interacts with the socket API causes the kernel to push a TCP packet at the end of every message -- so yes, it won't trigger the optimization very often. This isn't strictly speaking guaranteed, but in practice the acks come back fast enough that this is pretty much always the case. So as long as you're testing with unmodified websocat, you'll basically always see the end of a message be the end of a TCP packet.

I do not know if browsers behave the same way, but I suspect so. In practice I guess the implication is that the multi-packet optimization doesn't help very often.

I did hack websocat to set the TCP_PUSH socket option to force TCP frames to be filled completely before sending as a way of testing #383 - I can post up those sources later if you'd find them useful. It was helpful because it allowed me to arbitrarily align the header of the next message by changing the previous message length.

[mathieucarbou]

I did hack websocat to set the TCP_PUSH socket option to force TCP frames to be filled completely before sending as a way of testing #383

Ahah that’s clever! I searched for the CLI parameters to tweak the tcp layer but I completely forgot it was OSS! I don’t know though of I would be able to do what you did 😅