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

v9.8.0 proposal #19181

Merged
merged 73 commits into from
Mar 8, 2018
Merged

v9.8.0 proposal #19181

merged 73 commits into from
Mar 8, 2018

Conversation

MylesBorins
Copy link
Contributor

@MylesBorins MylesBorins commented Mar 6, 2018

2018-03-07, Version 9.8.0 (Current), @MylesBorins

Notable Changes

  • crypto:
    • add cert.fingerprint256 as SHA256 fingerprint (Hannes Magnusson) #17690
  • http2:
    • Fixed issues with aborted connections in the HTTP/2 implementation (Anna Henningsen) #18987 #19002
  • loader:
    • --inspect-brk now works properly for esmodules (Gus Caplan) #18949
  • src:
    • make process.dlopen() load well-known symbol (Ben Noordhuis) #18934
  • trace_events:
    • add file pattern cli option (Andreas Madsen) #18480
  • Added new collaborators

Commits

danbev and others added 30 commits March 5, 2018 09:43
Currently, when configured --without-ssl test-repl-tab-complete fails
with the following error:

assert.js:43
  throw new errors.AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: [ [], 'lexicalL' ] deepStrictEqual []
    at testRepl.complete.common.mustCall
      (node/test/parallel/test-repl-tab-complete.js:549:14)
    at /node/test/common/index.js:530:15
    at completionGroupsLoaded (repl.js:1204:5)
    at REPLServer.complete (repl.js:1090:11)
    at REPLServer.completer (repl.js:450:14)
    at REPLServer.complete (repl.js:919:18)
    at __dirname.forEach (parallel/test-repl-tab-complete.js:548:14)
    at Array.forEach (<anonymous>)
    at Object.<anonymous> (parallel/test-repl-tab-complete.js:545:29)
    at Module._compile (module.js:660:30)

This commit attempts to fix this test but I'm not sure if this is a
proper fix as I'm not familiar with the repl code base yet.

PR-URL: #17867
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: #18875
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: #17690
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: #18895
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: #18981
Fixes: #18978
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: James M Snell <[email protected]>
PR-URL: #18928
Reviewed-By: Claudio Rodriguez <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
There are two minor issues in the AsyncHook constructor, if the object
passed in has an after and/or destroy property that are not functions
the errors thrown will still be:
TypeError [ERR_ASYNC_CALLBACK]: before must be a function

This commit updates the code and adds a unit test.

PR-URL: #19000
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
PR-URL: #18958
Fixes: #18938
Ref: nodejs/build#1145
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
PR-URL: #18949
Fixes: #18948
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Eugene Ostroukhov <[email protected]>
Reviewed-By: James M Snell <[email protected]>
When socket is closed on a response for a request that is being piped to
a stream there is a condition where aborted event will be fired to http
client when socket is closing and the incomingMessage stream is still
set to readable.

We need a check for request being complete and to only raise the
'aborted' event on the http client if we have not yet completed reading
the response from the server.

Fixes: #18756

PR-URL: #18999
Reviewed-By: Shingo Inoue <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Tests in progress to reproduce issue consistently.

Fixes: #18756

PR-URL: #18999
Reviewed-By: Shingo Inoue <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
PR-URL: #18999
Reviewed-By: Shingo Inoue <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Directory symlinks in Windows require the 'dir' flag to be passed to
create the symlink correctly.

PR-URL: #19049
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: #19052
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
PR-URL: #19053
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
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.

PR-URL: #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]>
Make the test for pending deprecations work when the env var
is set during the whole test suite run.

PR-URL: #18991
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
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.

PR-URL: #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]>
`async` and `bytes` are only interesting when the write
is coming from JS, and unnecessary otherwise.

Also, make all of the stream `Write*()` bindings use the same
code for setting these, and upgrade to the non-deprecated versions.

PR-URL: #18963
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Passing a pointer to a static integer is sufficient for the test.

PR-URL: #19039
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Hitesh Kanwathirtha <[email protected]>
PR-URL: #18080
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
libuv and zlib symbols are also purposefully re-exported by Node.js for
use in Addons.

Refs: #17444

PR-URL: #19013
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Use common.platformTimeout() to give longer durations to Raspberry Pi
devices to make test more reliable.

PR-URL: #18126
Fixes: #16772
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Eugene Ostroukhov <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
The test was flaky because it relied on a specific order of
asynchronous operation that were fired paralellely. This was true
on most platform and conditions, but not all the time.

See: #18986

PR-URL: #19093
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
PR-URL: #19109
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Documentation for N-API Custom Asynchronous Operations incorrectly
stated that async execution happens on the main event loop.
Added details to napi_create_async_work about which threads are
used to invoke the execute and complete callbacks.

Changed 'async' to 'asynchronous' in the documentation for Custom
Asynchronous Operations. Changed "executes in parallel" to "can
execute in parallel" for the documentation of napi_create_async_work
execute parameter.

PR-URL: #19073
Fixes: #19071
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: #19126
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
The default message will be printed if the assertion fires. Use block
scope for related variables and tests.

PR-URL: #19054
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
TimothyGu and others added 12 commits March 7, 2018 09:30
Fixes: #17892
Fixes: #17893
Fixes: #18992

PR-URL: #18993
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
PR-URL: #19162
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Backport-PR-URL: #19185
PR-URL: #18297
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Replace v8::Persistent with node::Persistent, a specialization that
resets the persistent handle on destruction.  Prevents accidental
resource leaks when forgetting to call .Reset() manually.

I'm fairly confident this commit fixes a number of resource leaks that
have gone undiagnosed so far.

Backport-PR-URL: #19185
PR-URL: #18656
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
The previous commit made persistent handles auto-reset on destruction.
This commit removes the Reset() calls that are now no longer necessary.

Backport-PR-URL: #19185
PR-URL: #18656
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Don't try to update the internal field pointer of the JS object in the
destructor.

The garbage collector invokes the destructor when the object is
collected and is not necessarily in a valid state anymore.

Backport-PR-URL: #19185
PR-URL: #18656
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
This fixes a crash that occurred when a `Http2Stream` write
is completed after it is already destroyed.

Instead, don’t destroy the stream in that case and wait for
GC to take over.

Backport-PR-URL: #19185
PR-URL: #19002
Fixes: #18973
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
This adds support for ensuring that the top-level main into Node is
supported loading when it has no extension for backwards-compat with
NodeJS bin workflows.

In addition package.json caching is implemented in the module lookup
process.

Backport-PR-URL: #18923
PR-URL: #18728
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Backport-PR-URL: #18923
PR-URL: #18788
Refs: #18728
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Backport-PR-URL: #19180
PR-URL: #18925
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
We have two notes in API docs about Android support:
the first has no links, the second links to the table of supported OSs
where Android is not mentioned which may be confusing.

This PR makes both notes link to dedicated Android part of BUILDING.md.

Backport-PR-URL: #19183
PR-URL: #19004
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Backport-PR-URL: #19194
PR-URL: #18607
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins added a commit that referenced this pull request Mar 7, 2018
Notable Changes:

* crypto:
  - add cert.fingerprint256 as SHA256 fingerprint (Hannes Magnusson)
    #17690
* lib:
  - v8_prof_processor works again 🎉 (Anna Henningsen)
    #19059
* loader:
  - --inspect-brk now works properly for esmodules (Gus Caplan)
    #18949
* src:
  - handle exceptions in env-\>SetImmediates (James M Snell)
    #18297
  - make process.dlopen() load well-known symbol (Ben Noordhuis)
    #18934
* trace_events:
  - add file pattern cli option (Andreas Madsen)
    #18480

PR-URL: #19181
@MylesBorins
Copy link
Contributor Author

@addaleax
Copy link
Member

addaleax commented Mar 7, 2018

v8_prof_processor works again

@MylesBorins I wouldn’t put it in the notable changes section. It doesn’t actually work yet, it’s been broken for quite a while for some other reason that we still have to get into. What my patch did was only fixing part of that, and I think it’s not usable again (yet).

