Repository navigation
http2: enforce maxSessionMemory for client requests - #67361
gabedemesa wants to merge 1 commit into
Conversation
|
Review requested:
|
nodejs-github-bot
added
c++
|
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. |
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()`. A client whose socket stopped draining could therefore keep submitting requests and queue frames in `outgoing_buffers_` without bound. Check the session memory before submitting a request and fail only that request with `ERR_HTTP2_STREAM_ERROR` (`NGHTTP2_ENHANCE_YOUR_CALM`), leaving the session 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. Also count the `outgoing_buffers_` descriptors towards the session memory; each queued write costs 32 bytes regardless of its length. Refs: nodejs/undici#6181 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Signed-off-by: gdemesa <gdemesa@opengov.com>
3cb04f3 to
9e2e2fa
Compare
| // A peer that accepts the connection and then stops reading. Before the | ||
| // session memory check, every request was queued regardless of the limit. | ||
| const server = http2.createServer(); | ||
| server.on('connection', (socket) => setTimeout(() => socket.pause(), 30)); |
There was a problem hiding this comment.
The test still passes for me with this line removed. All 30,000 requests are submitted synchronously, so nothing has been written to the socket yet and the pause has no effect. As written this covers a synchronous burst, not a peer that stops reading.
Could you also add a check that the session is still usable after a rejection? That's the main claim of the PR and nothing asserts it.
| if (ret === false) { | ||
| // The session is over its `maxSessionMemory` budget. Only this request is | ||
| // rejected; the session itself stays usable. | ||
| this.destroy(new ERR_HTTP2_STREAM_ERROR( | ||
| nameForErrorCode[NGHTTP2_ENHANCE_YOUR_CALM])); | ||
| return; | ||
| } |
There was a problem hiding this comment.
The error message says NGHTTP2_ENHANCE_YOUR_CALM, but stream.rstCode ends up as 2 (NGHTTP2_INTERNAL_ERROR) when I run it. These should agree.
Also, this is the same error a caller gets when the peer resets the stream, so there's no way to tell that the request was never sent and is safe to retry. Is that intended?
| uint64_t total = current_session_memory_ + sizeof(Http2Session); | ||
| total += current_nghttp2_memory_; | ||
| total += outgoing_storage_.size(); | ||
| total += outgoing_buffers_.size() * sizeof(NgHttp2StreamWrite); |
There was a problem hiding this comment.
I don't think outgoing_buffers_ is what grows here. It only holds one SendPendingData() batch. The requests pile up in nghttp2's own queue, which current_nghttp2_memory_ already counts.
So this line doesn't seem needed for the fix, and it also changes the check for server sessions. Could it be dropped, and the commit message updated to match?
|
Thanks for working on this. I built it locally and the fix works as described. Three things inline. |
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 and queue frames inoutgoing_buffers_without bound.This checks the session memory in
Http2Session::Request()before submitting and fails only that request withERR_HTTP2_STREAM_ERROR(NGHTTP2_ENHANCE_YOUR_CALM). 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. Theoutgoing_buffers_descriptors (32 bytes per queued write) are now counted towards the session memory.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
30,000 GETs submitted with
maxSessionMemory: 1to a peer that accepts the connection and then pauses its socket (repro from nodejs/undici#6181):outboundQueueSizeENHANCE_YOUR_CALM)All
test/parallel/test-http2-*andtest/sequential/test-http2-*tests pass locally. The newtest-http2-max-session-memory-client-request.jsfails without the change.cpplintis clean; ESLint was not run locally.Not covered
write()backpressure can still queue without bound on that stream (also described in the issue). That is application-level backpressure.Refs: nodejs/undici#6181
🤖 Generated with Claude Code