Skip to content
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

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 60 additions & 2 deletions doc/api/http2.md
Original file line number Diff line number Diff line change
Expand Up @@ -849,6 +849,15 @@ used exclusively on HTTP/2 Clients. `Http2Stream` instances on the client
provide events such as `'response'` and `'push'` that are only relevant on
the client.

#### Event: 'continue'
<!-- YAML
added: REPLACEME
-->

Emitted when the server sends a `100 Continue` status, usually because
the request contained `Expect: 100-continue`. This is an instruction that
the client should send the request body.

#### Event: 'headers'
<!-- YAML
added: v8.4.0
Expand Down Expand Up @@ -1306,6 +1315,28 @@ added: v8.4.0
The `'timeout'` event is emitted when there is no activity on the Server for
a given number of milliseconds set using `http2server.setTimeout()`.

#### Event: 'checkContinue'
<!-- YAML
added: REPLACEME
-->

* `request` {http2.Http2ServerRequest}
* `response` {http2.Http2ServerResponse}

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 HTTP `Expect: 100-continue` is received. If this event is
not listened for, the server will automatically respond with a status
`100 Continue` as appropriate.

Handling this event involves calling [`response.writeContinue()`][] if the client
should continue to send the request body, or generating an appropriate HTTP
response (e.g. 400 Bad Request) if the client should not continue to send the
request body.

Note that when this event is emitted and handled, the [`'request'`][] event will
not be emitted.

### Class: Http2SecureServer
<!-- YAML
added: v8.4.0
Expand Down Expand Up @@ -1389,6 +1420,28 @@ per session. See the [Compatibility API](compatiblity-api).
added: v8.4.0
-->

#### Event: 'checkContinue'
<!-- YAML
added: REPLACEME
-->

* `request` {http2.Http2ServerRequest}
* `response` {http2.Http2ServerResponse}

If a [`'request'`][] listener is registered or [`'http2.createSecureServer()'`][]
is supplied a callback function, the `'checkContinue'` event is 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 status
`100 Continue` as appropriate.

Handling this event involves calling [`response.writeContinue()`][] if the client
should continue to send the request body, or generating an appropriate HTTP
response (e.g. 400 Bad Request) if the client should not continue to send the
request body.

Note that when this event is emitted and handled, the [`'request'`][] event will
not be emitted.
Copy link
Member

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 through  createServer / createSecureServer.

Copy link
Contributor Author

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 HTTP Expect: 100-continue is received. If this event is not listened for, the server will automatically respond with a status 100 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 status 100 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.

Copy link
Member

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. The first one

Copy link
Contributor Author

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.


### http2.createServer(options[, onRequestHandler])
<!-- YAML
added: v8.4.0
Expand Down Expand Up @@ -2537,8 +2590,9 @@ buffer. Returns `false` if all or part of the data was queued in user memory.
added: v8.4.0
-->

Throws an error as the `'continue'` flow is not current implemented. Added for
parity with [HTTP/1]().
Sends a status `100 Continue` to the client, indicating that the request body
should be sent. See the [`'checkContinue'`][] event on `Http2Server` and
`Http2SecureServer`.

### response.writeHead(statusCode[, statusMessage][, headers])
<!-- YAML
Expand Down Expand Up @@ -2618,6 +2672,7 @@ if the stream is closed.
[Settings Object]: #http2_settings_object
[Using options.selectPadding]: #http2_using_options_selectpadding
[Writable Stream]: stream.html#stream_writable_streams
[`'checkContinue'`]: #http2_event_checkcontinue
[`'request'`]: #http2_event_request
[`'unknownProtocol'`]: #http2_event_unknownprotocol
[`ClientHttp2Stream`]: #http2_class_clienthttp2stream
Expand All @@ -2628,14 +2683,17 @@ if the stream is closed.
[`ServerRequest`]: #http2_class_server_request
[`TypeError`]: errors.html#errors_class_typeerror
[`http2.SecureServer`]: #http2_class_http2secureserver
[`http2.createSecureServer()`]: #http2_createsecureserver_options_onrequesthandler
[`http2.Server`]: #http2_class_http2server
[`http2.createServer()`]: #http2_createserver_options_onrequesthandler
[`net.Socket`]: net.html#net_class_net_socket
[`request.socket.getPeerCertificate()`]: tls.html#tls_tlssocket_getpeercertificate_detailed
[`response.end()`]: #http2_response_end_data_encoding_callback
[`response.setHeader()`]: #http2_response_setheader_name_value
[`response.socket`]: #http2_response_socket
[`response.write()`]: #http2_response_write_chunk_encoding_callback
[`response.write(data, encoding)`]: http.html#http_response_write_chunk_encoding_callback
[`response.writeContinue()`]: #http2_response_writecontinue
[`response.writeHead()`]: #http2_response_writehead_statuscode_statusmessage_headers
[`stream.pushStream()`]: #http2_stream-pushstream
[`tls.TLSSocket`]: tls.html#tls_class_tls_tlssocket
Expand Down
11 changes: 8 additions & 3 deletions lib/internal/http2/compat.js
Original file line number Diff line number Diff line change
Expand Up @@ -529,9 +529,14 @@ class Http2ServerResponse extends Stream {
this.emit('finish');
}

// TODO doesn't support callbacks
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

node/lib/_http_server.js

Lines 174 to 177 in 689a643

ServerResponse.prototype.writeContinue = function writeContinue(cb) {
this._writeRaw('HTTP/1.1 100 Continue' + CRLF + CRLF, 'ascii', cb);
this._sent100 = true;
};

function _writeRaw(data, encoding, callback) {

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.

Copy link
Member

Choose a reason for hiding this comment

The 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;
}
}

Expand All @@ -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')) {
Expand Down
8 changes: 8 additions & 0 deletions lib/internal/http2/core.js
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ const {
HTTP2_METHOD_HEAD,
HTTP2_METHOD_CONNECT,

HTTP_STATUS_CONTINUE,
HTTP_STATUS_CONTENT_RESET,
HTTP_STATUS_OK,
HTTP_STATUS_NO_CONTENT,
Expand Down Expand Up @@ -2113,10 +2114,17 @@ class ClientHttp2Stream extends Http2Stream {
this[kState].headersSent = true;
if (id !== undefined)
this[kInit](id);
this.on('headers', handleHeaderContinue);
debug(`[${sessionName(session[kType])}] clienthttp2stream created`);
}
}

function handleHeaderContinue(headers) {
if (headers[HTTP2_HEADER_STATUS] === HTTP_STATUS_CONTINUE) {
this.emit('continue');
}
}

const setTimeout = {
configurable: true,
enumerable: true,
Expand Down
81 changes: 81 additions & 0 deletions test/parallel/test-http2-compat-expect-continue-check.js
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();
}));
}));
66 changes: 66 additions & 0 deletions test/parallel/test-http2-compat-expect-continue.js
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';

Copy link
Member

Choose a reason for hiding this comment

The 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();
}));
}));