Commit c5b07d4 ("lib: fix beforeExit not working with -e") runs the
to-be-evaluated code at a later time than before because it switches
from `process.nextTick()` to `setImmediate()`.
It affects `-e 'process.on("message", ...)'` because there is now a
larger time gap between startup and attaching the event listener,
increasing the chances of missing early messages. I'm reasonably
sure `process.nextTick()` was also susceptible to that, only less
pronounced.
Avoid the problem altogether by evaluating the code synchronously.
Harmonizes the logic with `Module.runMain()` from lib/module.js
which also calls `process._tickCallback()` afterwards.
PR-URL: https://github.com/nodejs/node/pull/11958
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Ensure that AdjustAmountOfExternalAllocatedMemory() is called when
the SecurePair is destroyed. Not doing so is not an actual memory
leak but it makes `process.memoryUsage().external` wildly inaccurate
and can cause performance problems due to excessive garbage collection.
PR-URL: https://github.com/nodejs/node/pull/11896
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
The allowHalfOpen comment was added in commit 8a3befa ("net: Refactor
to use streams2") from 2012 but it wasn't true even then as far as I
can tell: Node.js simply always does a shutdown(2) first.
It is true that streams2 withholds the 'end' event when allowHalfOpen
is true but the comment is about a callback that hangs off the 'finish'
event that is emitted after calling `socket.end()`.
PR-URL: https://github.com/nodejs/node/pull/11573
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Remove the explicit dependency on $(NODE_EXE), it always triggers a
rebuild due to it being a .PHONY rule. Add-ons in test/addons/ have
the same implicit dependency so it isn't completely without precedent.
PR-URL: https://github.com/nodejs/node/pull/11311
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
The previous commit stores baked-in files with non-ASCII characters
as UTF-16. Replace the \u2019 with a regular quote character so that
the files they're in can be stored as one-byte strings. The UTF-16
functionality is still tested by the Unicode diagram in lib/timers.js.
PR-URL: https://github.com/nodejs/node/pull/11129
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Commit a8734af ("src: make copies of startup environment variables")
from two weeks ago introduced a regression in the capturing of the
`--icu-data-dir=` switch: it captured the string up to the `=` instead
of what comes after it.
PR-URL: https://github.com/nodejs/node/pull/11255
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Replace node-weak with a small hand-rolled add-on. We can now drop
node-weak and nan, reducing the size of the source tree by about 750 kB
and the size of the tarball by about 150-300 kB.
PR-URL: https://github.com/nodejs/node/pull/11239
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Add a compile_commands.json generator for use with clang-based tooling.
Pass the (undocumented) -C switch to configure to make it generate the
files in out/Debug and out/Release.
PR-URL: https://github.com/nodejs/node/pull/7986
Reviewed-By: James M Snell <jasnell@gmail.com>
Fix a warning that was introduced in commit 67af1ad ("src: refactor
CopyProperties to remove JS") from a few days ago. This particular
change was suggested by me, mea culpa.
Fixes the following warning:
../src/node_contextify.cc:151:13: warning: ignoring return
value of function declared with warn_unused_result attribute
[-Wunused-result]
sandbox_obj->DefineProperty(context, key, *desc);
PR-URL: https://github.com/nodejs/node/pull/11197
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Check that invoking a callback on a receiver from a different context
works.
It ran afoul of an `env->context() == isolate->GetCurrentContext()`
assertion so retrieve the environment from the callback context and
the context to enter from the environment's context() method.
We could also have retrieved the environment from the receiver's context
and that would have made little practical difference. It just seemed
more correct to get it from the callback context because that is the
actual execution context.
PR-URL: https://github.com/nodejs/node/pull/9221
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Use zero-copy external string resources for storing the built-in JS
source code. Saves a few hundred kilobyte of memory and consistently
speeds up `benchmark/misc/startup.js` by 2.5%.
Everything old is new again! Commit 74954ce ("Add string class that
uses ExternalAsciiStringResource.") from 2011 did the same thing but
I removed that in 2013 in commit 34b0a36 ("src: don't use NewExternal()
with unaligned strings") because of a limitation in the V8 API.
V8 no longer requires that strings are aligned if they are one-byte
strings so it should be safe to re-enable external strings again.
PR-URL: https://github.com/nodejs/node/pull/5458
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com>
Commit 93a44d5 ("src: fix deferred events not working with -e") defers
evaluation of the script to the next tick.
A side effect of that change is that 'beforeExit' listeners run before
the actual script. 'beforeExit' is emitted when the event loop is
empty but process.nextTick() does not ref the event loop.
Fix that by using setImmediate(). Because it is implemented in terms
of a uv_check_t handle, it interacts with the event loop properly.
Fixes: https://github.com/nodejs/node/issues/8534
PR-URL: https://github.com/nodejs/node/pull/8821
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
There is no point in trying to search for files in a directory that
we know does not exist, so stop doing that.
Reduces the total number of stat(2) calls and the number of stat(2)
misses on a medium-sized application by about 21% and 29% respectively.
Reduces the total number of package.json open(2) calls and the number
of open(2) misses by about 21% and 93% (!) respectively.
Before:
% time seconds usecs/call calls errors syscall
------ ----------- ----------- --------- --------- ----------------
50.93 0.178419 38 4702 lstat
29.08 0.101875 36 2800 2010 stat
11.36 0.039796 43 932 215 open
5.39 0.018897 34 550 fstat
3.24 0.011337 34 336 pread
------ ----------- ----------- --------- --------- ----------------
100.00 0.350324 9320 2225 total
After:
% time seconds usecs/call calls errors syscall
------ ----------- ----------- --------- --------- ----------------
55.49 0.176638 38 4702 lstat
24.76 0.078826 35 2225 1435 stat
10.19 0.032434 44 733 16 open
6.19 0.019719 36 550 fstat
3.37 0.010723 32 336 pread
------ ----------- ----------- --------- --------- ----------------
100.00 0.318340 8546 1451 total
PR-URL: https://github.com/nodejs/node/pull/9196
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Discourage using require.extensions because it slows down the module
loader. The number of file system operations that the module system
has to perform in order to resolve a `require(...)` statement to a
filename is proportional to the number of registered extensions.
PR-URL: https://github.com/nodejs/node/pull/9196
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
NodeInstanceData is not used meaningfully and makes the initialization
logic harder to follow. Let's remove it and delete 100 lines of code
in one fell swoop.
PR-URL: https://github.com/nodejs/node/pull/9224
Reviewed-By: James M Snell <jasnell@gmail.com>
Don't bother shrinking the read buffer on the final read because we
dispose it immediately afterwards. Avoids some unnecessary memory
allocation and copying.
PR-URL: https://github.com/nodejs/node/pull/9132
Reviewed-By: James M Snell <jasnell@gmail.com>
Fix a regression introduced in commit 2996b5c ("crypto: Allow GCM
ciphers to have a longer IV length") from April 2016 where a misplaced
parenthesis in a 'is ECB cipher?' check made it possible to use empty
IVs with non-ECB ciphers.
Also fix some exit bugs in test/parallel/test-crypto-authenticated.js
that were introduced in commit 4a40832 ("test: cleanup IIFE tests")
where removing the IFFEs made the test exit prematurely instead of just
skipping subtests.
PR-URL: https://github.com/nodejs/node/pull/9032
Refs: https://github.com/nodejs/node/pull/6376
Refs: https://github.com/nodejs/node/issues/9024
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Unsanitized paths containing line feed characters can be used for
header injection and request splitting so reject them with an exception.
There seems to be no reasonable use case for allowing control characters
(characters <= 31) while there are several scenarios where they can be
used to exploit software bugs so reject control characters altogether.
PR-URL: https://github.com/nodejs/node/pull/8923
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: not-an-aardvark <not-an-aardvark@users.noreply.github.com>
Teach gtest to produce TAP so we can integrate it better with our CI
tooling.
TAP is printed to stdout but it can also be written to file by passing
the `--gtest_output=tap:filename.tap` switch to cctest.
PR-URL: https://github.com/nodejs/node/pull/8034
Reviewed-By: James M Snell <jasnell@gmail.com>
Compile out hardware engines. Most are stubs that dynamically load
the real driver but that poses a security liability when an attacker
is able to create a malicious DLL in one of the default search paths.
PR-URL: https://github.com/nodejs/node-private/pull/73
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Remove strings from the PER_ISOLATE_STRING_PROPERTIES list that are
only used once during initialization. It's less expensive to simply
create them when needed than turn them into v8::Eternal instances.
PR-URL: https://github.com/nodejs/node/pull/8207
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Commit 2272052 ("net: bind to `::` TCP address by default") from
April 2014 seems to have accidentally changed the default listen
address from 127.0.0.1 to 0.0.0.0, a.k.a. the "any" address.
From a security viewpoint it's undesirable to accept debug agent
connections from anywhere so let's change that back. Users can
override the default with the `--debug=<host>:<port>` switch.
Fixes: https://github.com/nodejs/node/issues/8081
PR-URL: https://github.com/nodejs/node/pull/8106
Reviewed-By: James M Snell <jasnell@gmail.com>
Thin archives were disabled in 2012 as a workaround (IIRC) for obsolete
tooling on one of Joyent's platforms. The last binutils versions that
didn't support them was released in 2007 so I think it's safe to assume
we can drop support for that now - except on SmartOS, where the tooling
still has a distinctive vintage feel to it.
Thin archives save space - it shrinks the size of PRODUCT_DIR by 30% -
and speed up the final linking step because it doesn't have to assemble
50 MB of static archives (twice! - first to create the archive, then to
copy it to PRODUCT_DIR). The archives are just 3.5 MB now and no longer
copied around.
PR-URL: https://github.com/nodejs/node/pull/7957
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: James M Snell <jasnell@gmail.com>
Make the inspector code easier to reason about by restructuring it
to avoid manual memory allocation and copying as much as possible.
An amusing side effect is that it reduces the total amount of memory
used in the test suite.
Before:
$ valgrind ./out/Release/cctest 2>&1 | grep 'total heap' | cut -c31-
1,017 allocs, 1,017 frees, 21,695,456 allocated
After:
$ valgrind ./out/Release/cctest 2>&1 | grep 'total heap' | cut -c31-
869 allocs, 869 frees, 14,484,641 bytes allocated
PR-URL: https://github.com/nodejs/node/pull/7906
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Fix handle leaks in Buffer::New() and Buffer::Copy() by creating the
handle scope before looking up the env with Environment::GetCurrent().
Environment::GetCurrent() calls v8::Isolate::GetCurrentContext(), which
creates a handle in the current scope, i.e., the scope created by the
caller of Buffer::New() or Buffer::Copy().
PR-URL: https://github.com/nodejs/node/pull/7711
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Disable stdio buffering, it interacts poorly with printf() calls from
elsewhere in the program (e.g., any logging from V8.) Unbreaks among
other things the `--trace_debug_json` switch.
Undoes commit 0966ab99 ("src: force line buffering for stderr"), which
in retrospect is not a proper fix. Turning on line buffering fixed a
flaky test on SmartOS but the test wasn't failing on other platforms,
where stderr wasn't line-buffered either. Mark the test flaky again,
it failed once in a run of 333 tries on the smartos-64 buildbot.
Disabling buffering should be safe even when mixed with non-blocking
stdio I/O because libuv goes to great lengths to reopen the tty file
descriptors and falls back to blocking I/O when that fails.
PR-URL: https://github.com/nodejs/node/pull/7610
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
`--debug=1.2.3.4:5678` and `--debug=example.com:5678` are now accepted,
likewise the `--debug-brk` and `--debug-port` switch. The latter is
now something of a misnomer but it's undocumented and for internal use
only so it shouldn't matter too much.
`--inspect=1.2.3.4:5678` and `--inspect=example.com:5678` are also
accepted but don't use the host name yet; they still bind to the
default address.
Fixes: https://github.com/nodejs/node/issues/3306
PR-URL: https://github.com/nodejs/node/pull/3316
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Ben Noordhuis