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

[v8.x backport] backport 17338, 18186, 18358, 18546 #19191

Closed
wants to merge 32 commits into from
Closed

[v8.x backport] backport 17338, 18186, 18358, 18546 #19191

wants to merge 32 commits into from

Conversation

joyeecheung
Copy link
Member

src: expose uv.errmap to binding
Refs: #17338

util: implement util.getSystemErrorName()
Refs: #18186

errors: lazy load util in internal/errors.js
Refs: #18358

util: skip type checks in internal getSystemErrorName
errors: move error creation helpers to errors.js
Refs: #18546

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. v8.x labels Mar 7, 2018
@joyeecheung
Copy link
Member Author

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

@MylesBorins
Copy link
Contributor

one more ci: https://ci.nodejs.org/job/node-test-pull-request/13786/

@gibfahn gibfahn self-assigned this Mar 26, 2018
@BethGriggs
Copy link
Member

@joyeecheung please could you rebase?

@MylesBorins MylesBorins force-pushed the v8.x-staging branch 2 times, most recently from 44cb0d3 to 16bf5fe Compare March 30, 2018 03:28
@joyeecheung joyeecheung force-pushed the backport-18546-to-v8.x branch from 39fa6fa to 7763326 Compare March 30, 2018 17:52
@joyeecheung
Copy link
Member Author

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

@MylesBorins
Copy link
Contributor

@joyeecheung would this potentially cause #19716 on 8.x?

@joyeecheung
Copy link
Member Author

@MylesBorins Yes this needs to land with the fix

@MylesBorins MylesBorins added the blocked PRs that are blocked by other issues or PRs. label Apr 3, 2018
@MylesBorins
Copy link
Contributor

@joyeecheung I've added "blocked" so this isn't accidentally landed. Please feel free to remove label when this is ready to land

@MylesBorins
Copy link
Contributor

@joyeecheung is the fix ready to be included?

@joyeecheung
Copy link
Member Author

@MylesBorins I think so, this would just need to have 22da2f7 added

@MylesBorins
Copy link
Contributor

@joyeecheung would you be able to add the commit and rebase?

hekike and others added 13 commits May 2, 2018 14:30
@hekike @MylesBorins
http2: add http fallback options to .createServer
58478d8 
This adds the Http1IncomingMessage and Http1ServerReponse options
to http2.createServer().

Backport-PR-URL: nodejs#20456
PR-URL: nodejs#15752
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
@MylesBorins
http: add options to http.createServer()
5ea29ef 
This adds the optional options argument to `http.createServer()`.
It contains two options: the `IncomingMessage` and `ServerReponse`
option.

Backport-PR-URL: nodejs#20456
PR-URL: nodejs#15752
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
@MylesBorins
http2: add req and res options to server creation
ae2ebde 
Add optional Http2ServerRequest and Http2ServerResponse options
to createServer and createSecureServer. Allows custom req & res
classes that extend the default ones to be used without
overriding the prototype.

Backport-PR-URL: nodejs#20456
PR-URL: nodejs#15560
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@addaleax @MylesBorins
http2: use _final instead of on('finish')
28a2729 
Backport-PR-URL: nodejs#20456
PR-URL: nodejs#18609
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
@addaleax @MylesBorins
doc: warn against concurrent http2stream.respondWithFD
8b234f5 
Upcoming changes to move away from synchronous I/O on the main
thread will imply that using the same file descriptor to
respond on multiple HTTP/2 streams at the same time is invalid,
because at least on Windows `uv_fs_read()` is race-y.

Therefore, warn against such usage.

Backport-PR-URL: nodejs#20456
PR-URL: nodejs#18762
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@danbev @MylesBorins
src: add nullptr check for session in DEBUG macro
c6a9abd 
Currenlty when configuring --debug-http2
/test/parallel/test-http2-getpackedsettings.js will segment fault:

$ out/Debug/node test/parallel/test-http2-getpackedsettings.js
Segmentation fault: 11

This is happening because the settings is created with the Environment in
PackSettings:
Http2Session::Http2Settings settings(env);
This will cause the session to be set to nullptr. When the init
function is later called the expanded DEBUG_HTTP2SESSION macro will
cause the segment fault when the session is dereferenced.

Backport-PR-URL: nodejs#20456
PR-URL: nodejs#18815
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@vsemozhetbyt @MylesBorins
doc: fix typo in http2.md
d85e5f4 
Backport-PR-URL: nodejs#20456
PR-URL: nodejs#18872
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@addaleax @MylesBorins
deps,src: align ssize_t ABI between Node & nghttp2
55cf145 
Previously, we performed casts that are considered undefined behavior.
Instead, just define `ssize_t` for nghttp2 the same way we define it
for the rest of Node.

Also, remove a TODO comment that would probably also be *technically*
correct but shouldn’t matter as long as nobody is complaining.

Backport-PR-URL: nodejs#20456
PR-URL: nodejs#18565
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@mcollina @MylesBorins
http2: fix condition where data is lost
b1d95ec 
Backport-PR-URL: nodejs#20456
PR-URL: nodejs#18895
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@addaleax @MylesBorins
http2: send error text in case of ALPN mismatch
1029369 
Send a human-readable HTTP/1 response in case of an unexpected
ALPN protocol. This helps with debugging this condition,
since previously the only result of it would be a closed socket.

Backport-PR-URL: nodejs#20456
PR-URL: nodejs#18986
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@addaleax @MylesBorins
http2: use original error for cancelling pending streams
0379350 
Previously, if `session.destroy()` was called with an error object,
the information contained in it would be discarded and a generic
`ERR_HTTP2_STREAM_CANCEL` would be used for all pending streams.

Instead, make the information from the original error object
available.

Backport-PR-URL: nodejs#20456
PR-URL: nodejs#18988
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@addaleax @MylesBorins
http2: fix endless loop when writing empty string
1f3ab4f 
Backport-PR-URL: nodejs#20456
PR-URL: nodejs#18924
Fixes: nodejs#18169
Refs: nodejs#18673
Refs: https://github.com/nodejs/node/blob/v9.5.0/src/node_http2.cc#L1481-L1484
Refs: https://github.com/nodejs/node/blob/v9.5.0/lib/_http_outgoing.js#L659-L661
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@XadillaX @MylesBorins
test: check endless loop while writing empty string
199d9a5 
Refs: nodejs#18673
Backport-PR-URL: nodejs#20456
PR-URL: nodejs#18924
Refs: nodejs#18673
Refs: https://github.com/nodejs/node/blob/v9.5.0/src/node_http2.cc#L1481-L1484
Refs: https://github.com/nodejs/node/blob/v9.5.0/lib/_http_outgoing.js#L659-L661
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
joyeecheung added 4 commits May 3, 2018 06:38
@joyeecheung
util: implement util.getSystemErrorName()
d501138 
Reimplement uv.errname() as internal/util.getSystemErrorName() to
avoid the memory leaks caused by unknown error codes
and avoid calling into C++ for the error names. Also
expose it as a public API for external use.

PR-URL: nodejs#18186
Refs: http://docs.libuv.org/en/v1.x/errors.html#c.uv_err_name
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@joyeecheung
errors: lazy load util in internal/errors.js
a3754e7 
PR-URL: nodejs#18358
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
@joyeecheung
util: skip type checks in internal getSystemErrorName
2179547 
PR-URL: nodejs#18546
Reviewed-By: James M Snell <[email protected]>
@joyeecheung
errors: move error creation helpers to errors.js
d8b637f 
This commit moves error creation helpers scattered around
under lib/ into lib/internal/errors.js in the hope of being clearer
about the differences of errors that we throw into the user land.

- Move util._errnoException and util._exceptionWithHostPort
  into internal/errors.js and simplify their logic so it's
  clearer what the properties these helpers create.
- Move the errnoException helper in dns.js to internal/errors.js
  into internal/errors.js and rename it to dnsException. Simplify
  it's logic so it no longer calls errnoException and skips
  the unnecessary argument checks.

PR-URL: nodejs#18546
Reviewed-By: James M Snell <[email protected]>
@joyeecheung joyeecheung force-pushed the backport-18546-to-v8.x branch from 7763326 to d8b637f Compare May 2, 2018 23:23
@joyeecheung
Copy link
Member Author

joyeecheung commented May 2, 2018 •
edited
Loading

@MylesBorins Updated. CI: https://ci.nodejs.org/job/node-test-pull-request/14643/

@BridgeAR @joyeecheung
errors: make message non-enumerable
74e3b14 
A error message should always be non-enumerable. This makes sure
that is true for dns errors as well. It also adds another check
in `common.expectsError` to make sure no other regressions are
introduced going forward.

Fixes nodejs#19716

PR-URL: nodejs#19719
Fixes: nodejs#19716
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins force-pushed the v8.x-staging branch 3 times, most recently from 5ac4bac to 591812f Compare May 22, 2018 15:31
MylesBorins pushed a commit that referenced this pull request May 22, 2018
@joyeecheung @MylesBorins
src: expose uv.errmap to binding
3a7ad60 
Add a errno -> [error code, uv error message] map to the uv binding
so the error message can be assembled in the JS layer.

Backport-PR-URL: #19191
PR-URL: #17338
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 22, 2018
@joyeecheung @MylesBorins
util: implement util.getSystemErrorName()
4a55524 
Reimplement uv.errname() as internal/util.getSystemErrorName() to
avoid the memory leaks caused by unknown error codes
and avoid calling into C++ for the error names. Also
expose it as a public API for external use.

Backport-PR-URL: #19191
PR-URL: #18186
Refs: http://docs.libuv.org/en/v1.x/errors.html#c.uv_err_name
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 22, 2018
@joyeecheung @MylesBorins
errors: lazy load util in internal/errors.js
05076ac 
Backport-PR-URL: #19191
PR-URL: #18358
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 22, 2018
@joyeecheung @MylesBorins
util: skip type checks in internal getSystemErrorName
de86203 
Backport-PR-URL: #19191
PR-URL: #18546
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 22, 2018
@joyeecheung @MylesBorins
errors: move error creation helpers to errors.js
64f4b19 
This commit moves error creation helpers scattered around
under lib/ into lib/internal/errors.js in the hope of being clearer
about the differences of errors that we throw into the user land.

- Move util._errnoException and util._exceptionWithHostPort
  into internal/errors.js and simplify their logic so it's
  clearer what the properties these helpers create.
- Move the errnoException helper in dns.js to internal/errors.js
  into internal/errors.js and rename it to dnsException. Simplify
  it's logic so it no longer calls errnoException and skips
  the unnecessary argument checks.

Backport-PR-URL: #19191
PR-URL: #18546
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 22, 2018
@BridgeAR @MylesBorins
errors: make message non-enumerable
a974479 
A error message should always be non-enumerable. This makes sure
that is true for dns errors as well. It also adds another check
in `common.expectsError` to make sure no other regressions are
introduced going forward.

Fixes #19716

Backport-PR-URL: #19191
PR-URL: #19719
Fixes: #19716
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins
Copy link
Contributor

landed in 591812f...a974479

MylesBorins pushed a commit that referenced this pull request Jun 14, 2018
@joyeecheung @MylesBorins
src: expose uv.errmap to binding
788fb29 
Add a errno -> [error code, uv error message] map to the uv binding
so the error message can be assembled in the JS layer.

Backport-PR-URL: #19191
PR-URL: #17338
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jun 14, 2018
@joyeecheung @MylesBorins
util: implement util.getSystemErrorName()
844755a 
Reimplement uv.errname() as internal/util.getSystemErrorName() to
avoid the memory leaks caused by unknown error codes
and avoid calling into C++ for the error names. Also
expose it as a public API for external use.

Backport-PR-URL: #19191
PR-URL: #18186
Refs: http://docs.libuv.org/en/v1.x/errors.html#c.uv_err_name
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jun 14, 2018
@joyeecheung @MylesBorins
errors: lazy load util in internal/errors.js
ff693d2 
Backport-PR-URL: #19191
PR-URL: #18358
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jun 14, 2018
@joyeecheung @MylesBorins
util: skip type checks in internal getSystemErrorName
ae81a04 
Backport-PR-URL: #19191
PR-URL: #18546
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jun 14, 2018
@joyeecheung @MylesBorins
errors: move error creation helpers to errors.js
c0c5ac6 
This commit moves error creation helpers scattered around
under lib/ into lib/internal/errors.js in the hope of being clearer
about the differences of errors that we throw into the user land.

- Move util._errnoException and util._exceptionWithHostPort
  into internal/errors.js and simplify their logic so it's
  clearer what the properties these helpers create.
- Move the errnoException helper in dns.js to internal/errors.js
  into internal/errors.js and rename it to dnsException. Simplify
  it's logic so it no longer calls errnoException and skips
  the unnecessary argument checks.

Backport-PR-URL: #19191
PR-URL: #18546
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jun 14, 2018
@BridgeAR @MylesBorins
errors: make message non-enumerable
da34021 
A error message should always be non-enumerable. This makes sure
that is true for dns errors as well. It also adds another check
in `common.expectsError` to make sure no other regressions are
introduced going forward.

Fixes #19716

Backport-PR-URL: #19191
PR-URL: #19719
Fixes: #19716
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: James M Snell <[email protected]>
rvagg pushed a commit that referenced this pull request Aug 16, 2018
@joyeecheung @rvagg
src: expose uv.errmap to binding
c39b002 
Add a errno -> [error code, uv error message] map to the uv binding
so the error message can be assembled in the JS layer.

Backport-PR-URL: #19191
PR-URL: #17338
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
rvagg pushed a commit that referenced this pull request Aug 16, 2018
@joyeecheung @rvagg
util: implement util.getSystemErrorName()
58b5a61 
Reimplement uv.errname() as internal/util.getSystemErrorName() to
avoid the memory leaks caused by unknown error codes
and avoid calling into C++ for the error names. Also
expose it as a public API for external use.

Backport-PR-URL: #19191
PR-URL: #18186
Refs: http://docs.libuv.org/en/v1.x/errors.html#c.uv_err_name
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
rvagg pushed a commit that referenced this pull request Aug 16, 2018
@joyeecheung @rvagg
errors: lazy load util in internal/errors.js
9696bf9 
Backport-PR-URL: #19191
PR-URL: #18358
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
rvagg pushed a commit that referenced this pull request Aug 16, 2018
@joyeecheung @rvagg
util: skip type checks in internal getSystemErrorName
66520af 
Backport-PR-URL: #19191
PR-URL: #18546
Reviewed-By: James M Snell <[email protected]>
rvagg pushed a commit that referenced this pull request Aug 16, 2018
@joyeecheung @rvagg
errors: move error creation helpers to errors.js
9dc1f50 
This commit moves error creation helpers scattered around
under lib/ into lib/internal/errors.js in the hope of being clearer
about the differences of errors that we throw into the user land.

- Move util._errnoException and util._exceptionWithHostPort
  into internal/errors.js and simplify their logic so it's
  clearer what the properties these helpers create.
- Move the errnoException helper in dns.js to internal/errors.js
  into internal/errors.js and rename it to dnsException. Simplify
  it's logic so it no longer calls errnoException and skips
  the unnecessary argument checks.

Backport-PR-URL: #19191
PR-URL: #18546
Reviewed-By: James M Snell <[email protected]>
rvagg pushed a commit that referenced this pull request Aug 16, 2018
@BridgeAR @rvagg
errors: make message non-enumerable
98f5b17 
A error message should always be non-enumerable. This makes sure
that is true for dns errors as well. It also adds another check
in `common.expectsError` to make sure no other regressions are
introduced going forward.

Fixes #19716

Backport-PR-URL: #19191
PR-URL: #19719
Fixes: #19716
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@gibfahn gibfahn

Labels
blocked PRs that are blocked by other issues or PRs. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.