fs: add FileHandle object fd wrapper

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: #18297 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
nodejs · Feb 6, 2018 · 85b37db · 85b37db
1 parent 7154bc0
commit 85b37db
Show file tree

Hide file tree

Showing 4 changed files with 277 additions and 4 deletions.
diff --git a/src/async_wrap.h b/src/async_wrap.h
@@ -36,6 +36,8 @@ namespace node {
 #define NODE_ASYNC_NON_CRYPTO_PROVIDER_TYPES(V)                               \
   V(NONE)                                                                     \
   V(DNSCHANNEL)                                                               \
+  V(FILEHANDLE)                                                               \
+  V(FILEHANDLECLOSEREQ)                                                       \
   V(FSEVENTWRAP)                                                              \
   V(FSREQWRAP)                                                                \
   V(FSREQPROMISE)                                                             \

diff --git a/src/env.h b/src/env.h
@@ -283,6 +283,8 @@ class ModuleWrap;
   V(buffer_prototype_object, v8::Object)                                      \
   V(context, v8::Context)                                                     \
   V(domain_callback, v8::Function)                                            \
+  V(fd_constructor_template, v8::ObjectTemplate)                              \
+  V(fdclose_constructor_template, v8::ObjectTemplate)                         \
   V(host_import_module_dynamically_callback, v8::Function)                    \
   V(host_initialize_import_meta_object_callback, v8::Function)                \
   V(http2ping_constructor_template, v8::ObjectTemplate)                       \

diff --git a/src/node_file.cc b/src/node_file.cc
@@ -97,6 +97,7 @@ using v8::Local;
 using v8::MaybeLocal;
 using v8::Number;
 using v8::Object;
+using v8::ObjectTemplate;
 using v8::Promise;
 using v8::String;
 using v8::Undefined;
@@ -108,6 +109,150 @@ using v8::Value;
 
 #define GET_OFFSET(a) ((a)->IsNumber() ? (a)->IntegerValue() : -1)
 
+// The FileHandle object wraps a file descriptor and will close it on garbage
+// collection if necessary. If that happens, a process warning will be
+// emitted (or a fatal exception will occur if the fd cannot be closed.)
+FileHandle::FileHandle(Environment* env, int fd)
+    : AsyncWrap(env,
+                env->fd_constructor_template()
+                    ->NewInstance(env->context()).ToLocalChecked(),
+                AsyncWrap::PROVIDER_FILEHANDLE), fd_(fd) {
+  MakeWeak<FileHandle>(this);
+  v8::PropertyAttribute attr =
+      static_cast<v8::PropertyAttribute>(v8::ReadOnly | v8::DontDelete);
+  object()->DefineOwnProperty(env->context(),
+                              FIXED_ONE_BYTE_STRING(env->isolate(), "fd"),
+                              Integer::New(env->isolate(), fd),
+                              attr).FromJust();
+}
+
+FileHandle::~FileHandle() {
+  CHECK(!closing_);  // We should not be deleting while explicitly closing!
+  Close();           // Close synchronously and emit warning
+  CHECK(closed_);    // We have to be closed at the point
+  CHECK(persistent().IsEmpty());
+}
+
+
+// Close the file descriptor if it hasn't already been closed. A process
+// warning will be emitted using a SetImmediate to avoid calling back to
+// JS during GC. If closing the fd fails at this point, a fatal exception
+// will crash the process immediately.
+inline void FileHandle::Close() {
+  if (closed_) return;
+  closed_ = true;
+  uv_fs_t req;
+  int ret = uv_fs_close(env()->event_loop(), &req, fd_, nullptr);
+  uv_fs_req_cleanup(&req);
+
+  struct err_detail { int ret; int fd; };
+
+  err_detail* detail = new err_detail { ret, fd_ };
+
+  if (ret < 0) {
+    // Do not unref this
+    env()->SetImmediate([](Environment* env, void* data) {
+      char msg[70];
+      err_detail* detail = static_cast<err_detail*>(data);
+      snprintf(msg, arraysize(msg),
+              "Closing file descriptor %d on garbage collection failed",
+              detail->fd);
+      // This exception will end up being fatal for the process because
+      // it is being thrown from within the SetImmediate handler and
+      // there is no JS stack to bubble it to. In other words, tearing
+      // down the process is the only reasonable thing we can do here.
+      env->ThrowUVException(detail->ret, "close", msg);
+      delete detail;
+    }, detail);
+    return;
+  }
+
+  // If the close was successful, we still want to emit a process warning
+  // to notify that the file descriptor was gc'd. We want to be noisy about
+  // this because not explicitly closing the garbage collector is a bug.
+  env()->SetUnrefImmediate([](Environment* env, void* data) {
+    char msg[70];
+    err_detail* detail = static_cast<err_detail*>(data);
+    snprintf(msg, arraysize(msg),
+            "Closing file descriptor %d on garbage collection",
+            detail->fd);
+    delete detail;
+    ProcessEmitWarning(env, msg);
+  }, detail);
+}
+
+void FileHandle::CloseReq::Resolve() {
+  InternalCallbackScope callback_scope(this);
+  HandleScope scope(env()->isolate());
+  Local<Promise> promise = promise_.Get(env()->isolate());
+  Local<Promise::Resolver> resolver = promise.As<Promise::Resolver>();
+  resolver->Resolve(env()->context(), Undefined(env()->isolate()));
+}
+
+void FileHandle::CloseReq::Reject(Local<Value> reason) {
+  InternalCallbackScope callback_scope(this);
+  HandleScope scope(env()->isolate());
+  Local<Promise> promise = promise_.Get(env()->isolate());
+  Local<Promise::Resolver> resolver = promise.As<Promise::Resolver>();
+  resolver->Reject(env()->context(), reason);
+}
+
+FileHandle* FileHandle::CloseReq::fd() {
+  HandleScope scope(env()->isolate());
+  Local<Value> val = ref_.Get(env()->isolate());
+  Local<Object> obj = val.As<Object>();
+  return Unwrap<FileHandle>(obj);
+}
+
+// Closes this FileHandle asynchronously and returns a Promise that will be
+// resolved when the callback is invoked, or rejects with a UVException if
+// there was a problem closing the fd. This is the preferred mechanism for
+// closing the FD object even tho the object will attempt to close
+// automatically on gc.
+inline Local<Promise> FileHandle::ClosePromise() {
+  Isolate* isolate = env()->isolate();
+  HandleScope scope(isolate);
+  Local<Context> context = env()->context();
+  auto maybe_resolver = Promise::Resolver::New(context);
+  CHECK(!maybe_resolver.IsEmpty());
+  Local<Promise::Resolver> resolver = maybe_resolver.ToLocalChecked();
+  Local<Promise> promise = resolver.As<Promise>();
+  if (!closed_ && !closing_) {
+    closing_ = true;
+    CloseReq* req = new CloseReq(env(), promise, object());
+    auto AfterClose = [](uv_fs_t* req) {
+      CloseReq* close = static_cast<CloseReq*>(req->data);
+      CHECK_NE(close, nullptr);
+      close->fd()->closing_ = false;
+      Isolate* isolate = close->env()->isolate();
+      if (req->result < 0) {
+        close->Reject(UVException(isolate, req->result, "close"));
+      } else {
+        close->fd()->closed_ = true;
+        close->Resolve();
+      }
+      delete close;
+    };
+    req->Dispatched();
+    int ret = uv_fs_close(env()->event_loop(), req->req(), fd_, AfterClose);
+    if (ret < 0) {
+      req->Reject(UVException(isolate, ret, "close"));
+      delete req;
+    }
+  } else {
+    // Already closed. Just reject the promise immediately
+    resolver->Reject(context, UVException(isolate, UV_EBADF, "close"));
+  }
+  return promise;
+}
+
+void FileHandle::Close(const FunctionCallbackInfo<Value>& args) {
+  FileHandle* fd;
+  ASSIGN_OR_RETURN_UNWRAP(&fd, args.Holder());
+  args.GetReturnValue().Set(fd->ClosePromise());
+}
+
+
 void FSReqWrap::Reject(Local<Value> reject) {
   MakeCallback(env()->oncomplete_string(), 1, &reject);
 }
@@ -142,8 +287,7 @@ FSReqPromise::FSReqPromise(Environment* env, Local<Object> req)
 
   Local<ArrayBuffer> ab =
       ArrayBuffer::New(env->isolate(), statFields_,
-                       sizeof(double) * 14,
-                       v8::ArrayBufferCreationMode::kInternalized);
+                       sizeof(double) * 14);
   object()->Set(env->context(),
                 env->statfields_string(),
                 Float64Array::New(ab, 0, 14)).FromJust();
