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.x backport] backport 18607, 18953 #19194

Closed
wants to merge 25 commits into from

Conversation

lpinca
Copy link
Member

@lpinca lpinca commented Mar 7, 2018

This is a backport of #18607 and #18953.

bnoordhuis and others added 24 commits March 6, 2018 15:25
Original commit message:

    Use wider types for max_old_space_size and co.

    Make --max_old_space_size and friends work with values >= 2**31.
    Such values did not work reliably (or sometimes not all) due to
    signed integer overflow in size computations, which is UB.

    Fixes nodejs#18786.

    Bug: chromium:814138
    Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
    Change-Id: Ibe23cef2417fd5b4a727022b8b0d4b50f1417182
    Reviewed-on: https://chromium-review.googlesource.com/927063
    Commit-Queue: Ben Noordhuis <[email protected]>
    Reviewed-by: Michael Lippautz <[email protected]>
    Cr-Commit-Position: refs/heads/master@{nodejs#51433}

PR-URL: nodejs#18920
Fixes: nodejs#18786
Reviewed-By: Yang Guo <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Move some code around and clean up the DLib helper class as prep work
for a follow-up commit.

PR-URL: nodejs#18934
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Look for symbol `node_register_module_v${NODE_MODULE_VERSION}` if the
add-on didn't self-register.  This can be used to create add-ons that
support multiple Node.js versions from a single shared object.

PR-URL: nodejs#18934
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Fix the gcovr version to a fixed version and uses patches
specific to that version. This avoids us being broken by
changes in the gcovr repo. Using file name for patches
specific to the version level will allow us to move up when
necessary without breaking coverage for earlier versions
of Node.js

PR-URL: nodejs#19094
Reviewed-By: Gibson Fahnestock <[email protected]>
ensures that instantiation result is only used during initial loading

PR-URL: nodejs#18965
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Added a simple example showing how to rename
a file.

Refs: https://github.com/nodejs/node/issues11135

PR-URL: nodejs#18812
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Add team that we can use to cc organizations or people
who do one of the following:

* bundle node into their product
* bundle node into their distributions
* provide an installer
* provide a version manager

PR-URL: nodejs#19098
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Original commit message:

    Introduce --disallow-code-generation-from-strings

    Exposing the existing Context::AllowCodeGenerationFromStrings(false) API
    to the command line.

    Bug: v8:7134
    Change-Id: I062ccff0b03c5bcf6878c41c455c0ded37a1d743
    Reviewed-on: https://chromium-review.googlesource.com/809631
    Reviewed-by: Michael Starzinger <[email protected]>
    Commit-Queue: Jakob Kummerow <[email protected]>
    Cr-Commit-Position: refs/heads/master@{nodejs#49911}

PR-URL: nodejs#18212
Refs: v8/v8@0bcb1d6
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Allow users to switch to the table of contents for older versions of
Node.js. This gets rid of the "Failed to add alternative version link"
warnings when building docs.

PR-URL: nodejs#19113
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
PR-URL: nodejs#19099
Reviewed-By: Shingo Inoue <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
There are some uses of "in general" that are unnecessary. (We don't need
to be tentative about the fact that tests should pass, for example.)

PR-URL: nodejs#19123
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Remove request that user provide a list of subsystems in the pull
request template.

* We already ask them to put the subsystems in the first line of the
  commit message.
* The subsystem is usually easy to determine.
* We have a bot that applies subsystem labels on new PRs and it seems to
  do a rather good job. Tools over rules. Let the bot do it.
* The fewer unnecessary things we ask for in the template, the lower the
  barrier to entry.
* Anecdotal, but I have never found it useful to have the person
  submitting the PR list out subsystems in the pull request post.

PR-URL: nodejs#19125
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Kyle Farnung <[email protected]>
PR-URL: nodejs#19087
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Convert manual memory management of STACK_OF(X509) instances to
std::unique_ptr with a custom deleter.

Note the tasteful application of std::move() to indicate a function
that consumes its argument.

PR-URL: nodejs#19087
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Currently test-http2-client-write-empty-string.js will throw "Error
[ERR_NO_CRYPTO]: Node.js is not compiled with OpenSSL crypto support"
when configured --without-ssl.

This commit moves the require of http2 to after the crypto check to
avoid this error.

PR-URL: nodejs#19111
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
iculslocs uses stdio, but didn't include the header.

PR-URL: nodejs#19150
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Fixes: nodejs#17892
Fixes: nodejs#17893
Fixes: nodejs#18992

PR-URL: nodejs#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: nodejs#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: nodejs#19185
PR-URL: nodejs#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: nodejs#19185
PR-URL: nodejs#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: nodejs#19185
PR-URL: nodejs#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: nodejs#19185
PR-URL: nodejs#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: nodejs#19185
PR-URL: nodejs#19002
Fixes: nodejs#18973
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
PR-URL: nodejs#18607
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@nodejs-github-bot nodejs-github-bot added net Issues and PRs related to the net subsystem. v9.x labels Mar 7, 2018
@lpinca lpinca changed the title [v9.x backport] net: inline and simplify onSocketEnd [v9.x backport] backport 18607, 18953 Mar 7, 2018
The listener does not do anything if `allowHalfOpen` is enabled. Add it
only when `allowHalfOpen` is disabled.

PR-URL: nodejs#18953
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@lpinca
Copy link
Member Author

lpinca commented Mar 7, 2018

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Thanks!

MylesBorins pushed a commit that referenced this pull request Mar 7, 2018
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
Copy link
Contributor

landed in ef4714c

@MylesBorins MylesBorins closed this Mar 7, 2018
@lpinca
Copy link
Member Author

lpinca commented Mar 7, 2018

@MylesBorins what happened to the other commit here? Should I open a separate PR?

@addaleax
Copy link
Member

addaleax commented Mar 7, 2018

@lpinca I think it should have landed together with the other commit here. Anyway, it does land cleanly now, so I’ve removed the backport-requested label from the original PR, so that it will get picked up by the next update to v9.x-staging that a releaser does anyway.

@lpinca lpinca deleted the backport/18607 branch March 7, 2018 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
net Issues and PRs related to the net subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.