Skip to content

fix(http): parse ws frames wrapped in the handshake TCP segment - #895

Closed
godsonhyl wants to merge 1 commit into
ithewei:masterfrom
godsonhyl:fix-ws-first-frame-after-upgrade
Closed

godsonhyl wants to merge 1 commit into
ithewei:masterfrom
godsonhyl:fix-ws-first-frame-after-upgrade

Conversation

@godsonhyl

Copy link
Copy Markdown

fix(http): parse ws frames wrapped in the handshake TCP segment

Problem

A WebSocket client may legally send its first frame in the same TCP segment as the
upgrade request (nothing in RFC 6455 forbids pipelining the first frame onto the
handshake). On the server this kills the channel deterministically:

  1. HttpHandler::FeedRecvData dispatches the buffer through the HTTP_V1 branch →
    http_parser_execute stops at the request boundary, so the frame bytes are the
    unparsed tail of the same buffer (nfeed < len).
  2. The HP_MESSAGE_COMPLETE hook runs synchronously inside that parse: the 101 is
    sent and SwitchWebSocket() flips protocol to WEBSOCKET — but the http parser
    still reports partial consumption for this call.
  3. on_recv (HttpServer.cpp) sees nfeed != readbytes → logs
    http parse error: success → closes the perfectly healthy upgraded io.

The client observes an open→close loop on every reconnect (it sends immediately, so
it can never stay connected). Sending only after the 101 arrives (e.g. waiting for
onopen, or a fixed delay) hides the bug — which is why the existing examples never
hit it.

Secondary bug fixed in the same path

SwitchWebSocket() left last_recv_pong_time/last_send_ping_time at their
constructor value 0. The first heartbeat tick therefore always sees
last_recv_pong_time < last_send_ping_time and closes the channel before any
ping/pong round-trip has had a chance to happen
, whenever the client has not
already sent a pong. Both timestamps are now seeded at upgrade time.

Fix

  • HttpHandler gains a ws_upgraded bit, set by SwitchWebSocket().
  • The HTTP_V1 recv branch, after feeding the http parser, checks for a completed
    upgrade + leftover bytes and feeds exactly that tail to ws_parser instead of
    failing. Real pipelined HTTP requests are unaffected: the tail path only triggers
    once protocol == WEBSOCKET.

Repro

Server with HttpService onopen/onmessage (or any of the bundled ws examples with
the client-side sleep removed); client that sends a text frame right after writing
the handshake (single TCP segment):

# before: [GET /market/<token>]=>[101 Switching Protocols]
#         ERROR ... http parse error: success [HttpHandler.cpp FeedRecvData]
#         io closed -> reconnect loop
# after:  101 + frame delivered to onmessage, channel stays open

Verified with a real client/server pair (login + market-subscribe traffic) plus the
bundled http/ws test examples — no regressions.

🤖 Generated with Claude Code

A websocket client may send its first frame in the same TCP segment as
the upgrade request. On the server, that frame is the unparsed tail of
the recv buffer handed to HttpHandler::FeedRecvData: the MESSAGE_COMPLETE
hook runs synchronously inside http-parser and emits the 101, but the
http parser stops at the request boundary and returns nfeed < len, so
on_recv saw 'http parse error: success' and closed the healthy upgraded
io -- the channel died on the client's first message.

Track the completed upgrade with a ws_upgraded flag and, when set, feed
the remaining bytes of the same buffer to ws_parser instead of failing.
Also seed last_recv_pong_time/last_send_ping_time at upgrade time: both
start at 0, so the first heartbeat tick would see recv < send and close
the channel before any ping/pong round-trip happened.

Co-Authored-By: Claude Code <noreply@anthropic.com>
@ithewei

ithewei commented Sep 30, 2026

Copy link
Copy Markdown
Owner

Thanks for the detailed report.
RFC 6455 §4.1 requires the client to wait for the server's response before sending anything else: "Once the client's opening handshake has been sent, the client MUST wait for a response from the server before sending any further data." A client that sends its first frame together with the upgrade request is therefore non-conforming. The server may still reject the upgrade (401/403/426/3xx), and in that case those bytes would be parsed as the next HTTP request. We'd rather not special-case this on the server side, so please have the client wait for the 101 before sending (e.g. send from onopen).

On the heartbeat change: with both timestamps at 0, the first tick evaluates 0 < 0 as false and sends a ping. The channel is only closed if no pong has arrived by the second tick, so seeding the timestamps doesn't change behavior.

@godsonhyl

Copy link
Copy Markdown
Author

Thanks for the detailed review — agreed on both points.

You are right that per RFC 6455 §4.1 a conforming client must wait for the 101 before sending, and right that the heartbeat seeding was a no-op (first tick evaluates 0 < 0 as false and sends a ping, so the close only ever happens on the second tick without a pong). That part of the patch can be dropped entirely.

On the server-side tolerance itself: we will keep the ws_upgraded tail-handoff in our downstream fork, since the strict behavior did close healthy sessions in practice when a client sent its first frame with the upgrade request — but I understand you prefer not to special-case non-conforming clients upstream, so closing this here rather than pushing further.

Closing this PR; thanks again for taking the time to look at it.

@godsonhyl godsonhyl closed this Sep 30, 2026
godsonhyl pushed a commit to godsonhyl/libhv that referenced this pull request Sep 30, 2026
…osed upstream, RFC 6455 argues clients must wait for 101)

Upstream (ithewei/libhv) rejected the server-side special case in PR ithewei#895:
a conforming client MUST wait for the 101 before sending (RFC 6455 4.1),
and the heartbeat timestamp seeding was a no-op (first tick: 0 < 0 is
false -> sends ping). Revert the working tree to the 0db9c63 pin content;
clients must send from OnOpen. Original delta kept on branch
fix-ws-first-frame-after-upgrade for reference.

Co-Authored-By: Claude Code <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants