Support http2 #36

sogaani · 2018-08-02T15:50:17Z

Support http2 to enable express tests with http2.
expressjs/express#3390.

body-parser cannot parse http2 body stream, because hasBody with http2ServerRequest always return false. A http2ServerRequest does not have any information about body had. I think hasBody with http2ServerRequest should return always true.

dougwilson

There is definitely something wrong here. For example a HEAD request, even in HTTP2, will never have a Body, so hasBody should return false. Same with almost all GET requests which would never have a body.

sogaani · 2018-08-02T16:12:21Z

@dougwilson Thank you for feedback. you are right. I will change hasBody to return false if request is OPTIONS, TRACE, CONNECT, HEADor GET method.

dougwilson · 2018-08-02T16:13:33Z

In HTTP1 a GET request may or may not have a body. I assume this is still the same in http/2 as I have used a body in a GET request to a http/2 server. Also that logic doesn't seem robust for custom methods or new future methods. There should be somewhere in the http/2 rfc that states exactly how to know if a request includes a body or not. That should be the implemented logic here.

sogaani · 2018-08-02T16:22:19Z

I dig in http/2 rfc next week and implement this.

dougwilson · 2018-08-02T16:22:36Z

Skipping around through the rfc you linked,, from what I can gather is that a body = data frames from the client on a stream. So to know if there is a body in http/2 you would say "will this steam send data frames?" and it seems that answer would be yes for the following condition: you get a header or continuation frame with end headers flag but no end stream flag set.

sogaani · 2018-08-02T16:30:25Z

So to know if there is a body in http/2 you would say "will this steam send data frames?" and it seems that answer would be yes for the following condition: you get a header or continuation frame with end headers flag but no end stream flag set.

Definitely yes. And I had checked some nodejs code how get end stream flag. But I cannot found... At least, stream still not ended when express get http2ServerRequest even if no body. If you know that, it is great!

dougwilson · 2018-08-02T16:34:13Z

It's possible that new API needs to be added to Node.js to get that information. User land code should be able to get at all the data at the http/2 level from my understanding. If it's purposely hidden though the API then at the very least have an API to know if the http/2 request actually has a body or not.

sogaani · 2018-08-06T15:53:01Z

I dig in nodejs code and found parameter that indicate already get end stream flag.

When Nodejs get end stream flag with HeaderFrame, Nodejs push null in order to end a readableStream and set stream._readableStream.ended to true.
https://github.com/nodejs/node/blob/master/lib/internal/http2/core.js#L301

I fixed this PR.

dougwilson

Awesome! Just the note about testing.

dougwilson · 2018-08-06T16:11:03Z

test/test.js