@@ -261,6 +405,16 @@ void AfterInteger(uv_fs_t* req) {
     req_wrap->Resolve(Integer::New(req_wrap->env()->isolate(), req->result));
 }
 
+void AfterOpenFileHandle(uv_fs_t* req) {
+  FSReqWrap* req_wrap = static_cast<FSReqWrap*>(req->data);
+  FSReqAfterScope after(req_wrap, req);
+
+  if (after.Proceed()) {
+    FileHandle* fd = new FileHandle(req_wrap->env(), req->result);
+    req_wrap->Resolve(fd->object());
+  }
+}
+
 void AfterStringPath(uv_fs_t* req) {
   FSReqBase* req_wrap = static_cast<FSReqBase*>(req->data);
   FSReqAfterScope after(req_wrap, req);
@@ -969,6 +1123,7 @@ static void ReadDir(const FunctionCallbackInfo<Value>& args) {
 
 static void Open(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
+  Local<Context> context = env->context();
 
   CHECK_GE(args.Length(), 3);
   CHECK(args[1]->IsInt32());
@@ -977,8 +1132,8 @@ static void Open(const FunctionCallbackInfo<Value>& args) {
   BufferValue path(env->isolate(), args[0]);
   CHECK_NE(*path, nullptr);
 
-  int flags = args[1]->Int32Value();
-  int mode = static_cast<int>(args[2]->Int32Value());
+  int flags = args[1]->Int32Value(context).ToChecked();
+  int mode = args[2]->Int32Value(context).ToChecked();
 
   if (args[3]->IsObject()) {
     CHECK_EQ(args.Length(), 4);
@@ -990,6 +1145,35 @@ static void Open(const FunctionCallbackInfo<Value>& args) {
   }
 }
 
+static void OpenFileHandle(const FunctionCallbackInfo<Value>& args) {
+  Environment* env = Environment::GetCurrent(args);
+  Local<Context> context = env->context();
+
+  CHECK_GE(args.Length(), 3);
+  CHECK(args[1]->IsInt32());
+  CHECK(args[2]->IsInt32());
+
+  BufferValue path(env->isolate(), args[0]);
+  CHECK_NE(*path, nullptr);
+
+  int flags = args[1]->Int32Value(context).ToChecked();
+  int mode = args[2]->Int32Value(context).ToChecked();
+
+  if (args[3]->IsObject()) {
+    CHECK_EQ(args.Length(), 4);
+    AsyncCall(env, args, "open", UTF8, AfterOpenFileHandle,
+              uv_fs_open, *path, flags, mode);
+  } else {
+    SYNC_CALL(open, *path, *path, flags, mode)
+    if (SYNC_RESULT < 0) {
+      args.GetReturnValue().Set(SYNC_RESULT);
+    } else {
+      HandleScope scope(env->isolate());
+      FileHandle* fd = new FileHandle(env, SYNC_RESULT);
+      args.GetReturnValue().Set(fd->object());
+    }
+  }
+}
 
 static void CopyFile(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
@@ -1391,6 +1575,7 @@ void InitFs(Local<Object> target,
   env->SetMethod(target, "access", Access);
   env->SetMethod(target, "close", Close);
   env->SetMethod(target, "open", Open);
+  env->SetMethod(target, "openFileHandle", OpenFileHandle);
   env->SetMethod(target, "read", Read);
   env->SetMethod(target, "fdatasync", Fdatasync);
   env->SetMethod(target, "fsync", Fsync);
@@ -1452,6 +1637,27 @@ void InitFs(Local<Object> target,
       FIXED_ONE_BYTE_STRING(env->isolate(), "FSReqPromise");
   fpt->SetClassName(promiseString);
   target->Set(context, promiseString, fpt->GetFunction()).FromJust();
+
+  // Create FunctionTemplate for FileHandle
+  Local<FunctionTemplate> fd = FunctionTemplate::New(env->isolate());
+  AsyncWrap::AddWrapMethods(env, fd);
+  env->SetProtoMethod(fd, "close", FileHandle::Close);
+  Local<ObjectTemplate> fdt = fd->InstanceTemplate();
+  fdt->SetInternalFieldCount(1);
+  Local<String> handleString =
+       FIXED_ONE_BYTE_STRING(env->isolate(), "FileHandle");
+  fd->SetClassName(handleString);
+  target->Set(context, handleString, fd->GetFunction()).FromJust();
+  env->set_fd_constructor_template(fdt);
+
+  // Create FunctionTemplate for FileHandle::CloseReq
+  Local<FunctionTemplate> fdclose = FunctionTemplate::New(env->isolate());
+  fdclose->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(),
+                        "FileHandleCloseReq"));
+  AsyncWrap::AddWrapMethods(env, fdclose);
+  Local<ObjectTemplate> fdcloset = fdclose->InstanceTemplate();
+  fdcloset->SetInternalFieldCount(1);
+  env->set_fdclose_constructor_template(fdcloset);
 }
 
 }  // namespace fs

diff --git a/src/node_file.h b/src/node_file.h
@@ -9,9 +9,12 @@
 namespace node {
 
 using v8::Context;
+using v8::FunctionCallbackInfo;
 using v8::HandleScope;
 using v8::Local;
 using v8::Object;
+using v8::Persistent;
+using v8::Promise;
 using v8::Undefined;
 using v8::Value;
 
@@ -112,6 +115,66 @@ class FSReqAfterScope {
   Context::Scope context_scope_;
 };
 
+// A wrapper for a file descriptor that will automatically close the fd when
+// the object is garbage collected
+class FileHandle : public AsyncWrap {
+ public:
+  FileHandle(Environment* env, int fd);
+  virtual ~FileHandle();
+
+  int fd() const { return fd_; }
+  size_t self_size() const override { return sizeof(*this); }
+
+  // Will asynchronously close the FD and return a Promise that will
+  // be resolved once closing is complete.
+  static void Close(const FunctionCallbackInfo<Value>& args);
+
+ private:
+  // Synchronous close that emits a warning
+  inline void Close();
+
+  class CloseReq : public ReqWrap<uv_fs_t> {
+   public:
+    CloseReq(Environment* env,
+             Local<Promise> promise,
+             Local<Value> ref)
+        : ReqWrap(env,
+                  env->fdclose_constructor_template()
+                      ->NewInstance(env->context()).ToLocalChecked(),
+                  AsyncWrap::PROVIDER_FILEHANDLECLOSEREQ) {
+      Wrap(object(), this);
+      promise_.Reset(env->isolate(), promise);
+      ref_.Reset(env->isolate(), ref);
+    }
+    ~CloseReq() {
+      uv_fs_req_cleanup(req());
+      promise_.Empty();
+      ref_.Empty();
+    }
+
+    FileHandle* fd();
+
+    size_t self_size() const override { return sizeof(*this); }
+
+    void Resolve();
+
+    void Reject(Local<Value> reason);
+
+   private:
+    Persistent<Promise> promise_;
+    Persistent<Value> ref_;
+  };
+
+  // Asynchronous close
+  inline Local<Promise> ClosePromise();
+
+  int fd_;
+  bool closing_ = false;
+  bool closed_ = false;
+
+  DISALLOW_COPY_AND_ASSIGN(FileHandle);
+};
+
 }  // namespace fs
 
 }  // namespace node