Repository navigation
http2: enforce maxSessionMemory for client requests - #67361
gabedemesa wants to merge 1 commit into
Conversation
|
Review requested:
|
|
Welcome to Node.js, and thank you for your first contribution! Before review, please take a moment to read:
Please make sure every commit is signed off. For a first pull request, GitHub Actions require collaborator approval and Jenkins CI must be started by a collaborator or triager, so an initial wait is normal. |
3cb04f3 to
9e2e2fa
Compare
|
Thanks for working on this. I built it locally and the fix works as described. Three things inline. |
6096c8d to
99f430c
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #67361 +/- ##
==========================================
- Coverage 90.73% 90.69% -0.04%
==========================================
Files 798 802 +4
Lines 280017 282123 +2106
Branches 53852 54364 +512
==========================================
+ Hits 254064 255883 +1819
- Misses 16236 16361 +125
- Partials 9717 9879 +162
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Thanks for the updates. I rebuilt 99f430c and all three points are addressed. The new test passed 300 repeated runs for me. Two non-blocking suggestions inline.
One process note: the commit has a Co-Authored-By: trailer naming an AI tool. AGENTS.md asks for Assisted-by: instead, and the AI guidelines recommend keeping commercial tool names out of commit messages, so that will probably need changing before this can land.
99f430c to
bad0b6e
Compare
|
Thanks for the approval. The commit now uses |
The documentation for `maxSessionMemory` says new streams are rejected while the limit is exceeded. That was only enforced for streams opened by the peer (`Http2Session::CanAddStream()`). `session.request()` went straight to `nghttp2_submit_request()`, so a client whose socket stopped draining could keep queuing requests in nghttp2 without bound. Check the session memory before submitting a request. If it is over the limit, reject only that request with the new `ERR_HTTP2_SESSION_MEMORY_EXCEEDED` error. Nothing is sent to the peer, so the request can be retried, and the session stays usable. As with response headers, request bodies queued for flow control are not counted, so large uploads do not block unrelated requests on the same session. Refs: nodejs/undici#6181 Assisted-by: a closed-source coding agent Signed-off-by: Gabe De Mesa <gdemesa@opengov.com>
bad0b6e to
a8fbede
Compare
The documentation for
maxSessionMemorysays newHttp2Streaminstances are rejected while the limit is exceeded. That was only enforced for streams opened by the peer (Http2Session::CanAddStream()).session.request()went straight tonghttp2_submit_request(), so a client whose socket stopped draining could keep submitting requests, queuing them in nghttp2 without bound.This checks the session memory in
Http2Session::Request()before submitting and fails only that request with a newERR_HTTP2_SESSION_MEMORY_EXCEEDEDerror. Nothing is sent to the peer, so the request is safe to retry, and the session stays usable. As with response headers (#67114), request bodies queued for flow control are not counted, so a large upload does not block unrelated requests on the same session.The rejection is signalled by returning
falsefrom the internalrequest()binding rather than a new constant, to avoid adding to the publichttp2.constantsor theNGHTTP2_ERR_*set thattest-http2-client-onconnect-errorsiterates.Verification
test-http2-max-session-memory-client-request.jsuses a minimal HTTP/2 peer on a plain TCP socket, so it can actually stop reading:ERR_HTTP2_SESSION_MEMORY_EXCEEDEDand the session stays open.The test fails on v26.13.1 and fails if the peer is not stalled. All
test/parallel/test-http2-*andtest/sequential/test-http2-*tests pass locally, as dotest-http2-client-onconnect-errorsandtest-internal-errors.cpplint, ESLint andlint-mdare clean.Not covered
write()backpressure can still queue without bound on that stream (also described in the issue). That is application-level backpressure.maxSessionMemorylimit for locally initiated requests.Refs: nodejs/undici#6181