fs: add v8 fast api to closeSync #53627
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nodejs-github-bot
added
c++
anonrig
force-pushed
the
add-fast-api-for-close
branch
from
3d363dc to
734d1ba
Compare
H4ad
reviewed
Jun 28, 2024
H4ad
reviewed
Jun 28, 2024
H4ad
reviewed
Jun 28, 2024
anonrig
force-pushed
the
add-fast-api-for-close
branch
from
734d1ba to
8354379
Compare
tniessen
reviewed
Jun 28, 2024
H4ad
reviewed
Jun 28, 2024
H4ad
approved these changes
Jun 28, 2024
anonrig
added
author ready
Qard
approved these changes
Jul 2, 2024
github-actions
bot
removed
the
request-ci
jasnell
approved these changes
Jul 4, 2024
joyeecheung
reviewed
Jul 11, 2024
src/node_file.cc
Outdated
| options.fallback = true; | ||
| } else { | ||
| // Only remove unmanaged fds if the close was successful. | ||
| env->RemoveUnmanagedFd(fd); |
There was a problem hiding this comment.
This is unsafe, RemoveUnmanagedFd() can call ProcessEmitWarning() which calls back into JS process.emitWarning() and violates the fast API protocol. Although since it's just a warning, you could probably just add a parameter to RemoveUnmanagedFd to make it invoke ProcessEmitWarning() later in a native Immediate, and scheduling a native immediate is safe.
There was a problem hiding this comment.
If I understood you correctly, inside RemoveUnmanagedFd you want me to conditionally call the following right?
SetImmediateThreadsafe([](Environment* env) {
ProcessEmitWarning(
this, "File descriptor %d closed but not opened in unmanaged mode", fd);
});
If not, what is the correct way to schedule a native immediate?
There was a problem hiding this comment.
I think I got it. Can you review it again? @joyeecheung
anonrig
force-pushed
the
add-fast-api-for-close
branch
from
8354379 to
7b573d1
Compare
|
cc @nodejs/cpp-reviewers |
jasnell
approved these changes
Jul 15, 2024
| @@ -1841,12 +1841,23 @@ void Environment::AddUnmanagedFd(int fd) { | |||
| } | |||
| } | |||
|
|
|||
| void Environment::RemoveUnmanagedFd(int fd) { | |||
| void Environment::RemoveUnmanagedFd(int fd, bool schedule_native_immediate) { | |||
There was a problem hiding this comment.
Not: generally not a fan of bool arguments like this. I prefer enums that allow the call sites to be clearer about what the argument is as opposed to just true or false.
anonrig
added
the
commit-queue
nodejs-github-bot
removed
the
commit-queue
|
Landed in ed6f45b |
aduh95
pushed a commit
that referenced
this pull request
Jul 16, 2024
PR-URL: #53627 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]> Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: James M Snell <[email protected]>
aduh95
pushed a commit
that referenced
this pull request
Jul 16, 2024
PR-URL: #53627 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]> Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: James M Snell <[email protected]>
Merged
1 task
ehsankhfr
pushed a commit
to ehsankhfr/node
that referenced
this pull request
Jul 18, 2024
PR-URL: nodejs#53627 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]> Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: James M Snell <[email protected]>
richardlau
added
dont-land-on-v18.x
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds V8 Fast API to
fs.closeSync(fd)implementationcc @nodejs/performance @nodejs/fs @nodejs/cpp-reviewers