Skip to content

Ws over h2 pmd fixes#3637

Open
thefallentree wants to merge 1 commit into
warmcat:mainfrom
thefallentree:ws-over-h2-pmd-fixes
Open

Ws over h2 pmd fixes#3637
thefallentree wants to merge 1 commit into
warmcat:mainfrom
thefallentree:ws-over-h2-pmd-fixes

Conversation

@thefallentree

@thefallentree thefallentree commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

ws-over-h2: permessage-deflate short return + ext tx-drain wedge

@thefallentree
thefallentree force-pushed the ws-over-h2-pmd-fixes branch from 44936df to 057a687 Compare July 20, 2026 13:14
…e, client extension offer -- with selftest

Three fixes for permessage-deflate on ws-over-h2 (RFC 8441)
encapsulated streams, plus a ctest-wired selftest:

1) The encapsulated tx path returned the h2 role's write result, which
   reflects the POST-COMPRESSION frame size -- with pmd active that is
   routinely smaller than what the caller passed in, so any
   well-compressed message made callers checking (wr < len) conclude a
   short write and kill a healthy connection.  The lws_write() contract
   is to report how much of the CALLER's payload was accepted, and the
   h1 path already returns orig_len for exactly this reason; do the
   same, still propagating write errors as < 0.

2) When pmd's compressed output exceeds its chunk buffer, the ws role
   sets ws->tx_draining_ext and expects the next POLLOUT to send the
   remaining fragments -- the h1 path services this in
   rops_handle_POLLOUT_ws() priority 5, before letting the user write
   anything new.  An h2-encapsulated child never reaches that path: its
   only POLLOUT servicing is the child loop in
   rops_perform_user_POLLOUT_h2(), which went straight to the user
   callback.  Meanwhile lws_send_pipe_choked() reports choked while
   tx_draining_ext is set, so a user write loop gated on it never
   writes again either: the drain never advances and the stream wedges
   permanently.  Service the drain from the child loop, ahead of the
   user callback.  It slots in after the generic 'deal with partial at
   nwsi' branch that 774c64a ("ws-over-h2: deal with choked nwsi")
   routes carries-ws children through, preserving its ordering: a child
   on a backed-up network socket still defers there first, the same
   priority h1 connections get, and the extension drain only runs once
   the nwsi partial has cleared.

3) lws_h2_client_handshake() only emitted sec-websocket-version and
   sec-websocket-protocol, so an lws client could never negotiate
   permessage-deflate (or any extension) over h2 even against a willing
   server.  RFC 8441 Sect 5 carries the ws handshake headers unchanged
   over extended CONNECT, so build the same offer list as
   lws_generate_client_ws_handshake() and emit it after the
   pseudo-headers; the response side already instantiates accepted
   extensions in the shared lws_client_ws_upgrade() path.

api-test-ws-h2-pmd (needs LWS_WITHOUT_EXTENSIONS=OFF): single-process
h2 ws server + client, both offering pmd; the server bulk-sends a 64KB
deterministic pattern in 4KB messages gated only on
lws_send_pipe_choked(), alternating trivially-compressible blocks
(deflated frame far smaller than the payload) with incompressible hash
noise (deflated output overflows pmd's default 1KB chunk buffer,
forcing tx_draining_ext).  It fails if the connection is not genuinely
ws-over-h2, if the extended CONNECT did not offer pmd, if any
lws_write() reports fewer bytes accepted than requested, if the pattern
corrupts, or if the transfer stalls.  Reverting any one of the three
fixes makes it fail in the corresponding distinct way.
@thefallentree
thefallentree force-pushed the ws-over-h2-pmd-fixes branch from 057a687 to d4e6802 Compare July 20, 2026 19:34
@lws-team

Copy link
Copy Markdown
Member

Uh... this was 4 patches which I have taken into _temp... now it's 1 patch... this is replacing the four patches, or it's in addition to the four patches? Or something else entirely?

@thefallentree

Copy link
Copy Markdown
Contributor Author

I am sorry — it was just squashed from previous commits. Nothing new

@thefallentree

Copy link
Copy Markdown
Contributor Author

Actually this is a follow up fix for the commits you merged

@sonarqubecloud

Copy link
Copy Markdown

@lws-team

Copy link
Copy Markdown
Member

So for what I have on _temp branch, trying to add what you have on your branch now - d4e6802 - gives an empty patch. So I think I am up to date. I will push all this to main later today

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