http2: handle 100-continue flow & writeContinue #15039

apapirovski · 2017-08-26T03:00:38Z

This started out as work on tests and progressed into tackling 100-continue in http2. Not sure if it's even on the right track... still finding my way around the codebase. Hopefully not stepping on any toes here.

This adds an implementation for writeContinue based on the h2 spec & the existing http implementation. ClientHttp2Stream now emits a continue event when it receives headers with :status 100.

The current writeContinue implementation doesn't allow a callback and I'm not really sure how to tackle it given the notes in core about additionalHeaders & nghttp2.
Not sure if handleHeaderContinue is in the right spot or if I need to have any other safety checks in there. It seemed like onSessionHeaders already handles most of the heavy work in that regard.
The test case is adapted from the one for http and should be thorough. That's the one bit that should be good to go.

Thanks in advance!

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

http2, test

apapirovski · 2017-08-26T17:22:47Z

Rewrote the original test case and also added a test case for the default expected behaviour without a user-bound checkContinue listener. Feedback appreciated.

jasnell · 2017-08-29T04:28:37Z

Thank you for this! I'll take a look in detail tomorrow.

mcollina

Thanks for contributing! You are definitely on the right track! I've left some notes to be addressed, let us know if you have any questions.

mcollina · 2017-08-29T07:59:52Z

lib/internal/http2/compat.js

-    // TODO mcollina check what is the continue flow
-    throw new Error('not implemented yet');
+    const stream = this[kStream];
+    if (stream === undefined) return;


I don't think we should return false here, see https://github.com/nodejs/node/blob/master/lib/_http_outgoing.js#L252-L253.

Sorry, did you mean we should return false to match the old behaviour? Wanted to confirm before I go doing the wrong thing, haha.

mcollina · 2017-08-29T08:00:14Z

lib/internal/http2/compat.js

+    if (stream === undefined) return;
+    this[kStream].additionalHeaders({
+      [constants.HTTP2_HEADER_STATUS]: constants.HTTP_STATUS_CONTINUE
+    });


this needs to have a return type, ideally true in this case.

mcollina · 2017-08-29T08:00:44Z

test/parallel/test-http2-compat-expect-continue-check.js

+const testResBody = 'other stuff!\n';
+const testResHeaderKey = 'abcd';
+const testResHeaderVal = '1';
+


can you add a quick description of the test in comments here?

mcollina · 2017-08-29T08:01:30Z

test/parallel/test-http2-compat-expect-continue-check.js

+
+  res.writeContinue();
+
+  setTimeout(common.mustCall(() => handler(req, res)), 100);


why the setTimeout here? If it's needed to defer for the I/O cycle, can you add a comment about it?

@jasnell do we have a better way to do this? How could we know it has finished writing it?

Will add a comment. Not sure there's a better way to allow 'continue' to fire on the client before we continue writing.

mcollina · 2017-08-29T08:01:49Z

test/parallel/test-http2-compat-expect-continue-check.js

