Skip to content
This repository has been archived by the owner on Jul 6, 2018. It is now read-only.

http2: edge case errors, consistency, nits #167

Closed
wants to merge 7 commits into from

Conversation

sebdeckers
Copy link
Contributor

Discovered these while working with everyone's new favourite API, res.respondWithFile. 😉

  • Cleaned up a stray console.log in test-http2-respond-file.js. /cc @jasnell
  • Made the compat res.end(cb) callback behave the same as http1 (only fires the first time). Also no errors thrown on repeated res.end() calls.
  • Fixed errors when calling res.stream.respondWithFile from the compatibility layer. Seems like a valid use-case (well, I'm using it 🤣).
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

http2

@sebdeckers
Copy link
Contributor Author

Pushed a few more changes.

  • Achieves the same result without touching stream._readableState.
  • Additional test case for HEAD requests at the compat response layer (was breaking with the earlier patch).
  • Expose the internal flag state.headersSent as ServerHttp2Stream.headersSent to signal whether or not the .respond() methods can still be called. Earlier this was tracked separately by Http2ServerResponse.headersSent but that went out of sync now that HEAD requests and FD's are handled directly by the stream.
  • Updated the var declarations to const and let where appropriate. Sorry about the lines changed noise.

@sebdeckers
Copy link
Contributor Author

Added:

  • http2.connect() documentation shows that options and listener are optional. Added test cases for this.
  • Log output consistency fix to put the session type prefix in square brackets.

jasnell

This comment was marked as off-topic.

jasnell

This comment was marked as off-topic.

@sebdeckers sebdeckers force-pushed the compat-respond-with-file branch from 1eec6da to 64376eb Compare July 10, 2017 23:57
@sebdeckers
Copy link
Contributor Author

@jasnell De-linted. Welcome back! 👋

@jasnell jasnell mentioned this pull request Jul 12, 2017
4 tasks
jasnell pushed a commit that referenced this pull request Jul 13, 2017
* edge case errors, consistency, nits
* ServerHttp2Stream.headersSent property, proxied by Http2ServerResponse
  This avoids brittle abstraction-leaking to guess whether or not
  ServerHttp2Stream.respond() may still be called on HEAD requests
  where the stream is ended before headers are sent.
* docs for ServerHttp2Stream headersSent & pushAllowed (Squash)
* var to const/let
* consistent log message prefix
* doc fix & test for http2.connect() optional arguments
* eslint object-curly-spacing: ["error", "always"] (Squash)

PR-URL: #167
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Jul 13, 2017

Landed!

@jasnell jasnell closed this Jul 13, 2017
jasnell pushed a commit to jasnell/http2-1 that referenced this pull request Jul 14, 2017
* edge case errors, consistency, nits
* ServerHttp2Stream.headersSent property, proxied by Http2ServerResponse
  This avoids brittle abstraction-leaking to guess whether or not
  ServerHttp2Stream.respond() may still be called on HEAD requests
  where the stream is ended before headers are sent.
* docs for ServerHttp2Stream headersSent & pushAllowed (Squash)
* var to const/let
* consistent log message prefix
* doc fix & test for http2.connect() optional arguments
* eslint object-curly-spacing: ["error", "always"] (Squash)

PR-URL: nodejs#167
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants