This commit set the Environment as a thread local when CreateEnvironment
is called which is currently not being done. This would lead to a
segment fault if later node::AtExit is called without specifying the
environment parameter. This specific issue was reported by Electron.
If I recall correctly, back when this was implemented the motivation was
that if embedders have multiple environments per isolate they should be
using the AtExit functions that take an environment. This is not the
case with Electron which only create a single environment (as far as I
know), and if a native module calls AtExit this would lead to the
segment fault.
I was able to reproduce Electron issue and the provided test simulates
it. I was also able to use this patch and verify that it works for the
Electron issue as well.
PR-URL: https://github.com/nodejs/node/pull/18573
Refs: https://github.com/nodejs/node/pull/9163
Refs: https://github.com/electron/electron/issues/11299
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Matheus Marchini <matheus@sthima.com>
Instead of using nextTick to process failed lists, just attempt to
process them again from C++ if the process is still alive.
This also allows the removal of domain specific code in timers.
The current behaviour is not quite ideal as it means that all lists
after the failed one will process on an arbitrary nextTick, even if
they're — say — not due to fire for another 2 days...
PR-URL: https://github.com/nodejs/node/pull/18486
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Up until now, Node did not clear the current error code
attempting to read environment variables on Windows.
Since checking the error code is the way we distinguish between
missing and zero-length environment variables, this could lead to a
false positive when the error code was still tainted.
In the simplest case, accessing a missing variable and then a
zero-length one would lead Node to believe that both calls yielded
an error.
Before:
> process.env.I=''; process.env.Q; process.env.I
undefined
> process.env.I=''; /*process.env.Q;*/ process.env.I
''
After:
> process.env.I=''; process.env.Q; process.env.I
''
> process.env.I=''; /*process.env.Q;*/ process.env.I
''
This only affects Node 8 and above, since before
1aa595e5bd we always constructed a
`v8::String::Value` instance for passing the lookup key to the OS,
which in in turn always made a heap allocation and therefore
reset the error code.
PR-URL: https://github.com/nodejs/node/pull/18463
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
currently if you want to use dynamic import you must use both the
`--experimental-modules` and the `--harmony-dynamic-imports` flags.
Chrome is currently shipping dynamic import unflagged, the flag
only remains in V8 to guard embedders who have not set the appropriate
callback from throwing an unhandled rejection when the feature is used.
As such it is reasonable to enable the flag by default for
`--experimental-modules`
PR-URL: https://github.com/nodejs/node/pull/18387
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Currently, both process.binding and internalBinding have to call into
C++ regardless of whether the module has been cached or not. This
creates significant overhead to all binding calls and unfortunately
the rest of the codebase doesn't really optimize the amount of times
that bindings are required (as an example: 12 files require the
async_wrap binding).
Changing all the usage of this function throughout the codebase would
introduce a lot of churn (and is kind of a hassle) so instead this PR
introduces a JS fast path for both functions for cases where the
binding has already been cached. While micro benchmarks are not super
meaningful here (it's not like we call binding that often...), this
does speed up the cached call by 400%.
In addition, move moduleLoadList creation and management entirely into
JS-land as it requires less code and is more efficient.
PR-URL: https://github.com/nodejs/node/pull/18365
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Move the majority of C++ domain-related code into JS land by introducing
a top level domain callback which handles entering & exiting the domain.
Move the rest of the domain necessities into their own file that creates
an internal binding, to avoid exposing domain-related code on the
process object.
Modify an existing test slightly to better test domain-related code.
PR-URL: https://github.com/nodejs/node/pull/18291
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Since the `DestroyAsyncIdsCallback` in Node.js can be scheduled as
a result of GC, that means that it can accidentally keep the event
loop open when it shouldn't.
Replace `SetImmediate` with the newly introduced `SetUnrefImmediate`
and in addition introduce RunBeforeExit callbacks, of which
`DestroyAsyncIdsCallback` is now the first. These callbacks will run
before the `beforeExit` event is emitted (which will now only be
emitted if the event loop is still not active).
PR-URL: https://github.com/nodejs/node/pull/18241
Fixes: https://github.com/nodejs/node/issues/18190
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
Remove the unnecessary microTasksTickObject for scheduling microtasks
and instead use TickInfo to keep track of whether promise rejections
exist that need to be emitted. Consequently allow the microtasks to
execute on average fewer times, in more predictable manner than
previously.
Simplify unhandled & handled rejection tracking to do more in C++ to
avoid needing to expose additional info in JS.
When new unhandledRejections are emitted within an unhandledRejection
handler, allow the event loop to proceed first instead. This means
that if the end-user code handles all promise rejections on nextTick,
rejections within unhandledRejection now won't spiral into an infinite
loop.
PR-URL: https://github.com/nodejs/node/pull/18207
Fixes: https://github.com/nodejs/node/issues/17913
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Do not share unnecessary information about nextTick state
between JS & C++, instead only track whether a nextTick
is scheduled or not.
Turn nextTickQueue into an Object instead of a class
since multiple instances are never created.
Other assorted refinements and refactoring.
PR-URL: https://github.com/nodejs/node/pull/17738
Reviewed-By: Anna Henningsen <anna@addaleax.net>
- Handle exceptions when getting `process.emitWarning` or when
calling it properly
- Add `Maybe<bool>` to the return type, like the V8 API uses it
to indicate failure conditions
- Update call sites to account for that and clean up/return to JS
when encountering an error
- Add an internal `ProcessEmitDeprecationWarning()` sibling
for use in https://github.com/nodejs/node/pull/17417,
with common code extracted to a helper function
- Allow the warning to contain non-Latin-1 characters. Since the
message will usually be a template string containing data passed
from the user, this is the right thing to do.
- Add tests for the failure modes (except string creation failures)
and UTF-8 warning messages.
PR-URL: https://github.com/nodejs/node/pull/17420
Refs: https://github.com/nodejs/node/pull/17417
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Keep track of C++ `TryCatch` state to avoid aborting when
an exception is thrown inside one, and re-throw in JS
to make sure the exception is being picked up a second time by
a second uncaught exception handler, if necessary.
Add a bit of a hack to `AppendExceptionLine` to avoid overriding
the line responsible for re-throwing the exception.
PR-URL: https://github.com/nodejs/node/pull/17394
Fixes: https://github.com/nodejs/node/issues/13258
Reviewed-By: James M Snell <jasnell@gmail.com>
Garbage collection is triggered by the heap object tracker. The garbage
collector may spawn garbage collection tasks on the foreground thread.
This is only allowed after registering the isolate in the platform
which happens in IsolateData. This CL moves the starting of the heap
object tracker after calling IsolateData to ensure that.
PR-URL: https://github.com/nodejs/node/pull/17249
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Add a utility functions for addons to use when they need
a reference to the current event loop.
Currently, `uv_default_loop()` works if the embedder is the
single-threaded default node executable, but even without
the presence of e.g. workers that might not really an API
guarantee for when Node is embedded.
PR-URL: https://github.com/nodejs/node/pull/17109
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Report (for example) "node[1337]" as the human-readable name rather
than the more generic and less helpful "Node.js Main Context."
While not perfect yet, it should be an improvement to people that
debug multiple processes from DevTools, VS Code, etc.
PR-URL: https://github.com/nodejs/node/pull/17087
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
This removes the `process._needImmediateCallback` property
and its semantics of having a 1/0 switch that tells C++ whether
immediates are currently scheduled.
Instead, a counter keeping track of all immediates is created,
that can be increased on `setImmediate()` or decreased when an
immediate is run or cleared.
This is faster, because rather than reading/writing a C++ getter,
this operation can be performed as a direct memory read/write via
a typed array. The only C++ call that is left to make is
activating the native handles upon creation of the first
`Immediate` after the queue is empty.
One other (good!) side-effect is that `immediate._destroyed` now
reliably tells whether an `immediate` is still scheduled to run or not.
Also, as a nice extra, this should make it easier to implement
an internal variant of `setImmediate` for C++ that piggybacks
off the same mechanism, which should be useful at least for
async hooks and HTTP/2.
Benchmark results:
$ ./node benchmark/compare.js --new ./node --old ./node-master-1b093cb93df0 --runs 10 --filter immediate timers | Rscript benchmark/compare.R
[00:08:53|% 100| 4/4 files | 20/20 runs | 1/1 configs]: Done
improvement confidence p.value
timers/immediate.js type="breadth" thousands=2000 25.61 % ** 1.432301e-03
timers/immediate.js type="breadth1" thousands=2000 7.66 % 1.320233e-01
timers/immediate.js type="breadth4" thousands=2000 4.61 % 5.669053e-01
timers/immediate.js type="clear" thousands=2000 311.40 % *** 3.896291e-07
timers/immediate.js type="depth" thousands=2000 17.54 % ** 9.755389e-03
timers/immediate.js type="depth1" thousands=2000 17.09 % *** 7.176229e-04
timers/set-immediate-breadth-args.js millions=5 10.63 % * 4.250034e-02
timers/set-immediate-breadth.js millions=10 20.62 % *** 9.150439e-07
timers/set-immediate-depth-args.js millions=10 17.97 % *** 6.819135e-10
PR-URL: https://github.com/nodejs/node/pull/17064
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Worker threads need an event loop without active libuv handles in
order to shut down. One source of handles that was previously
not accounted for were delayed V8 tasks; these create timers
that would be standing in the way of clearing the event loop.
To solve this, keep track of the scheduled tasks in a list
and close their timer handles before the corresponding isolate/loop
is removed from the platform.
It is not clear from the V8 documentation what the expectation is
with respect to pending background tasks at the end of the
isolate lifetime; however, an alternative approach of executing
these scheduled tasks when flushing them led to an infinite loop
of tasks scheduling each other; so it seems safe to assume that
the behaviour implemented in this patch is at least acceptable.
Original-PR-URL: https://github.com/ayojs/ayo/pull/120
Original-Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
PR-URL: https://github.com/nodejs/node/pull/16700
Reviewed-By: James M Snell <jasnell@gmail.com>
Daniel Bevenius