+
+  res.writeHead(200, {
+    'content-type': 'text/plain',
+    [testResHeaderKey]: [testResHeaderVal]


I don't think we should have these set dynamically.

mcollina · 2017-08-29T08:02:06Z

test/parallel/test-http2-compat-expect-continue.js

+const http2 = require('http2');
+
+const testResBody = 'other stuff!\n';
+


can you add a description of the test?

mcollina · 2017-08-29T08:06:52Z

test/parallel/test-http2-compat-expect-continue-check.js

+  console.error('Client sent request');
+
+  req.on('continue', common.mustCall(() => {
+    console.error('Client received 100-continue');


we need to verify that this event happens before the response

Ah, yeah. Good point. Lost that when I refactored these tests. Will fix.

mcollina · 2017-08-29T08:11:20Z

test/parallel/test-http2-compat-expect-continue.js

+    assert.strictEqual(headers[':status'], 200);
+  }));
+
+  req.on('data', common.mustCall((chunk) => { body += chunk; }));


you will need setEncoding('utf8') to concatenate the body like this. chunk should be a Buffer here.

Great point, totally missed it! Thanks.

mcollina · 2017-08-29T08:12:02Z

lib/internal/http2/compat.js

@@ -529,9 +529,13 @@ class Http2ServerResponse extends Stream {
    this.emit('finish');
  }

+  // TODO doesn't support callbacks


IMHO callbacks are not needed here, they are not there in the HTTP1 API.

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;

};

node/lib/_http_outgoing.js

Line 247 in 689a643

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.

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

apapirovski · 2017-08-29T10:40:52Z

Thanks @mcollina! Will update today. I'll reply to some points individually above.

apapirovski · 2017-08-29T11:30:04Z

Ok, I was able to tackle everything but the callback situation with writeContinue. See my comment above. #15039 (comment)

mcollina

Would you mind checking if the docs needs updating as well? I think so.

The code is ok for me, minus the callback thing (which we might want to do in another PR).

mcollina · 2017-08-29T11:55:29Z

test/parallel/test-http2-compat-expect-continue-check.js

+  res.writeContinue();
+
+  // timeout so that we allow the client to receive continue first
+  setTimeout(common.mustCall(() => handler(req, res)), 100);


If we are keeping the timeout, we should multiply 100 with common.platformTimeout, otherwise it might fail on slower platforms (ARM).

apapirovski · 2017-08-29T11:57:23Z

Yep, the docs for writeContinue do need updating. I was leaving it for last. Will update today, including any other feedback (like the timeout).

For the callback, I'll keep the comment in there as a reminder, unless there are any objections.

Thanks for all the feedback!

apapirovski · 2017-08-29T12:15:24Z

Re: the docs, do we want to document checkContinue as well? If so, what's the usual logic for sorting events under Http2Server and Http2SecureServer? It's not really alphabetical or anything else right now.

Also, how do you handle the 'version added' note? Should I even worry about that? Thanks in advance.

mcollina · 2017-08-29T12:31:10Z

@apapirovski also add checkContinue were it fits more naturally. If you want to improve the docs, e.g. make that in alphabetical order, do it in another PR.

For the "version added" it should be:

<!-- YAML
added: REPLACEME
-->

apapirovski · 2017-08-29T19:08:55Z

Btw the documentation has now been added (and the timeout issue addressed). Let me know if there's anything I should update.

I'll squash into one commit with proper commit message once everything has been reviewed, otherwise it's a pain to track the changes.

mcollina · 2017-08-30T01:25:33Z

doc/api/http2.md

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


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.

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.

I prefer the first one.

Agreed. The first one

Agreed. Wanted to make sure it wasn't weird starting with the 'if'. 👍 Updated and committed.

mcollina · 2017-08-30T03:15:11Z

CI: https://ci.nodejs.org/job/node-test-pull-request/9875/

mcollina

LGTM

apapirovski · 2017-08-30T03:17:46Z

@mcollina would you like me to squash or do you want to just have whoever potentially merges do that? Not sure how you handle things usually. The first commit on this has the proper message format FWIW.

Thanks!

jasnell · 2017-08-30T03:19:11Z

Either way works but let's let the ci run complete before squashing. Doing it mid run can cause hiccups

Adds an implementation for writeContinue based on the h2 spec & the existing http implementation. ClientHttp2Stream now also emits a continue event when it receives headers with :status 100. Includes two test cases for default server continue behaviour and for the exposed checkContinue listener.

apapirovski · 2017-08-30T14:15:57Z

Squashed commit added. Let me know if there's anything else I can do! Thanks for the review @mcollina & @jasnell.

benjamingr · 2017-08-30T18:11:49Z

CI failure looks unrelated. @jasnell can we land this?

Adds an implementation for writeContinue based on the h2 spec & the existing http implementation. ClientHttp2Stream now also emits a continue event when it receives headers with :status 100. Includes two test cases for default server continue behaviour and for the exposed checkContinue listener. PR-URL: #15039 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell · 2017-08-30T19:08:16Z

Landed in ad3d2ce

Adds an implementation for writeContinue based on the h2 spec & the existing http implementation. ClientHttp2Stream now also emits a continue event when it receives headers with :status 100. Includes two test cases for default server continue behaviour and for the exposed checkContinue listener. PR-URL: nodejs#15039 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Adds an implementation for writeContinue based on the h2 spec & the existing http implementation. ClientHttp2Stream now also emits a continue event when it receives headers with :status 100. Includes two test cases for default server continue behaviour and for the exposed checkContinue listener. PR-URL: nodejs/node#15039 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Adds an implementation for writeContinue based on the h2 spec & the existing http implementation. ClientHttp2Stream now also emits a continue event when it receives headers with :status 100. Includes two test cases for default server continue behaviour and for the exposed checkContinue listener. PR-URL: #15039 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added the http2 Issues or PRs related to the http2 subsystem. label Aug 26, 2017

hiroppy added the wip Issues and PRs that are still a work in progress. label Aug 26, 2017

apapirovski force-pushed the patch-100-continue-flow branch from 8ddeae5 to ffa7ef8 Compare August 26, 2017 17:21

jasnell requested a review from mcollina August 29, 2017 04:28

jasnell self-assigned this Aug 29, 2017

mcollina reviewed Aug 29, 2017

View reviewed changes

apapirovski force-pushed the patch-100-continue-flow branch from f9835ec to cb97536 Compare August 29, 2017 11:27

mcollina reviewed Aug 29, 2017

View reviewed changes

apapirovski changed the title ~~WIP: http2: handle 100-continue flow & writeContinue~~ http2: handle 100-continue flow & writeContinue Aug 29, 2017

mcollina reviewed Aug 30, 2017

View reviewed changes

mcollina approved these changes Aug 30, 2017

View reviewed changes

apapirovski force-pushed the patch-100-continue-flow branch from 8c49c5a to bbb9f71 Compare August 30, 2017 14:12

jasnell approved these changes Aug 30, 2017

View reviewed changes

jasnell closed this Aug 30, 2017

apapirovski deleted the patch-100-continue-flow branch September 4, 2017 14:35

MylesBorins mentioned this pull request Sep 10, 2017

v8.5.0 proposal #15308

Merged

MylesBorins added the dont-land-on-v6.x label Sep 20, 2017


		res.writeContinue();

		setTimeout(common.mustCall(() => handler(req, res)), 100);

		const http2 = require('http2');

		const testResBody = 'other stuff!\n';

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

http2: handle 100-continue flow & writeContinue #15039

http2: handle 100-continue flow & writeContinue #15039

Conversation

apapirovski commented Aug 26, 2017 • edited Loading

Checklist

Affected core subsystem(s)

apapirovski commented Aug 26, 2017

jasnell commented Aug 29, 2017

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apapirovski commented Aug 29, 2017

apapirovski commented Aug 29, 2017

mcollina left a comment

Choose a reason for hiding this comment

mcollina Aug 29, 2017 • edited Loading

Choose a reason for hiding this comment

apapirovski commented Aug 29, 2017 • edited Loading

apapirovski commented Aug 29, 2017 • edited Loading

mcollina commented Aug 29, 2017

apapirovski commented Aug 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina commented Aug 30, 2017

mcollina left a comment

Choose a reason for hiding this comment

apapirovski commented Aug 30, 2017 • edited Loading

jasnell commented Aug 30, 2017

apapirovski commented Aug 30, 2017

benjamingr commented Aug 30, 2017 • edited Loading

jasnell commented Aug 30, 2017

apapirovski commented Aug 26, 2017 •

edited

Loading

mcollina Aug 29, 2017 •

edited

Loading

apapirovski commented Aug 29, 2017 •

edited

Loading

apapirovski commented Aug 29, 2017 •

edited

Loading

apapirovski commented Aug 29, 2017 •

edited

Loading

apapirovski commented Aug 30, 2017 •

edited

Loading

benjamingr commented Aug 30, 2017 •

edited

Loading