@addaleax
Copy link
Member

addaleax commented Mar 7, 2018

handle exceptions in env->SetImmediates

Also not really a notable change, tbh … before the other commits from the same PR, there were no such exceptions happening at all, and we’re not backporting the rest of the PR.

It’s really only backported to v9.x to avoid merge conflicts. :)

(We can also remove the SEMVER-MINOR tag from the changelog here, I guess. It’s just a drawback of the Backport-PR-URL: approach that we can’t tag the backport separately…)

doc: add MoonBall to collaborators

I think some of the others in @nodejs/release always include these as notable changes?

http2: no stream destroy while its data is on the wire

tls,http2: handle writes after SSL destroy more gracefully

I think you can call these out as notable changes. Something like “Fixed issues with aborted connections in the HTTP/2 implementation.” should be fine?

@addaleax
Copy link
Member

addaleax commented Mar 7, 2018

v8_prof_processor works again

@MylesBorins I wouldn’t put it in the notable changes section. It doesn’t actually work yet, it’s been broken for quite a while for some other reason that we still have to get into. What my patch did was only fixing part of that, and I think it’s not usable again (yet).

Correcting myself here: Okay, it appears to be partially working, but the tests still fail on master, and the output doesn't quite seem correct...... Sigh.

Notable Changes:

* crypto:
  - add cert.fingerprint256 as SHA256 fingerprint (Hannes Magnusson)
    #17690
* http2:
  - Fixed issues with aborted connections in the HTTP/2 implementation
    (Anna Henningsen)
    #18987
    #19002
* loader:
  - --inspect-brk now works properly for esmodules (Gus Caplan)
    #18949
* src:
  - make process.dlopen() load well-known symbol (Ben Noordhuis)
    #18934
* trace_events:
  - add file pattern cli option (Andreas Madsen)
    #18480
* Added new collaborators:
  - Chen Gang (MoonBall) https://github.com/MoonBall

PR-URL: #19181
@MylesBorins
Copy link
Contributor Author

Updated the changelog as requested. Also fixed minor linting error (doc related). CITGM looks good just waiting for last arm to finish running

@MylesBorins MylesBorins merged commit 27ba6e2 into v9.x Mar 8, 2018
MylesBorins added a commit that referenced this pull request Mar 8, 2018
MylesBorins added a commit that referenced this pull request Mar 8, 2018
Notable Changes:

* crypto:
  - add cert.fingerprint256 as SHA256 fingerprint (Hannes Magnusson)
    #17690
* http2:
  - Fixed issues with aborted connections in the HTTP/2 implementation
    (Anna Henningsen)
    #18987
    #19002
* loader:
  - --inspect-brk now works properly for esmodules (Gus Caplan)
    #18949
* src:
  - make process.dlopen() load well-known symbol (Ben Noordhuis)
    #18934
* trace_events:
  - add file pattern cli option (Andreas Madsen)
    #18480
* Added new collaborators:
  - Chen Gang (MoonBall) https://github.com/MoonBall

PR-URL: #19181
@gibfahn gibfahn deleted the v9.8.0-proposal branch March 8, 2018 19:28
@targos targos mentioned this pull request Mar 18, 2018
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
Notable Changes:

* crypto:
  - add cert.fingerprint256 as SHA256 fingerprint (Hannes Magnusson)
    nodejs#17690
* http2:
  - Fixed issues with aborted connections in the HTTP/2 implementation
    (Anna Henningsen)
    nodejs#18987
    nodejs#19002
* loader:
  - --inspect-brk now works properly for esmodules (Gus Caplan)
    nodejs#18949
* src:
  - make process.dlopen() load well-known symbol (Ben Noordhuis)
    nodejs#18934
* trace_events:
  - add file pattern cli option (Andreas Madsen)
    nodejs#18480
* Added new collaborators:
  - Chen Gang (MoonBall) https://github.com/MoonBall

PR-URL: nodejs#19181
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. meta Issues and PRs related to the general management of the project. openssl Issues and PRs related to the OpenSSL dependency. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.