fs: Add file descriptor support to *File() funcs #8522

jwueller · 2014-10-06T18:26:32Z

These changes affect the following functions and their synchronous counterparts:

fs.readFile()
fs.writeFile()
fs.appendFile()

If the first parameter is a uint >= 0, it is treated as a file descriptor. In all other cases, the original implementation is used to ensure backwards compatibility. File descriptor ownership is never taken from the user.

The documentation was adjusted to reflect these API changes. A note was added to make the user aware of file descriptor ownership and the conditions under which a file descriptor can be used by each of these functions.

Tests were extended to test for file descriptor parameters under the conditions noted in the relevant documentation.

jwueller · 2014-10-06T18:29:37Z

as requested by @rlidwka and @chrisdickinson in #8471

trevnorris · 2014-10-07T21:53:14Z

doc/api/fs.markdown


-If the `encoding` option is specified then this function returns a
-string. Otherwise it returns a buffer.


Why remove these two lines?

A similar note is part of the fs.readFile(file[, options], callback) documentation, which is referenced by fs.readFileSync(file[, options]). No other synchronous method has any additional notes aside from referencing the asynchronous version. I tried to make the documentation more uniform, despite the note being accurate. Less duplication, less potential for errors.

Would you prefer me putting it back in?

Go ahead and leave it in. Most other functions *Sync() functions can refer the user to a man page. This is a special case sense the return value is not always the same (i.e. Buffer or String).

jwueller · 2014-10-09T22:23:51Z

Thanks for the review, @trevnorris! I made the proposed changes and updated the branch. Let me know if there is anything else you would like changed.

jwueller · 2014-10-23T20:25:35Z

@trevnorris I am not sure whether you got notified (sorry to disturb if you are just busy). Is there anything else I can do to improve this?

trevnorris · 2014-10-25T07:10:10Z

@jwueller No worries about bugging me. It's helpful to be pinged. :)

So far everything is looking good.

/cc @indutny @tjfontaine Either of you have a problem with this change?

jwueller · 2014-11-03T18:21:43Z

Since @trevnorris said it's helpful, I'll shamelessly ping again ;) @indutny @tjfontaine

bnoordhuis · 2014-11-05T18:29:24Z

lib/fs.js

@@ -113,6 +113,10 @@ function nullCheck(path, callback) {
  return true;
 }

+function isFd(path) {
+  return (path >>> 0) === path && path > 0;


Maybe just typeof(path) === 'number' && fd >= 0? Zero is a valid file descriptor.

That allows path to be NaN. The point of the uint32 conversion check is that detects that, and is less prone to deoptimization than typeof.

Okay, but the point about zero being a valid file descriptor still stands. :-)

True.

@jwueller Please change it to (path >>> 0) === path && path >= 0.

Yes, I already did that and the other suggestions locally. It will get pushed as soon as I am back at my workstation (probably Monday).

jwueller · 2014-11-12T20:45:54Z

@bnoordhuis @trevnorris There you go; finally got around to pushing my changes.

trevnorris · 2014-11-17T19:48:57Z

lib/fs.js

+    } else {
+      fs.close(fd, function() {
+        callback(er);
+      });


Potential problem is that the fs.close() itself could error. Not sure what the best solution would be in that case, but seems like a potential fd leak if we ignore any errors coming from the fs.close() cb.

@bnoordhuis Thoughts on how to deal w/ this?

@trevnorris Can one even recover from a file failing to close? It appears that the state of a file descriptor is unspecified if it fails:

If close() is interrupted by a signal that is to be caught, it shall return -1 with errno set to [EINTR] and the state of fildes is unspecified. If an I/O error occurred while reading from or writing to the file system during close(), it may return -1 with errno set to [EIO]; if this error is returned, the state of fildes is unspecified.

Assuming fs.close() calls close(2), it cannot fail to release the fd. The error returns from close are purely advisory. Linux documents only EBADF (which isn't a leak, obviously), EIO (which is a delayed error from a prev i/o operation, interesting, but not a leak, and only NFS, I think), and EINTR, of which the following note applies:

Note that the return value should only be used for diagnostics. In particular close() should not be retried after an EINTR since this may cause a reused descriptor from another thread to be closed.

man 2 close

@sam-github According to the fs documentation, we are dealing with close(2). I haven't looked at the actual code, though.

Hm, looks like the open group might've had to align to solaris: https://docs.oracle.com/cd/E23824_01/html/821-1463/close-2.html#scrolltoc which allows the fd to be left in an unspecified condition. Wtf? Anyhow, no recover possible, indeed.

Wow. WTF are you supposed to do w/ a fd in an "unspecified condition"?

Either way, should we report this to the user, and if so then how?

Reporting it would probably not be very helpful, since we are already in an error state at this point. The first failure is likely more interesting to the user, as he might be able to do something about it. There is nothing a user can do about close(2) failing. The second error may even be a subsequent failure of the first. I propose we follow the usual paradigm and report the "topmost" error to the user.

I second @jwueller's suggestion. It's what libuv does.

Sounds good.

trevnorris · 2014-11-17T19:54:09Z

@jwueller Looking good. Few more comments.

jwueller · 2014-11-23T14:56:07Z

@trevnorris branch updated, one remaining question

misterdjules · 2015-02-18T20:17:14Z

doc/api/fs.markdown

@@ -527,7 +535,10 @@ Example:
      console.log('The "data to append" was appended to file!');
    });

