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.

Sign up for GitHub

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

Jump to bottom

Support http2 #36

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

Support http2 #36

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
3a18e17
http2 support
sogaani Aug 2, 2018
61e5115
Add http2 tests
sogaani Aug 2, 2018
e8e8cbc
Check body by if readableStream ended
sogaani Aug 6, 2018
c2806ac
Lint
sogaani Aug 6, 2018
e0f0cc6
Don't use mock in http2 test
sogaani Aug 7, 2018
70d00de
Fix travis tests
sogaani Aug 7, 2018
e6da059
Fix travis tests again
sogaani Aug 7, 2018
765a024
Enable no content type test
sogaani Aug 7, 2018
09e012d
Fix travis tests again
sogaani Aug 7, 2018
9bb739f
Fix travis tests again
sogaani Aug 7, 2018
0108d17
Close server at test ended
sogaani Aug 7, 2018
395e271
hasBody with http2 request should returns true after 'end' event occured
sogaani Aug 9, 2018
9efda35
Lint
sogaani Aug 9, 2018
97c0d7a
Fix hasBody with http2 request while or after 'end' event
sogaani Aug 9, 2018
e5b45d4
use endAfterHeaders
sogaani Sep 21, 2018
a18f5a6
lint
sogaani Sep 21, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 7 additions & 0 deletions .travis.yml
View file
Open in desktop
Expand Up @@ -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 https://github.com/visionmedia/superagent.git"

# Update Node.js modules
- "test ! -d node_modules || npm prune"
Expand All @@ -37,3 +38,9 @@ script:
- "test -z $(npm -ps ls eslint ) || npm run-script lint"
after_script:
- "test -e ./coverage/lcov.info && npm install coveralls@2 && cat ./coverage/lcov.info | coveralls"
matrix:
include:
- node_js: "9.5"
env: HTTP2_TEST=1
- node_js: "8.9"
env: HTTP2_TEST=1
22 changes: 21 additions & 1 deletion index.js
View file
Open in desktop
Expand Up @@ -84,16 +84,36 @@ function typeis (value, types_) {
* or `content-length` headers set.
* http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.3
*
* A http/2 request with DataFrame can have no `content-length` header.
* https://httpwg.org/specs/rfc7540.html
*
* A http/2 request without DataFrame send HeaderFrame with end-stream-flag.
* If nodejs gets end-stream-flag, then nodejs ends readable stream.
* https://github.com/nodejs/node/blob/master/lib/internal/http2/core.js#L301
*
* @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)) ||
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

req.headers['transfer-encoding'] !== undefined ||
!isNaN(req.headers['content-length'])
}

/**
* Check if a request is a http2 request.
*
* @param {Object} request
* @return {Boolean}
* @public
*/

function ishttp2 (req) {
return req.httpVersionMajor === 2
}

/**
* Check if the incoming request contains the "Content-Type"
* header field, and it contains any of the give mime `type`s.
Expand Down