- Instead of storing a pointer whose type refers to the specific
subclass of `BaseObject`, just store a `BaseObject*` directly.
This means in particular that one can cast to classes along
the way of the inheritance chain without issues, and that
`BaseObject*` no longer needs to be the first superclass
in the case of multiple inheritance.
In particular, this renders hack-y solutions to this problem (like
ddc19be6de) obsolete and addresses
a `TODO` comment of mine.
- Move wrapping/unwrapping methods to the `BaseObject` class.
We use these almost exclusively for `BaseObject`s, and I hope
that this gives a better idea of how (and for what) these are used
in our code.
- Perform initialization/deinitialization of the internal field
in the `BaseObject*` constructor/destructor. This makes the code
a bit more obviously correct, avoids explicit calls for this
in subclass constructors, and in particular allows us to avoid
crash situations when we previously called `ClearWrap()`
during GC.
This also means that we enforce that the object passed to the
`BaseObject` constructor needs to have an internal field.
This is the only reason for the test change.
- Change the signature of `MakeWeak()` to not require a pointer
argument. Previously, this would always have been the same
as `this`, and no other value made sense. Also, the parameter
was something that I personally found somewhat confusing
when becoming familiar with Node’s code.
- Add a `TODO` comment that motivates switching to real inheritance
for the JS types we expose from the native side. This patch
brings us a lot closer to being able to do that.
- Some less significant drive-by cleanup.
Since we *effectively* already store the `BaseObject*` pointer
anyway since ddc19be6de, I do not
think that this is going to have any impact on diagnostic tooling.
Fixes: https://github.com/nodejs/node/issues/18897
PR-URL: https://github.com/nodejs/node/pull/20455
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
- Reduce reference to the global `statValues` by returning
the changed stats array from the synchronous methods. Having
a local returned value also makes the future integration
of BigInt easier.
- Also returns the filled array from node::FillGlobalStatsArray
and node::FillStatsArray in the C++ side.
PR-URL: https://github.com/nodejs/node/pull/20167
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Gus Caplan <me@gus.host>
- Pass kFsStatsFieldsLength between JS and C++ instead of using the
magic number 14
- Pass the global stats array to the completion callback of
asynchronous FSReqWrap similar to how the stats arrays are passed
to the FSReqPromise resolvers
- Abstract the stats converter and take an offset to compute the
old stats in fs.watchFile
- Use templates in node::FillStatsArray and FSReqPromise in preparation
for BigInt intergration
- Put the global stat array filler in node_internals.h because it is
shared by node_file.cc and node_stat_watcher.cc
PR-URL: https://github.com/nodejs/node/pull/19714
Reviewed-By: James M Snell <jasnell@gmail.com>
In the async case, if the buffer was copied instead of being moved
then the buf will not get deleted after the request is done.
This was introduced when the FSReqWrap:: Ownership was
removed in 4b9ba9b, and ReleaseEarly was no longer called upon
destruction of FSReqWrap.
Create a custom Init function so we can use the MaybeStackBuffer in
the FSReqBase to copy the string in the async case. The data will
then get destructed when the FSReqBase is destructed.
Fixes: https://github.com/nodejs/node/issues/19356
PR-URL: https://github.com/nodejs/node/pull/19357
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
This enables accessing files using a more standard pattern.
Once some more refactoring has been performed on the other existing
`StreamBase` streams, this could also be used to implement `fs`
streams in a more standard manner.
PR-URL: https://github.com/nodejs/node/pull/18936
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
When the async uv_fs_* call errors out synchronously in AsyncDestCall,
the after callbacks (e.g. AfterNoArgs) would delete the req_wrap
in FSReqAfterScope, and AsyncDestCall would set those req_wrap to
nullptr afterwards. But when it returns to the top-layer bindings,
the bindings all call `req_wrap->SetReturnValue()` again without
checking if `req_wrap` is nullptr, causing a segfault.
This has not been caught in any of the tests because we usually do a
lot of argument checking in the JS layer before invoking the uv_fs_*
functions, so it's rare to get a synchronous error from them.
Currently we never need the binding to return the wrap to JS layer,
so we can just call `req_wrap->SetReturnValue()` to return undefined
for normal FSReqWrap and the promise for FSReqPromise in AsyncDestCall
instead of doing this in the top-level bindings.
PR-URL: https://github.com/nodejs/node/pull/18811
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
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>
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>
The `node::fs::FileHandle` object wraps a file descriptor
and will close it on garbage collection along with a
process warning. The intent is to prevent (as much as
possible) file descriptors from being leaked if the user
does not close them explicitly.
PR-URL: https://github.com/nodejs/node/pull/18297
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Joyee Cheung