-
-
Notifications
You must be signed in to change notification settings - Fork 31.9k
http2: add session tracking and graceful server shutdown of http2 server #57586
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
Changes from all commits
bb71a11
7a852db
6bc96f1
dd751ae
be55b6d
8ee2e3e
90acf08
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 | ||||||
---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,67 @@ | ||||||||
'use strict'; | ||||||||
|
||||||||
const common = require('../common'); | ||||||||
if (!common.hasCrypto) | ||||||||
common.skip('missing crypto'); | ||||||||
const assert = require('assert'); | ||||||||
const http2 = require('http2'); | ||||||||
|
||||||||
// This test verifies that the server waits for active connections/sessions | ||||||||
// to complete and all data to be sent before fully closing | ||||||||
|
||||||||
// Create a server that will send a large response in chunks | ||||||||
const server = http2.createServer(); | ||||||||
|
||||||||
// Track server events | ||||||||
server.on('stream', common.mustCall((stream, headers) => { | ||||||||
|
||||||||
// Initiate the server close before client data is sent, this will | ||||||||
// test if the server properly waits for the stream to finish | ||||||||
server.close(); | ||||||||
setImmediate(() => { | ||||||||
stream.respond({ | ||||||||
'content-type': 'text/plain', | ||||||||
':status': 200 | ||||||||
}); | ||||||||
|
||||||||
// Create a large response (1MB) to ensure it takes some time to send | ||||||||
const chunk = Buffer.alloc(64 * 1024, 'x'); | ||||||||
|
||||||||
// Send 16 chunks (1MB total) to simulate a large response | ||||||||
for (let i = 0; i < 16; i++) { | ||||||||
stream.write(chunk); | ||||||||
} | ||||||||
|
||||||||
// Stream end should happen after data is written | ||||||||
stream.end(); | ||||||||
}); | ||||||||
|
||||||||
stream.on('close', common.mustCall(() => { | ||||||||
assert.strictEqual(stream.readableEnded, true); | ||||||||
assert.strictEqual(stream.writableFinished, true); | ||||||||
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.
Suggested change
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. just found from nodejs docs that readable do not have 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. Yes, it was a typo. |
||||||||
})); | ||||||||
})); | ||||||||
|
||||||||
// Start the server | ||||||||
server.listen(0, common.mustCall(() => { | ||||||||
// Create client and request | ||||||||
const client = http2.connect(`http://localhost:${server.address().port}`); | ||||||||
const req = client.request({ ':path': '/' }); | ||||||||
|
||||||||
// Track received data | ||||||||
let receivedData = 0; | ||||||||
req.on('data', (chunk) => { | ||||||||
receivedData += chunk.length; | ||||||||
}); | ||||||||
|
||||||||
// When request closes | ||||||||
req.on('close', common.mustCall(() => { | ||||||||
// Should receive all data | ||||||||
assert.strictEqual(req.readableEnded, true); | ||||||||
assert.strictEqual(receivedData, 64 * 1024 * 16); | ||||||||
assert.strictEqual(req.writableFinished, true); | ||||||||
})); | ||||||||
|
||||||||
// Start the request | ||||||||
req.end(); | ||||||||
})); |
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,58 @@ | ||||||||
'use strict'; | ||||||||
|
||||||||
const common = require('../common'); | ||||||||
if (!common.hasCrypto) | ||||||||
common.skip('missing crypto'); | ||||||||
const assert = require('assert'); | ||||||||
const http2 = require('http2'); | ||||||||
|
||||||||
// This test verifies that the server closes idle connections | ||||||||
|
||||||||
const server = http2.createServer(); | ||||||||
|
||||||||
server.on('session', common.mustCall((session) => { | ||||||||
session.on('stream', common.mustCall((stream) => { | ||||||||
// Respond to the request with a small payload | ||||||||
stream.respond({ | ||||||||
'content-type': 'text/plain', | ||||||||
':status': 200 | ||||||||
}); | ||||||||
stream.end('hello'); | ||||||||
stream.on('close', common.mustCall(() => { | ||||||||
assert.strictEqual(stream.writableFinished, true); | ||||||||
})); | ||||||||
Comment on lines
+21
to
+23
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.
Suggested change
Feel free to keep it, but I don't think it is useful as the server is closed after the request ends (the 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 have a but different opinion, I would like to keep it 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. we should ensure that server side stream have closed before server close 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 server does not close if there are open streams, no? 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. IMO if there are open stream we can’t call it graceful closure 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. Yes, that's the point, if the stream is not closed via 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. Do you think we should still remove that? |
||||||||
})); | ||||||||
session.on('close', common.mustCall()); | ||||||||
})); | ||||||||
|
||||||||
// Start the server | ||||||||
server.listen(0, common.mustCall(() => { | ||||||||
// Create client and initial request | ||||||||
const client = http2.connect(`http://localhost:${server.address().port}`); | ||||||||
|
||||||||
// This will ensure that server closed the idle connection | ||||||||
client.on('close', common.mustCall()); | ||||||||
|
||||||||
// Make an initial request | ||||||||
const req = client.request({ ':path': '/' }); | ||||||||
|
||||||||
// Process the response data | ||||||||
req.setEncoding('utf8'); | ||||||||
let data = ''; | ||||||||
req.on('data', (chunk) => { | ||||||||
data += chunk; | ||||||||
}); | ||||||||
|
||||||||
// When initial request ends | ||||||||
req.on('end', common.mustCall(() => { | ||||||||
assert.strictEqual(data, 'hello'); | ||||||||
// Close the server as client is idle now | ||||||||
setImmediate(() => { | ||||||||
server.close(); | ||||||||
}); | ||||||||
})); | ||||||||
|
||||||||
// Don't explicitly close the client, we want to keep it | ||||||||
// idle and check if the server closes the idle connection. | ||||||||
// As this is what we want to test here. | ||||||||
})); |
Uh oh!
There was an error while loading. Please reload this page.