Previously, fs.readdirSync calls the function returned by
env->push_values_to_array_function() in batch and check the returned
Maybe right away in C++, which can lead to assertions if the call stack
already reaches the maximum size. This patch fixes that by returning
early the call fails so the stack overflow error will be properly
thrown into JS land.
PR-URL: https://github.com/nodejs/node/pull/18647
Fixes: https://github.com/nodejs/node/issues/18645
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Replace var for let or const.
PR-URL: https://github.com/nodejs/node/pull/18649
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Julian Duque <julianduquej@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
The specific issues raised by Coverity are:
** CID 182716: Control flow issues (DEADCODE)
/src/node_file.cc: 1192
>>> CID 182716: Control flow issues (DEADCODE)
>>> Execution cannot reach this statement:
"args->GetReturnValue().Set(...".
** CID 182715: Uninitialized members (UNINIT_CTOR)
/src/node_file.h: 29
>>> CID 182715: Uninitialized members (UNINIT_CTOR)
>>> Non-static class member "syscall_" is not initialized in this
constructor nor in any functions that it calls.
PR-URL: https://github.com/nodejs/node/pull/18629
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.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.
PR-URL: https://github.com/nodejs/node/pull/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>
Implement string decoder in C++. The perks are a decent speed boost
(for decoding, whereas creation show some performance degradation),
that this can now be used more easily to add native decoding support
to C++ streams and (arguably) more readable variable names.
PR-URL: https://github.com/nodejs/node/pull/18537
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Tests relying on synchronous timing have been migrated to use events.
PR-URL: https://github.com/nodejs/node/pull/17828
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Adds the remaining options from tls.createSecureContext() to the
string generated by Agent#getName(). This allows https.request() to
accept the options and generate unique sockets appropriately.
PR-URL: https://github.com/nodejs/node/pull/16402
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
In ICU 61.x, icu4c will no longer put its declarations in the global namespace.
Everything will be in the "icu::" namespace (or icu_60:: in the linker).
Prepare for this.
https://ssl.icu-project.org/trac/ticket/13460
This adds puctiations to the comments, uses a capital letters for
the first character, removes a few obsolete comments and switches
to assert.ok when suitable.
It also moves all `assert.deepEqual()` and `assert.deepStrictEqual()`
tests to the appropriate file.
PR-URL: https://github.com/nodejs/node/pull/18610
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
The code coverage in `root/internal/vm/Module.js` lacked test coverage
for the url options paramter. The test adds a check to ensure error
is thrown.
PR-URL: https://github.com/nodejs/node/pull/18664
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
The emit{Before,After} APIs in AsyncResource are problematic.
* emit{Before,After} are named to suggest that the only thing they do
is emit the before and after hooks. However, they in fact, mutate
the current execution context.
* They must be properly nested. Failure to do so by user code leads
to catastrophic (unrecoverable) exceptions. It is very easy for the
users to forget that they must be using a try/finally block around
the code that must be surrounded by these operations. Even the
example provided in the official docs makes this mistake. Failing
to use a finally can lead to a catastrophic crash if the callback
ends up throwing.
This change provides a safer `runInAsyncScope` API as an alternative
and deprecates emit{Before,After}.
PR-URL: https://github.com/nodejs/node/pull/18513
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
Instead of using kOnTimeout index to track a special list
processing function, just pass in a function to C++ at
startup that executes all handles and determines which
function to call.
This change improves the performance of unpooled timeouts
by roughly 20%, as well as makes the unref/ref processing
easier to follow.
PR-URL: https://github.com/nodejs/node/pull/18582
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.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.
PR-URL: https://github.com/nodejs/node/pull/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>
Currently doc building doesn't support ES-style default params in
function definitions which causes an error.
PR-URL: https://github.com/nodejs/node/pull/18678
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Gus Caplan <me@gus.host>
The current stack trace thrown in case `assert.throws(fn, object)`
is used did not filter the stack trace. This fixes it.
PR-URL: https://github.com/nodejs/node/pull/18595
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Users of MakeCallback that adds the domain property to carry context,
should start using the async_context variant of MakeCallback or the
AsyncResource class.
PR-URL: https://github.com/nodejs/node/pull/17417
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
For tests that use anonymous namespaces, some tagged the close
of the namespace with 'namespace' while others used
'anonymous namespace'. It was suggested I should use
'anonymous namespace' in a recent PR review so make all of the
tests consistent with this.
PR-URL: https://github.com/nodejs/node/pull/18583
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Due to extensive reliance on timings and the fs module, this test
is currently inherently flaky. Refactor it to simply use setImmediate
and only one busy loop.
PR-URL: https://github.com/nodejs/node/pull/18567
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Unused since 34b535f4ca.
PR-URL: https://github.com/nodejs/node/pull/18568
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Remove the various **Note:** prefixes throughout the docs.
PR-URL: https://github.com/nodejs/node/pull/18592
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
The current output uses JSON.stringify to escape the config values.
This switches to util.inspect to have a better readable output.
PR-URL: https://github.com/nodejs/node/pull/18597
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
The original test case hides the underlying cause by using
`PassThrough`. This change adds a test case for the underlying cause.
This makes it clearer and easier to be understood.
Refs: https://github.com/nodejs/node/pull/18372
PR-URL: https://github.com/nodejs/node/pull/18575
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
When async hooks integration for Timers was introduced, it was
not included in the code for unref'd or subsequently ref'd
timers which means those timers only have Timerwrap hooks.
PR-URL: https://github.com/nodejs/node/pull/18579
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
It's possible for user-code to flip an existing timeout to
be an interval during its execution, in which case the
current code would crash due to start being undefined. Fix
this by providing a default start value within rearm.
PR-URL: https://github.com/nodejs/node/pull/18579
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
A recent commit removed the usage of the second argument of
tryOnTimeout but left the definition in place. Remove it.
PR-URL: https://github.com/nodejs/node/pull/18579
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
The ctx.error is supposed to be handled in fs.readlinkSync,
but was handled in fs.symlinkSync by mistake.
Also fix the error number check in readlink to be consistent
with SYNC_CALL.
PR-URL: https://github.com/nodejs/node/pull/18548
Refs: https://github.com/nodejs/node/pull/18348
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
The method to implement is `_write` not `_transform`.
PR-URL: https://github.com/nodejs/node/pull/18604
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
- Throw getPathFromURL() and nullCheck() errors synchronously instead
of deferring them to the next tick, since we already throw
validatePath() errors synchronously.
- Merge nullCheck() into validatePath()
- Never throws in `fs.exists()`, instead, invoke the callback with
false, or emit a warning when the callback is not a function.
This is to bring it inline with fs.existsSync(), which never throws.
- Updates the comment of rethrow()
- Throw ERR_INVALID_ARG_VALUE for null checks
PR-URL: https://github.com/nodejs/node/pull/18308
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
This helps to show the cause of errors in the CI.
PR-URL: https://github.com/nodejs/node/pull/18507
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
This test was added over six years ago, and the behavior
seems to have changed since then. When the test was originally
written, common.mustCall() did not exist. The only assertion
in this test was not actually executing. Instead of an 'error'
event being emitted as expected, an 'abort' event was emitted.
PR-URL: https://github.com/nodejs/node/pull/18508
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Instead of separately calling into C++ from JS to retrieve
the Timer.now() value, pass it in as an argument.
PR-URL: https://github.com/nodejs/node/pull/18562
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
PR-URL: https://github.com/nodejs/node/pull/18562
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
The readline module wants a truthy time while using Timer.now() doesn't
necessarily guarantee that early on in the process' life. It also
doesn't actually resolve the timing issues experienced in an earlier
issue. Instead, this PR fixes the related tests and moves them back
to parallel.
Refs: https://github.com/nodejs/node/issues/14674
PR-URL: https://github.com/nodejs/node/pull/18563
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
