Node 10.0.0 fs.close is called with a second `undefined` argument.

[mattcollier]

• Version: 10.0.0
• Platform: Debian 9
• Subsystem:

The documentation for fs.close states "No arguments other than a possible exception are given to the completion callback."

When used in conjunction with API's like async.waterfall, it is important that the callback is called with exactly one argument (err) because additional arguments are passed on to the next task.

I have found that in node 10.0.0 fs.close is invoking the callback with two arguments (err, undefined) here: https://github.com/nodejs/node/blob/v10.x/lib/fs.js#L506

I initially thought the issue might be due to recent changes in the makeCallback function, but that is not the case. The oncomplete handler is being called with a second undefined argument.

I see that there have been many changes to FSReqWrap in native land. Maybe the issue is in there somewhere?

I made this JS patch as a workaround: v10.x...digitalbazaar:v10.x

tracking: digitalbazaar/bedrock#25

[tniessen]

cc @nodejs/fs

[BridgeAR]

In general, I would also only expect the callback to be called with a single argument. However, the language actually considers undefined as not existent so it should be fine.

Nevertheless, I think we should fix it and only return a single argument in the error case as we state in the documentation.

[targos]

@joyeecheung maybe it's a side-effect of moving the errors from C++?

[joyeecheung]

@targos It is introduced by #17689 actually. My guess is adding a no-argument overload for FSReqWrap::Resolve should fix it

node/src/node_file.cc

Line 478 in a1fc528

req_wrap->Resolve(Undefined(req_wrap->env()->isolate()));

but I am not sure why undefined is resolved in the first place. cc @jasnell

[joyeecheung]

Trying to fix it but I keep getting a crash caused by errno being EBADF in uv__io_poll, and it only reproduces when I run the test in parallel. The test passes when being run alone though.

(lldb) v8 bt
 * thread #1: tid = 0x0000, 0x00007fff6da11e3e libsystem_kernel.dylib`__pthread_kill + 10, stop reason = signal SIGSTOP
  * frame #0: 0x00007fff6da11e3e libsystem_kernel.dylib`__pthread_kill + 10
    frame #1: 0x00007fff6db50150 libsystem_pthread.dylib`pthread_kill + 333
    frame #2: 0x00007fff6d96e312 libsystem_c.dylib`abort + 127
    frame #3: 0x00000001019e3915 node`uv__io_poll(loop=0x0000000102d1ea60, timeout=-1) at kqueue.c:237
    frame #4: 0x00000001019c5dcf node`uv_run(loop=0x0000000102d1ea60, mode=UV_RUN_DEFAULT) at core.c:368
    frame #5: 0x000000010008b5a1 node`node::Start(isolate=0x0000000105806000, isolate_data=0x00007ffeefbfedb0, argc=2, argv=0x0000000105006890, exec_argc=0, exec_argv=0x0000000105006990) at node.cc:4504
    frame #6: 0x00000001000865e9 node`node::Start(event_loop=0x0000000102d1ea60, argc=2, argv=0x0000000105006890, exec_argc=0, exec_argv=0x0000000105006990) at node.cc:4583
    frame #7: 0x0000000100085dfa node`node::Start(argc=2, argv=0x0000000105006890) at node.cc:4636
    frame #8: 0x0000000101c3d12e node`main(argc=2, argv=0x00007ffeefbff648) at node_main.cc:124
    frame #9: 0x0000000100001034 node`start + 52
(lldb) frame select 3
frame #3: 0x00000001019e3915 node`uv__io_poll(loop=0x0000000102d1ea60, timeout=-1) at kqueue.c:237
   234
   235 	    if (nfds == -1) {
   236 	      if (errno != EINTR)
-> 237 	        abort();
   238
   239 	      if (timeout == 0)
   240 	        return;
(lldb) p errno
(void *) $0 = 0x0000000000000009

See patch

diff --git a/src/node_file.cc b/src/node_file.cc
index 89c53afc5b..8aec109e1a 100644
--- a/src/node_file.cc
+++ b/src/node_file.cc
@@ -413,6 +413,14 @@ void FSReqWrap::ResolveStat(const uv_stat_t* stat) {
   Resolve(node::FillGlobalStatsArray(env(), stat));
 }

+void FSReqWrap::Resolve() {
+  Local<Value> argv[1] {
+    Null(env()->isolate())
+  };
+  MakeCallback(env()->oncomplete_string(), arraysize(argv), argv);
+}
+
+
 void FSReqWrap::Resolve(Local<Value> value) {
   Local<Value> argv[2] {
     Null(env()->isolate()),
@@ -475,7 +483,7 @@ void AfterNoArgs(uv_fs_t* req) {
   FSReqAfterScope after(req_wrap, req);

   if (after.Proceed())
-    req_wrap->Resolve(Undefined(req_wrap->env()->isolate()));
+    req_wrap->Resolve();
 }

 void AfterStat(uv_fs_t* req) {
diff --git a/src/node_file.h b/src/node_file.h
index d6c8aa443c..54de533801 100644
--- a/src/node_file.h
+++ b/src/node_file.h
@@ -62,6 +62,7 @@ class FSReqBase : public ReqWrap<uv_fs_t> {
   }

   virtual void Reject(Local<Value> reject) = 0;
+  virtual void Resolve() = 0;
   virtual void Resolve(Local<Value> value) = 0;
   virtual void ResolveStat(const uv_stat_t* stat) = 0;
   virtual void SetReturnValue(const FunctionCallbackInfo<Value>& args) = 0;
@@ -90,6 +91,7 @@ class FSReqWrap : public FSReqBase {
       : FSReqBase(env, req, AsyncWrap::PROVIDER_FSREQWRAP) { }

   void Reject(Local<Value> reject) override;
+  void Resolve() override;
   void Resolve(Local<Value> value) override;
   void ResolveStat(const uv_stat_t* stat) override;
   void SetReturnValue(const FunctionCallbackInfo<Value>& args) override;
@@ -128,6 +130,18 @@ class FSReqPromise : public FSReqBase {
     resolver->Reject(env()->context(), reject).FromJust();
   }

+  void Resolve() override {
+    finished_ = true;
+    HandleScope scope(env()->isolate());
+    InternalCallbackScope callback_scope(this);
+    Local<Value> val =
+        object()->Get(env()->context(),
+                      env()->promise_string()).ToLocalChecked();
+    Local<Promise::Resolver> resolver = val.As<Promise::Resolver>();
+    resolver->Resolve(env()->context(),
+                      Undefined(env()->isolate())).FromJust();
+  }
+
   void Resolve(Local<Value> value) override {
     finished_ = true;
     HandleScope scope(env()->isolate());

See test

'use strict';

// This tests that the fs APIs only passes error to the completion callback
// if it's the only callback argument documented.
const common = require('../common');
const assert = require('assert');
const fs = require('fs');
const tmpdir = require('../common/tmpdir');
const path = require('path');

tmpdir.refresh();

const nonexistent = path.join(tmpdir.path, 'noexistent');

fs.open(nonexistent, 'r', common.mustCall(function(...args) {
  assert.strictEqual(args.length, 1);
  common.expectsError({
    type: Error,
    code: 'ENOENT'
  })(args[0]);
}));

const file = path.join(tmpdir.path, 'file.txt');
const content = Buffer.from('test');

function onOpen(...args) {
  assert.deepStrictEqual(args[0], null);
  assert.strictEqual(args.length, 2);
  const fd = args[1];
  assert.strictEqual(typeof fd, 'number');
  fs.write(fd, content, common.mustCall(function(...args) {
    assert.deepStrictEqual(args, [null, content.length, content]);
    fs.close(args[1], common.mustCall(function(...args) {
      assert.deepStrictEqual(fs.readFileSync(file), content);
      assert.deepStrictEqual(args, [null]);
    }));
  }));
}

fs.open(file, 'w+', common.mustCall(onOpen));

[joyeecheung]

Assign to myself so I don't forget, although I am kind of puzzled by the EBADF since the patch only inlines the creation of Undefined for FSReqPromise and removes the undefined passed to the callbacks in AfterNoArgs (although the crash may be totally unrelated to this bug and just reveals another bug due to the way the test is written..)