Repository navigation
http2: enforce maxSessionMemory for client requests #67361
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -832,6 +832,7 @@ class Http2Session : public AsyncWrap, | |
| uint64_t total = current_session_memory_ + sizeof(Http2Session); | ||
| total += current_nghttp2_memory_; | ||
| total += outgoing_storage_.size(); | ||
| total += outgoing_buffers_.size() * sizeof(NgHttp2StreamWrite); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think 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? |
||
| return total; | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,95 @@ | ||
| 'use strict'; | ||
|
|
||
| const common = require('../common'); | ||
| if (!common.hasCrypto) | ||
| common.skip('missing crypto'); | ||
|
|
||
| const assert = require('assert'); | ||
| const http2 = require('http2'); | ||
| const { once } = require('events'); | ||
|
|
||
| const { NGHTTP2_ENHANCE_YOUR_CALM } = http2.constants; | ||
|
|
||
| // maxSessionMemory is documented to reject new streams while the limit is | ||
| // exceeded. Make sure that also holds for requests initiated by a client | ||
| // session, so that a peer that does not read cannot make the client queue | ||
| // frames without bound. As for response headers, request bodies that are | ||
| // queued waiting for flow control are not counted, so a large upload does not | ||
| // prevent unrelated requests on the same session. | ||
|
|
||
| async function queuedUploadDoesNotBlockRequests() { | ||
| // The server never opens its flow-control window, so the upload stays queued | ||
| // in the client session. | ||
| const server = http2.createServer({ settings: { initialWindowSize: 0 } }); | ||
| server.on('stream', common.mustCall((stream) => { | ||
| stream.on('error', () => {}); | ||
| }, 2)); | ||
| server.listen(0); | ||
| await once(server, 'listening'); | ||
|
|
||
| const client = http2.connect(`http://localhost:${server.address().port}`, { | ||
| maxSessionMemory: 1, | ||
| }); | ||
| client.on('error', common.mustNotCall()); | ||
| await once(client, 'remoteSettings'); | ||
|
|
||
| const uploadArrived = once(server, 'stream'); | ||
| const upload = client.request({ ':method': 'POST', ':path': '/upload' }); | ||
| upload.on('error', () => {}); | ||
| upload.end(Buffer.alloc(4 * 1024 * 1024)); | ||
| await uploadArrived; | ||
|
|
||
| const other = client.request({ ':path': '/other' }); | ||
| other.on('error', () => {}); | ||
| await once(server, 'stream'); | ||
|
|
||
| other.destroy(); | ||
| upload.destroy(); | ||
| client.destroy(); | ||
| server.close(); | ||
| } | ||
|
|
||
| async function manyRequestsAreBounded() { | ||
| // 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)); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| server.on('stream', (stream) => stream.on('error', () => {})); | ||
| server.listen(0); | ||
| await once(server, 'listening'); | ||
|
|
||
| const client = http2.connect(`http://localhost:${server.address().port}`, { | ||
| maxSessionMemory: 1, | ||
| }); | ||
| client.on('error', () => {}); | ||
| await once(client, 'connect'); | ||
|
|
||
| const total = 30_000; | ||
| let rejected = 0; | ||
| for (let i = 0; i < total; i++) { | ||
| const req = client.request({ ':path': `/${i}` }); | ||
| req.on('error', (err) => { | ||
| if (err.code === 'ERR_HTTP2_STREAM_ERROR' && | ||
| err.message.includes('NGHTTP2_ENHANCE_YOUR_CALM')) { | ||
| rejected++; | ||
| } | ||
| }); | ||
| req.end(); | ||
| } | ||
| await new Promise(setImmediate); | ||
|
|
||
| assert.ok(rejected > 0, 'expected some requests to be rejected'); | ||
| // Without the check all of the requests (plus the connection preface) were | ||
| // queued: the number of queued frames grew with the number submitted. | ||
| assert.ok(client.state.outboundQueueSize < total, | ||
| `queued ${client.state.outboundQueueSize} of ${total}`); | ||
| assert.strictEqual(NGHTTP2_ENHANCE_YOUR_CALM, 11); | ||
|
|
||
| client.destroy(); | ||
| server.close(); | ||
| } | ||
|
|
||
| (async () => { | ||
| await queuedUploadDoesNotBlockRequests(); | ||
| await manyRequestsAreBounded(); | ||
| })().then(common.mustCall()); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message says
NGHTTP2_ENHANCE_YOUR_CALM, butstream.rstCodeends 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?