-
Notifications
You must be signed in to change notification settings - Fork 30.7k
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
http2: handle 100-continue flow & writeContinue #15039
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 | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -529,9 +529,14 @@ class Http2ServerResponse extends Stream { | |||||||||||
this.emit('finish'); | ||||||||||||
} | ||||||||||||
|
||||||||||||
// TODO doesn't support callbacks | ||||||||||||
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. IMHO callbacks are not needed here, they are not there in the HTTP1 API. 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. So the reason I added this is because the h1 API actually does support a callback, even though it's not documented. Lines 174 to 177 in 689a643
Line 247 in 689a643
That said, I don't really know the best way to handle this callback with h2 because the write happens on the next tick and there are no events or anything to notify me. 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. @jasnell do you see a way to add the callback to the C++ layer? For parity, we would need to add a callback to https://github.com/apapirovski/node/blob/cb9753692d4485249cee00e6c675a00e089f1f2e/lib/internal/http2/core.js#L2093. |
||||||||||||
writeContinue() { | ||||||||||||
// TODO mcollina check what is the continue flow | ||||||||||||
throw new Error('not implemented yet'); | ||||||||||||
const stream = this[kStream]; | ||||||||||||
if (stream === undefined) return false; | ||||||||||||
this[kStream].additionalHeaders({ | ||||||||||||
[constants.HTTP2_HEADER_STATUS]: constants.HTTP_STATUS_CONTINUE | ||||||||||||
}); | ||||||||||||
return true; | ||||||||||||
} | ||||||||||||
} | ||||||||||||
|
||||||||||||
|
@@ -556,7 +561,7 @@ function onServerStream(stream, headers, flags) { | |||||||||||
if (server.listenerCount('checkContinue')) { | ||||||||||||
server.emit('checkContinue', request, response); | ||||||||||||
} else { | ||||||||||||
response.sendContinue(); | ||||||||||||
response.writeContinue(); | ||||||||||||
server.emit('request', request, response); | ||||||||||||
} | ||||||||||||
} else if (server.listenerCount('checkExpectation')) { | ||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,81 @@ | ||
// Flags: --expose-http2 | ||
'use strict'; | ||
|
||
const common = require('../common'); | ||
if (!common.hasCrypto) | ||
common.skip('missing crypto'); | ||
const assert = require('assert'); | ||
const http2 = require('http2'); | ||
|
||
const testResBody = 'other stuff!\n'; | ||
|
||
// Checks the full 100-continue flow from client sending 'expect: 100-continue' | ||
// through server receiving it, triggering 'checkContinue' custom handler, | ||
// writing the rest of the request to finally the client receiving to. | ||
|
||
function handler(req, res) { | ||
console.error('Server sent full response'); | ||
|
||
res.writeHead(200, { | ||
'content-type': 'text/plain', | ||
'abcd': '1' | ||
}); | ||
res.end(testResBody); | ||
} | ||
|
||
const server = http2.createServer( | ||
common.mustNotCall('Full request received before 100 Continue') | ||
); | ||
|
||
server.on('checkContinue', common.mustCall((req, res) => { | ||
console.error('Server received Expect: 100-continue'); | ||
|
||
res.writeContinue(); | ||
|
||
// timeout so that we allow the client to receive continue first | ||
setTimeout( | ||
common.mustCall(() => handler(req, res)), | ||
common.platformTimeout(100) | ||
); | ||
})); | ||
|
||
server.listen(0); | ||
|
||
server.on('listening', common.mustCall(() => { | ||
let body = ''; | ||
|
||
const port = server.address().port; | ||
const client = http2.connect(`http://localhost:${port}`); | ||
const req = client.request({ | ||
':method': 'POST', | ||
':path': '/world', | ||
expect: '100-continue' | ||
}); | ||
console.error('Client sent request'); | ||
|
||
let gotContinue = false; | ||
req.on('continue', common.mustCall(() => { | ||
console.error('Client received 100-continue'); | ||
gotContinue = true; | ||
})); | ||
|
||
req.on('response', common.mustCall((headers) => { | ||
console.error('Client received response headers'); | ||
|
||
assert.strictEqual(gotContinue, true); | ||
assert.strictEqual(headers[':status'], 200); | ||
assert.strictEqual(headers['abcd'], '1'); | ||
})); | ||
|
||
req.setEncoding('utf-8'); | ||
req.on('data', common.mustCall((chunk) => { body += chunk; })); | ||
|
||
req.on('end', common.mustCall(() => { | ||
console.error('Client received full response'); | ||
|
||
assert.strictEqual(body, testResBody); | ||
|
||
client.destroy(); | ||
server.close(); | ||
})); | ||
})); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,66 @@ | ||
// Flags: --expose-http2 | ||
'use strict'; | ||
|
||
const common = require('../common'); | ||
if (!common.hasCrypto) | ||
common.skip('missing crypto'); | ||
const assert = require('assert'); | ||
const http2 = require('http2'); | ||
|
||
const testResBody = 'other stuff!\n'; | ||
|
||
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. can you add a description of the test? |
||
// Checks the full 100-continue flow from client sending 'expect: 100-continue' | ||
// through server receiving it, sending back :status 100, writing the rest of | ||
// the request to finally the client receiving to. | ||
|
||
const server = http2.createServer(); | ||
|
||
let sentResponse = false; | ||
|
||
server.on('request', common.mustCall((req, res) => { | ||
console.error('Server sent full response'); | ||
|
||
res.end(testResBody); | ||
sentResponse = true; | ||
})); | ||
|
||
server.listen(0); | ||
|
||
server.on('listening', common.mustCall(() => { | ||
let body = ''; | ||
|
||
const port = server.address().port; | ||
const client = http2.connect(`http://localhost:${port}`); | ||
const req = client.request({ | ||
':method': 'POST', | ||
':path': '/world', | ||
expect: '100-continue' | ||
}); | ||
console.error('Client sent request'); | ||
|
||
let gotContinue = false; | ||
req.on('continue', common.mustCall(() => { | ||
console.error('Client received 100-continue'); | ||
gotContinue = true; | ||
})); | ||
|
||
req.on('response', common.mustCall((headers) => { | ||
console.error('Client received response headers'); | ||
|
||
assert.strictEqual(gotContinue, true); | ||
assert.strictEqual(sentResponse, true); | ||
assert.strictEqual(headers[':status'], 200); | ||
})); | ||
|
||
req.setEncoding('utf8'); | ||
req.on('data', common.mustCall((chunk) => { body += chunk; })); | ||
|
||
req.on('end', common.mustCall(() => { | ||
console.error('Client received full response'); | ||
|
||
assert.strictEqual(body, testResBody); | ||
|
||
client.destroy(); | ||
server.close(); | ||
})); | ||
})); |
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.
We need to specify that this event will be emitted only if there is a
'request'
event handler, or a function is passed throughcreateServer
/createSecureServer
.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.
Any thoughts on which of these two options is more legible? Thanks!
If a
'request'
listener is registered or'http2.createServer()'
is supplied a callback function, the'checkContinue'
event is emitted each time a request with an HTTPExpect: 100-continue
is received. If this event is not listened for, the server will automatically respond with a status100 Continue
as appropriate.or
Emitted each time a request with an HTTP
Expect: 100-continue
is received. If this event is not listened for, the server will automatically respond with a status100 Continue
as appropriate. Note that this event will be not be emitted if no'request'
listener is registered and'http2.createServer()'
was not supplied a callback function.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.
I prefer the first one.
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.
Agreed. The first one
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.
Agreed. Wanted to make sure it wasn't weird starting with the 'if'. 👍 Updated and committed.