Skip to content

Commit

Permalink
fs: fix FileHandle::ClosePromise to return persisted Promise
Browse files Browse the repository at this point in the history
Makes the FileHandle::ClosePromise() idempotent, always returning
the same Promise after it has already been successfully called
once. Avoids the possibility of accidental double close events.

Signed-off-by: James M Snell <[email protected]>

PR-URL: #39331
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
  • Loading branch information
jasnell authored and BethGriggs committed Jul 29, 2021
1 parent 26ada49 commit 8b58e57
Show file tree
Hide file tree
Showing 2 changed files with 46 additions and 33 deletions.
73 changes: 40 additions & 33 deletions src/node_file.cc
Original file line number Diff line number Diff line change
Expand Up @@ -345,44 +345,51 @@ MaybeLocal<Promise> FileHandle::ClosePromise() {
Isolate* isolate = env()->isolate();
EscapableHandleScope scope(isolate);
Local<Context> context = env()->context();

Local<Value> close_resolver =
object()->GetInternalField(FileHandle::kClosingPromiseSlot);
if (!close_resolver.IsEmpty() && !close_resolver->IsUndefined()) {
CHECK(close_resolver->IsPromise());
return close_resolver.As<Promise>();
}

CHECK(!closed_);
CHECK(!closing_);
CHECK(!reading_);

auto maybe_resolver = Promise::Resolver::New(context);
CHECK(!maybe_resolver.IsEmpty());
Local<Promise::Resolver> resolver = maybe_resolver.ToLocalChecked();
Local<Promise> promise = resolver.As<Promise>();
CHECK(!reading_);
if (!closed_ && !closing_) {
closing_ = true;
Local<Object> close_req_obj;
if (!env()
->fdclose_constructor_template()
->NewInstance(env()->context())
.ToLocal(&close_req_obj)) {
return MaybeLocal<Promise>();
}
CloseReq* req = new CloseReq(env(), close_req_obj, promise, object());
auto AfterClose = uv_fs_callback_t{[](uv_fs_t* req) {
std::unique_ptr<CloseReq> close(CloseReq::from_req(req));
CHECK_NOT_NULL(close);
close->file_handle()->AfterClose();
Isolate* isolate = close->env()->isolate();
if (req->result < 0) {
HandleScope handle_scope(isolate);
close->Reject(
UVException(isolate, static_cast<int>(req->result), "close"));
} else {
close->Resolve();
}
}};
int ret = req->Dispatch(uv_fs_close, fd_, AfterClose);
if (ret < 0) {
req->Reject(UVException(isolate, ret, "close"));
delete req;

Local<Object> close_req_obj;
if (!env()->fdclose_constructor_template()
->NewInstance(env()->context()).ToLocal(&close_req_obj)) {
return MaybeLocal<Promise>();
}
closing_ = true;
object()->SetInternalField(FileHandle::kClosingPromiseSlot, promise);

CloseReq* req = new CloseReq(env(), close_req_obj, promise, object());
auto AfterClose = uv_fs_callback_t{[](uv_fs_t* req) {
std::unique_ptr<CloseReq> close(CloseReq::from_req(req));
CHECK_NOT_NULL(close);
close->file_handle()->AfterClose();
Isolate* isolate = close->env()->isolate();
if (req->result < 0) {
HandleScope handle_scope(isolate);
close->Reject(
UVException(isolate, static_cast<int>(req->result), "close"));
} else {
close->Resolve();
}
} else {
// Already closed. Just reject the promise immediately
resolver->Reject(context, UVException(isolate, UV_EBADF, "close"))
.Check();
}};
int ret = req->Dispatch(uv_fs_close, fd_, AfterClose);
if (ret < 0) {
req->Reject(UVException(isolate, ret, "close"));
delete req;
}

return scope.Escape(promise);
}

Expand Down Expand Up @@ -2538,7 +2545,7 @@ void Initialize(Local<Object> target,
env->SetProtoMethod(fd, "close", FileHandle::Close);
env->SetProtoMethod(fd, "releaseFD", FileHandle::ReleaseFD);
Local<ObjectTemplate> fdt = fd->InstanceTemplate();
fdt->SetInternalFieldCount(StreamBase::kInternalFieldCount);
fdt->SetInternalFieldCount(FileHandle::kInternalFieldCount);
StreamBase::AddMethods(env, fd);
env->SetConstructorFunction(target, "FileHandle", fd);
env->set_fd_constructor_template(fdt);
Expand Down
6 changes: 6 additions & 0 deletions src/node_file.h
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,12 @@ class FileHandleReadWrap final : public ReqWrap<uv_fs_t> {
// the object is garbage collected
class FileHandle final : public AsyncWrap, public StreamBase {
public:
enum InternalFields {
kFileHandleBaseField = StreamBase::kInternalFieldCount,
kClosingPromiseSlot,
kInternalFieldCount
};

static FileHandle* New(BindingData* binding_data,
int fd,
v8::Local<v8::Object> obj = v8::Local<v8::Object>());
Expand Down

0 comments on commit 8b58e57

Please sign in to comment.