-## fs.appendFileSync(filename, data, [options])
+Note that any specified file descriptor needs to be openend for appending.


Typo: openend should be opened.

misterdjules · 2015-02-18T20:59:16Z

@jwueller @trevnorris What do we need to do to move this PR forward?

trevnorris · 2015-02-18T21:59:40Z

@misterdjules probably a rebase. Last I looked it was fine. Once it's rebased I'll take another glance and sign off.

These changes affect the following functions and their synchronous counterparts: * fs.readFile() * fs.writeFile() * fs.appendFile() If the first parameter is a uint >= 0, it is treated as a file descriptor. In all other cases, the original implementation is used to ensure backwards compatibility. File descriptor ownership is never taken from the user. The documentation was adjusted to reflect these API changes. A note was added to make the user aware of file descriptor ownership and the conditions under which a file descriptor can be used by each of these functions. Tests were extended to test for file descriptor parameters under the conditions noted in the relevant documentation.

jwueller · 2015-05-10T12:49:19Z

@trevnorris Oh my, I just noticed that this PR is still open and pushed a rebase. Do we need anything else?

jwueller · 2015-06-06T18:07:09Z

@trevnorris @misterdjules Should I add a test for fs.writeFileSync(fd, ...) in this PR, or do you want that one separately?

jasnell · 2015-08-13T20:58:02Z

@jwueller ... well, thank you for being so patient on this one. At this point, given the API change, this is something that would need to be closed here then re-targeted at master with a new PR on http://github.com/nodejs/node.

jwueller · 2015-08-27T22:59:23Z

@jasnell I'll do that, thank you for the update.

Nodejs-Jenkins added pr master labels Oct 6, 2014

trevnorris reviewed Oct 7, 2014
View reviewed changes

bnoordhuis reviewed Nov 5, 2014
View reviewed changes

trevnorris reviewed Nov 17, 2014
View reviewed changes

misterdjules reviewed Feb 18, 2015
View reviewed changes

jasnell added the api-change label Aug 15, 2015

jasnell closed this Aug 27, 2015

sam-github mentioned this pull request Jan 17, 2017

doc: clarify the behavior of readFile with fd nodejs/node#10853

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fs: Add file descriptor support to *File() funcs #8522

fs: Add file descriptor support to *File() funcs #8522

jwueller commented Oct 6, 2014

jwueller commented Oct 6, 2014

trevnorris Oct 7, 2014

jwueller Oct 9, 2014

trevnorris Oct 9, 2014

jwueller commented Oct 9, 2014

jwueller commented Oct 23, 2014

trevnorris commented Oct 25, 2014

jwueller commented Nov 3, 2014

bnoordhuis Nov 5, 2014

trevnorris Nov 6, 2014

bnoordhuis Nov 6, 2014

trevnorris Nov 8, 2014

jwueller Nov 8, 2014

jwueller commented Nov 12, 2014

trevnorris Nov 17, 2014

jwueller Nov 17, 2014

sam-github Nov 17, 2014

jwueller Nov 17, 2014

sam-github Nov 17, 2014

trevnorris Nov 17, 2014

jwueller Nov 17, 2014

bnoordhuis Nov 17, 2014

trevnorris Nov 18, 2014

trevnorris commented Nov 17, 2014

jwueller commented Nov 23, 2014

misterdjules Feb 18, 2015

misterdjules commented Feb 18, 2015

trevnorris commented Feb 18, 2015

jwueller commented May 10, 2015

jwueller commented Jun 6, 2015

jasnell commented Aug 13, 2015

jwueller commented Aug 27, 2015


		If the `encoding` option is specified then this function returns a
		string. Otherwise it returns a buffer.

fs: Add file descriptor support to *File() funcs #8522

fs: Add file descriptor support to *File() funcs #8522

Conversation

jwueller commented Oct 6, 2014

jwueller commented Oct 6, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwueller commented Oct 9, 2014

jwueller commented Oct 23, 2014

trevnorris commented Oct 25, 2014

jwueller commented Nov 3, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwueller commented Nov 12, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trevnorris commented Nov 17, 2014

jwueller commented Nov 23, 2014

Choose a reason for hiding this comment

misterdjules commented Feb 18, 2015

trevnorris commented Feb 18, 2015

jwueller commented May 10, 2015

jwueller commented Jun 6, 2015

jasnell commented Aug 13, 2015

jwueller commented Aug 27, 2015