+        'content-type': type || ''
+      },
+      stream: {
+        _readableState: {


At this point if we are depending on Node.js internals for the state, we need to test against the actual Node.js implementation instead of a simple mock.

Either that or there is specific documentation in Node.js documenting this exact behavior, though I didn't see that anywhere. Without one of these we won't know when Node.js changes it's behavior as our tests won't fail.

I will fix tests tomorrow.

dougwilson · 2018-08-07T14:29:56Z

test/test.js

-      assert.strictEqual(typeis(req, ['image/*']), false)
-      assert.strictEqual(typeis(req, ['text/*', 'image/*']), false)
-    })
+    if (!process.env.HTTP2_TEST) {


Why is this excludes from HTTP/2 tests? To me this seems like it should work in http/2.

I agree with you. I just cannot found how to unset content-type in superagent. I'm checking superagent code.

dougwilson · 2018-08-07T14:56:20Z

test/test.js

+      callback(req)
+    })
+
+    server = server.listen(function () {


Don't forget to close the server, otherwise every test leaves a new server open, eventually going to starve the test memory over time.

dougwilson · 2018-08-07T14:56:41Z

test/test.js

+      callback(req)
+    })
+
+    server = server.listen(function () {


Don't forget to close the server, otherwise every test leaves a new server open, eventually going to starve the test memory over time.

sogaani · 2018-08-09T10:40:59Z

@dougwilson I addressed review comment. I think this is good for now. 👍

dougwilson · 2018-08-09T11:53:44Z

.travis.yml

@@ -26,6 +26,7 @@ before_install:
  - "test $TRAVIS_NODE_VERSION != '0.6' || npm rm --save-dev istanbul"
  - "test $TRAVIS_NODE_VERSION != '0.8' || npm rm --save-dev istanbul"
  - "test $(echo $TRAVIS_NODE_VERSION | cut -d. -f1) -ge 4 || npm rm --save-dev $(grep -E '\"eslint\\S*\"' package.json | cut -d'\"' -f2)"
+  - "test -z $(echo $HTTP2_TEST) || npm install --only=dev https://github.com/visionmedia/superagent.git"


Please only install deps from npm.

Unfortunately, superagent with http2 have not released. I will wait for release.

Also what does the only flag do? According to https://docs.npmjs.com/cli/install it only does something if you do not provide a module name.

This is my mistake. --only=dev flag does nothing here. I should delete this flag.

dougwilson · 2018-08-09T12:13:16Z

index.js

 * @param {Object} request
 * @return {Boolean}
 * @public
 */

 function hasbody (req) {
-  return req.headers['transfer-encoding'] !== undefined ||
+  return (ishttp2(req) && !req.stream._readableState.ended) ||


Some testing seems like this may have an issue. If you add a req.on('data' listener and read the body, this then returns false for http/2 but true for http/1. The current interface it is expected to be true for both, as hasBody determines if the request has body, regardless of it you happened to have read it already or not.

I checked and happens an issue you said. I address this issue and add a test.

dougwilson · 2018-08-09T14:47:03Z

index.js

 * @param {Object} request
 * @return {Boolean}
 * @public
 */

 function hasbody (req) {
-  return req.headers['transfer-encoding'] !== undefined ||
+  return (ishttp2(req) && (req.stream.readable || req.stream._readableState.endEmitted)) ||


It seems like this still has some kind of issue. I would expect the following test to pass:

it('should not indicate body', function (done) { createBodylessRequest('', function (req) { var data = '' req.on('data', function (chunk) { data += chunk }) req.on('end', function (chunk) { process.nextTick(function () { assert.strictEqual(typeis.hasBody(req), false) done() }) }) }) })

It seems like just attaching a data event will make this module claim the request has a body, even if the original request did not have a body.

I addressed issue. I found a parameter _readableState.sync which is default true and became false after reading data.

Nice! What does that flag actually mean / is it documented so we can rely on some kind of behavior? The underscore prefix just has me worries about the long term.

It means 'readable'/'data' event is emitted immediately or not.

See also comment in source.
https://github.com/nodejs/node/blob/master/lib/_stream_readable.js#L104
// a flag to be able to tell if the event 'readable'/'data' is emitted
// immediately, or on a later tick. We set this to true at first, because
// any actions that shouldn't happen until "later" should generally also
// not happen before the first read call.

I don't think Node.js considers docs in source code as promised behavior.

I'm also unclear on how It means 'readable'/'data' event is emitted immediately or not. has any indication on if the http/2 stream has data frames or not? It seems to be unrelated to the unlying state of the http/2 layer to me.

Searching the Node.js issue tracker for the underscore property pulls up nodejs/node#445 which sounds like it shouldn't be used inside Node.js code, let alone third-party things.

I don't think Node.js considers docs in source code as promised behavior.

Yes. However, current Nodejs has no other parameter to indicate hasBody after event 'end'. Should we suggest new API for Nodejs?

I'm also unclear on how It means 'readable'/'data' event is emitted immediately or not. has any indication on if the http/2 stream has data frames or not? It seems to be unrelated to the unlying state of the http/2 layer to me.

sync relates to readableStream rather than http/2 layer state. When http/2 layer get DataFrame, readableStream(ServerHttp2Request) emit 'data' event immediately or not.

Yes. However, current Nodejs has no other parameter to indicate hasBody after event 'end'. Should we suggest new API for Nodejs?

Based on the discussion in nodejs/node#445 it sounds like the answer is it's a must, as the activity seems to indicate that this will break sooner rather than later, and they want to get everything public.

OK, I will file a issue in nodejs repo and pending this PR.

dougwilson · 2018-09-13T13:34:04Z

OK, I will file a issue in nodejs repo and pending this PR.

Did you ever make this issue / can you provide a link I can look at?

sogaani · 2018-09-13T13:36:33Z

Yes, I already make the issue.
See also nodejs/node#22497

dougwilson · 2018-09-13T13:49:29Z

Thanks! I left a comment there. Let's give 30 days from now on that issue if it's not resolved and then we'll go with the reaching into internals solution of this PR 👍

sogaani · 2018-09-21T15:08:43Z

I fix PR to use request.stream.endAfterHeaders released on node 10.11.

sogaani added 2 commits August 3, 2018 00:40

http2 support

3a18e17

Add http2 tests

61e5115

dougwilson suggested changes Aug 2, 2018

View reviewed changes

jshttp deleted a comment from coveralls Aug 2, 2018

sogaani added 2 commits August 7, 2018 00:46

Check body by if readableStream ended

e8e8cbc

Lint

c2806ac

dougwilson suggested changes Aug 6, 2018

View reviewed changes

sogaani added 3 commits August 7, 2018 23:09

Don't use mock in http2 test

e0f0cc6

Fix travis tests

70d00de

Fix travis tests again

e6da059

dougwilson reviewed Aug 7, 2018

View reviewed changes

dougwilson added enhancement pr labels Aug 7, 2018

sogaani added 3 commits August 7, 2018 23:47

Enable no content type test

765a024

Fix travis tests again

09e012d

Fix travis tests again

9bb739f

dougwilson reviewed Aug 7, 2018

View reviewed changes

Close server at test ended

0108d17

dougwilson suggested changes Aug 9, 2018

View reviewed changes

sogaani added 2 commits August 9, 2018 21:59

hasBody with http2 request should returns true after 'end' event occured

395e271

Lint

9efda35

dougwilson suggested changes Aug 9, 2018

View reviewed changes

Fix hasBody with http2 request while or after 'end' event

97c0d7a

sogaani force-pushed the http2 branch 2 times, most recently from 4ed52f8 to 88c2633 Compare September 21, 2018 15:00

use endAfterHeaders

e5b45d4

sogaani force-pushed the http2 branch from 88c2633 to e5b45d4 Compare September 21, 2018 15:06

lint

a18f5a6

dougwilson force-pushed the master branch from 6d674ad to caf728b Compare February 21, 2022 22:27

Support http2 #36

Are you sure you want to change the base?

Support http2 #36

Conversation

sogaani commented Aug 2, 2018

dougwilson left a comment

Choose a reason for hiding this comment

sogaani commented Aug 2, 2018

dougwilson commented Aug 2, 2018

sogaani commented Aug 2, 2018

dougwilson commented Aug 2, 2018

sogaani commented Aug 2, 2018

dougwilson commented Aug 2, 2018

sogaani commented Aug 6, 2018

dougwilson 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

sogaani commented Aug 9, 2018

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

dougwilson commented Sep 13, 2018

sogaani commented Sep 13, 2018

dougwilson commented Sep 13, 2018

sogaani commented Sep 21, 2018