Skip to content

Commit

Permalink
src: add option to track unmanaged file descriptors
Browse files Browse the repository at this point in the history
Add the ability to track “raw” file descriptors, i.e. integers returned
by `fs.open()`, and close them on `Environment` shutdown, to match the
behavior for all other resource types (which are also closed on
shutdown).

PR-URL: #34303
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
  • Loading branch information
addaleax authored and cjihrig committed Jul 22, 2020
1 parent 390c823 commit c10dbd6
Show file tree
Hide file tree
Showing 6 changed files with 48 additions and 1 deletion.
4 changes: 4 additions & 0 deletions src/env-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -817,6 +817,10 @@ inline bool Environment::owns_inspector() const {
return flags_ & EnvironmentFlags::kOwnsInspector;
}

inline bool Environment::tracks_unmanaged_fds() const {
return flags_ & EnvironmentFlags::kTrackUnmanagedFds;
}

bool Environment::filehandle_close_warning() const {
return emit_filehandle_warning_;
}
Expand Down
24 changes: 24 additions & 0 deletions src/env.cc
Original file line number Diff line number Diff line change
Expand Up @@ -619,6 +619,12 @@ void Environment::RunCleanup() {
}
CleanupHandles();
}

for (const int fd : unmanaged_fds_) {
uv_fs_t close_req;
uv_fs_close(nullptr, &close_req, fd, nullptr);
uv_fs_req_cleanup(&close_req);
}
}

void Environment::RunAtExitCallbacks() {
Expand Down Expand Up @@ -981,6 +987,24 @@ Environment* Environment::worker_parent_env() const {
return worker_context()->env();
}

void Environment::AddUnmanagedFd(int fd) {
if (!tracks_unmanaged_fds()) return;
auto result = unmanaged_fds_.insert(fd);
if (!result.second) {
ProcessEmitWarning(
this, "File descriptor %d opened in unmanaged mode twice", fd);
}
}

void Environment::RemoveUnmanagedFd(int fd) {
if (!tracks_unmanaged_fds()) return;
size_t removed_count = unmanaged_fds_.erase(fd);
if (removed_count == 0) {
ProcessEmitWarning(
this, "File descriptor %d closed but not opened in unmanaged mode", fd);
}
}

void Environment::BuildEmbedderGraph(Isolate* isolate,
EmbedderGraph* graph,
void* data) {
Expand Down
6 changes: 6 additions & 0 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -1058,6 +1058,7 @@ class Environment : public MemoryRetainer {
inline bool should_not_register_esm_loader() const;
inline bool owns_process_state() const;
inline bool owns_inspector() const;
inline bool tracks_unmanaged_fds() const;
inline uint64_t thread_id() const;
inline worker::Worker* worker_context() const;
Environment* worker_parent_env() const;
Expand Down Expand Up @@ -1274,6 +1275,9 @@ class Environment : public MemoryRetainer {
inline std::unordered_map<char*, std::unique_ptr<v8::BackingStore>>*
released_allocated_buffers();

void AddUnmanagedFd(int fd);
void RemoveUnmanagedFd(int fd);

private:
inline void ThrowError(v8::Local<v8::Value> (*fun)(v8::Local<v8::String>),
const char* errmsg);
Expand Down Expand Up @@ -1414,6 +1418,8 @@ class Environment : public MemoryRetainer {
int64_t initial_base_object_count_ = 0;
std::atomic_bool is_stopping_ { false };

std::unordered_set<int> unmanaged_fds_;

std::function<void(Environment*, int)> process_exit_handler_ {
DefaultProcessExitHandler };

Expand Down
5 changes: 4 additions & 1 deletion src/node.h
Original file line number Diff line number Diff line change
Expand Up @@ -412,7 +412,10 @@ enum Flags : uint64_t {
// Set if Node.js should not run its own esm loader. This is needed by some
// embedders, because it's possible for the Node.js esm loader to conflict
// with another one in an embedder environment, e.g. Blink's in Chromium.
kNoRegisterESMLoader = 1 << 3
kNoRegisterESMLoader = 1 << 3,
// Set this flag to make Node.js track "raw" file descriptors, i.e. managed
// by fs.open() and fs.close(), and close them during FreeEnvironment().
kTrackUnmanagedFds = 1 << 4
};
} // namespace EnvironmentFlags

Expand Down
6 changes: 6 additions & 0 deletions src/node_file.cc
Original file line number Diff line number Diff line change
Expand Up @@ -653,6 +653,9 @@ void AfterInteger(uv_fs_t* req) {
FSReqBase* req_wrap = FSReqBase::from_req(req);
FSReqAfterScope after(req_wrap, req);

if (req->result >= 0 && req_wrap->is_plain_open())
req_wrap->env()->AddUnmanagedFd(req->result);

if (after.Proceed())
req_wrap->Resolve(Integer::New(req_wrap->env()->isolate(), req->result));
}
Expand Down Expand Up @@ -862,6 +865,7 @@ void Close(const FunctionCallbackInfo<Value>& args) {

CHECK(args[0]->IsInt32());
int fd = args[0].As<Int32>()->Value();
env->RemoveUnmanagedFd(fd);

FSReqBase* req_wrap_async = GetReqWrap(args, 1);
if (req_wrap_async != nullptr) { // close(fd, req)
Expand Down Expand Up @@ -1706,6 +1710,7 @@ static void Open(const FunctionCallbackInfo<Value>& args) {

FSReqBase* req_wrap_async = GetReqWrap(args, 3);
if (req_wrap_async != nullptr) { // open(path, flags, mode, req)
req_wrap_async->set_is_plain_open(true);
AsyncCall(env, req_wrap_async, args, "open", UTF8, AfterInteger,
uv_fs_open, *path, flags, mode);
} else { // open(path, flags, mode, undefined, ctx)
Expand All @@ -1715,6 +1720,7 @@ static void Open(const FunctionCallbackInfo<Value>& args) {
int result = SyncCall(env, args[4], &req_wrap_sync, "open",
uv_fs_open, *path, flags, mode);
FS_SYNC_TRACE_END(open);
if (result >= 0) env->AddUnmanagedFd(result);
args.GetReturnValue().Set(result);
}
}
Expand Down
4 changes: 4 additions & 0 deletions src/node_file.h
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,9 @@ class FSReqBase : public ReqWrap<uv_fs_t> {
const char* data() const { return has_data_ ? *buffer_ : nullptr; }
enum encoding encoding() const { return encoding_; }
bool use_bigint() const { return use_bigint_; }
bool is_plain_open() const { return is_plain_open_; }

void set_is_plain_open(bool value) { is_plain_open_ = value; }

FSContinuationData* continuation_data() const {
return continuation_data_.get();
Expand All @@ -113,6 +116,7 @@ class FSReqBase : public ReqWrap<uv_fs_t> {
enum encoding encoding_ = UTF8;
bool has_data_ = false;
bool use_bigint_ = false;
bool is_plain_open_ = false;
const char* syscall_ = nullptr;

BaseObjectPtr<BindingData> binding_data_;
Expand Down

0 comments on commit c10dbd6

Please sign in to comment.