Fix: Control frame processing in the WebSocket client

[Yves57]

Several fixes:

• If a control frame payload is empty (the most frequent case), _nx_websocket_client_packet_trim() (line ~1910) trims the whole packet (if there no other pending frame). And so _nx_websocket_client_data_process() was stopping with a status code NX_CONTINUE instead of continuing the processing. The fix has several effets:
  • If a CLOSE message is received, the callback set with nx_websocket_client_connection_status_callback_set() is called now, and NetXDuo returns the expected status code NX_WEBSOCKET_DISCONNECTED to the user code.
  • If a PING or a PONG message is received, we correctly jump to the switch/case.
• If a PING message is received, according to RFC 6455 Section 5.5.2 / 5.5.3, we return a PONG message with the same payload. Fixes WebSocket: Client does not respond with Pong after receiving Ping #368 .
• If a PONG message is received with an empty payload, it is not returned anymore to the end-user at the moment of a next message is received by the WebSocket stack.

PS: I think a remaining bug is not fixed yet. In the case of "interleaved" frames (for example a PING frame received in the middle of several fragmented data frames), the next data frames are not correctly processed.

[Yves57]

We release the mutex just because _nx_websocket_client_send() will unlock/lock it. Do we need to create a separate function _nx_websocket_client_send_internal() to avoid redundant lock/unlocks?
I'm not sure if it can lead or not a to race condition (like line ~2657) if a message is received exactly at this moment (necessarly from a different thread).

[Yves57]

To be moved in the NX_WEBSOCKET_CLIENT struct?

[fdesbiens]

Thanks Yves — the analysis behind this is right, and #368 is a real bug worth fixing. Your central observation is correct: when a control frame has no payload, trimming the header consumes the whole packet, _nx_websocket_client_packet_trim() returns NX_NO_PACKET, and the old unconditional return(NX_CONTINUE) meant the frame switch was never reached. CLOSE never reached the status callback and PING was never answered.

I have pushed two commits to your branch. Please read the second half of this note before picking the work back up: the branch is currently red, and not only for the reason I fixed.

What I changed

82477bc — Guarded the empty control frame paths in data_process

Removing that early return also removed the only thing keeping *packet_ptr == NX_NULL out of the switch. _nx_websocket_client_packet_trim() returns NX_NO_PACKET only when it has released the packet and cleared the pointer — that is the if (*packet_ptr) { ... return(NX_WEBSOCKET_SUCCESS); } followed by return(NX_NO_PACKET) at the end of it. Your if (*packet_ptr) guard around the unmask block shows you had this in mind, but it needed to extend into the switch, where three of the four arms dereference the pointer:

• PING called _nx_packet_data_extract_offset(), whose first act is reading nx_packet_length through the pointer. It is the internal entry point, so there is no error-checking wrapper to catch the NULL.
• PONG called _nx_websocket_client_packet_trim(), whose first statement is the same kind of read.
• The data arms read nx_packet_append_ptr, so a zero-length TEXT or BINARY frame faulted too — and RFC 6455 permits those.

Only CLOSE survived, because it never touches the packet. That is almost certainly why the CLOSE behaviour you were chasing looked fixed while the rest was not.

A lone empty PING is exactly two bytes on the wire, so this is the ordinary keepalive shape rather than a corner case. It only appears intermittent because a following frame in the same segment leaves the pointer non-NULL and the fault does not occur.

Four other things in the same paths, in decreasing order of how much they mattered:

The PONG send now keeps the mutex. You released it around _nx_websocket_client_send(), which is correct in itself — that function acquires the mutex on entry, so it must be called unlocked. But by that point the trimmed packet had already been stored into nx_websocket_client_processing_packet, so during the window another thread calling nx_websocket_client_disconnect() could run _nx_websocket_client_cleanup() and release it. I moved the send into a helper that holds the mutex throughout instead: ThreadX mutexes are recursive, so _nx_websocket_client_send()'s nested acquire and release leave our ownership intact, and the window disappears rather than being narrowed.

Every wait in that helper is NX_NO_WAIT. Allocating with NX_WAIT_FOREVER while holding the client mutex deadlocks on an exhausted pool: the threads that would free packets need the same mutex. Note the other two allocation sites in the file pass the caller's wait_option rather than hardcoding a wait. Relatedly, failing to send a PONG no longer calls _nx_websocket_client_cleanup() — a lost PONG costs a keepalive round trip, which is not worth tearing down a working connection for.

bytes_copied is now checked. _nx_packet_data_extract_offset() reports a short copy through that parameter rather than through its status, so an unchecked result could echo uninitialised stack bytes from ping_payload to the peer.

NX_ASSERT no longer stands in for a status check. It expands to nothing in a default build (nx_api.h:169), so the trim result was being discarded; and when asserts are enabled, NX_ASSERT_FAIL is for (;;) { tx_thread_sleep(NX_WAIT_FOREVER); }, which hangs the thread rather than reporting anything. Either the condition cannot happen and the status should not be captured, or it can and needs handling — I went with handling it.

Also: the payload buffer is scoped to the PING case rather than the function, so the 125 bytes are not on the stack of every frame received; and the four tab characters are now spaces, since the file has none elsewhere.

e85cf6e — Added a regression test for empty WebSocket frames

test/regression/websocket_test/netx_websocket_empty_control_frame_test.c, registered in the netxduo regression CMake list. It sends an empty PING, PONG, data frame and CLOSE, each alone in its own TCP segment so the NULL case is genuinely reached, and checks the PING is answered with an empty PONG, that nothing is handed to the application as data, that the CLOSE reaches the callback and returns NX_WEBSOCKET_DISCONNECTED, that the mutex is released on every path, and that no packet is released twice.

I confirmed it discriminates rather than assuming it: on the parent commit it segfaults; with the fix it passes.

The part that is still open, and why the branch is red

netx_websocket_one_packet_with_multi_frames_test fails on this branch. It passes on dev. It failed before my commits and it still fails after them — my changes neither caused nor fixed it.

This is the case you flagged in your PR description: "in the case of interleaved frames (for example a PING frame received in the middle of several fragmented data frames), the next data frames are not correctly processed." I would put it more strongly than the description does. That is not a pre-existing limitation left unfixed — it is a regression this PR introduces, and an existing test already catches it, as a segfault rather than a failed assertion.

The test's server_response_frame_packet_1_3 is precisely that shape: the tail of a fragmented binary frame, then a PING with a two-byte payload, then a complete binary frame, all in one segment. The crash surfaces inside the test, at the point where it dereferences a packet after a non-zero status, but the cause is upstream: after the PING is handled, the frame that followed it is no longer delivered where the test expects it.

I instrumented both builds and compared the frame traces. The first ten frames are identical, PING and following binary frame included; dev then goes on to process four more. So the divergence is in sequencing after a control frame, not in the PING handling itself.

I stopped there rather than guessing. Getting it right means reworking how nx_websocket_client_frame_fragmented, nx_websocket_client_frame_opcode and nx_websocket_client_frame_header_found interact when a control frame arrives mid-fragmentation, and I would be inventing the intended semantics rather than restoring known-good behaviour. That is your call to make, and it is the work your PS was pointing at.

So: two of the three things in this PR are now done and covered by a test, and the third has gone from a note in the description to a failing test you can iterate against. If it would help to talk through the interleaving semantics before you change the state machine, I am happy to.

One practical note: I only added commits, so a plain git pull will bring them down — no reset needed.