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.

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

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

Closed

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

@MylesBorins
Copy link
Contributor

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

@joyeecheung please could you rebase?

@joyeecheung
Copy link
Member Author

@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
This adds the Http1IncomingMessage and Http1ServerReponse options
to http2.createServer().

Backport-PR-URL: nodejs#20456
PR-URL: nodejs#15752
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
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 <matteo.collina@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
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 <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Backport-PR-URL: nodejs#20456
PR-URL: nodejs#18609
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
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 <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
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 <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Backport-PR-URL: nodejs#20456
PR-URL: nodejs#18872
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
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 <matteo.collina@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Backport-PR-URL: nodejs#20456
PR-URL: nodejs#18895
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
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 <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
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 <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
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 <jasnell@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
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 <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
PR-URL: nodejs#18358
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
PR-URL: nodejs#18546
Reviewed-By: James M Snell <jasnell@gmail.com>
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 <jasnell@gmail.com>
@joyeecheung
Copy link
Member Author

joyeecheung commented May 2, 2018

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 <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@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
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 <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request May 22, 2018
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 <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request May 22, 2018
Backport-PR-URL: #19191
PR-URL: #18358
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
MylesBorins pushed a commit that referenced this pull request May 22, 2018
Backport-PR-URL: #19191
PR-URL: #18546
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 22, 2018
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 <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 22, 2018
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 <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins
Copy link
Contributor

landed in 591812f...a974479

MylesBorins pushed a commit that referenced this pull request Jun 14, 2018
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 <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Jun 14, 2018
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 <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Jun 14, 2018
Backport-PR-URL: #19191
PR-URL: #18358
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
MylesBorins pushed a commit that referenced this pull request Jun 14, 2018
Backport-PR-URL: #19191
PR-URL: #18546
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jun 14, 2018
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 <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jun 14, 2018
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 <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
rvagg pushed a commit that referenced this pull request Aug 16, 2018
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 <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
rvagg pushed a commit that referenced this pull request Aug 16, 2018
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 <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
rvagg pushed a commit that referenced this pull request Aug 16, 2018
Backport-PR-URL: #19191
PR-URL: #18358
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
rvagg pushed a commit that referenced this pull request Aug 16, 2018
Backport-PR-URL: #19191
PR-URL: #18546
Reviewed-By: James M Snell <jasnell@gmail.com>
rvagg pushed a commit that referenced this pull request Aug 16, 2018
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 <jasnell@gmail.com>
rvagg pushed a commit that referenced this pull request Aug 16, 2018
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 <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.