Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 24 additions & 9 deletions doc/api/fs.markdown
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -460,9 +460,9 @@ The callback is given the three arguments, `(err, bytesRead, buffer)`.

Synchronous version of `fs.read`. Returns the number of `bytesRead`.

## fs.readFile(filename[, options], callback)
## fs.readFile(file[, options], callback)

* `filename` {String}
* `file` {String | Integer} filename or file descriptor
* `options` {Object}
* `encoding` {String | Null} default = `null`
* `flag` {String} default = `'r'`
Expand All @@ -480,18 +480,19 @@ contents of the file.

If no encoding is specified, then the raw buffer is returned.

Note that any specified file descriptor needs to support reading. Specified file
descriptors will not be closed automatically.

## fs.readFileSync(filename[, options])
## fs.readFileSync(file[, options])

Synchronous version of `fs.readFile`. Returns the contents of the `filename`.
Synchronous version of `fs.readFile`. Returns the contents of the `file`.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why remove these two lines?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

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

## fs.writeFile(file, data[, options], callback)

## fs.writeFile(filename, data[, options], callback)

* `filename` {String}
* `file` {String | Integer} filename or file descriptor
* `data` {String | Buffer}
* `options` {Object}
* `encoding` {String | Null} default = `'utf8'`
Expand All @@ -516,9 +517,16 @@ Example:

The synchronous version of `fs.writeFile`. Returns `undefined`.

## fs.appendFile(filename, data[, options], callback)
Any specified file descriptor needs to support writing. Specified file
descriptors will not be closed automatically.

Note that it is unsafe to use `fs.writeFile` multiple times on the same file
without waiting for the callback. For this scenario,
`fs.createWriteStream` is strongly recommended.

## fs.appendFile(file, data[, options], callback)

* `filename` {String}
* `file` {String | Integer} filename or file descriptor
* `data` {String | Buffer}
* `options` {Object}
* `encoding` {String | Null} default = `'utf8'`
Expand All @@ -540,6 +548,13 @@ Example:

The synchronous version of `fs.appendFile`. Returns `undefined`.

Note that any specified file descriptor needs to be opened for appending.
Specified file descriptors will not be closed automatically.

## fs.appendFileSync(file, data[, options])

The synchronous version of `fs.appendFile`. Returns `undefined`.

## fs.watchFile(filename[, options], listener)

Stability: 2 - Unstable. Use fs.watch instead, if possible.
Expand Down
123 changes: 91 additions & 32 deletions lib/fs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,10 @@ function nullCheck(path, callback) {
return true;
}

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

// Static method to set the stats properties on a Stats object.
fs.Stats = function(
dev,
Expand Down Expand Up @@ -263,17 +267,23 @@ fs.readFile = function(path, options, callback_) {
var buffers; // list for when size is unknown
var pos = 0;
var fd;
var isUserFd = isFd(path); // file descriptor ownership

if (isUserFd) {
readFd(null, path);
} else {
var flag = options.flag || 'r';
fs.open(path, flag, 438 /*=0666*/, readFd);
}

function readFd(err_, fd_) {
if (err_) return callback(err_);

var flag = options.flag || 'r';
fs.open(path, flag, 438 /*=0666*/, function(er, fd_) {
if (er) return callback(er);
fd = fd_;

fs.fstat(fd, function(er, st) {
if (er) {
return fs.close(fd, function() {
callback(er);
});
return error(er);
}

size = st.size;
Expand All @@ -287,14 +297,13 @@ fs.readFile = function(path, options, callback_) {
if (size > kMaxLength) {
var err = new RangeError('File size is greater than possible Buffer: ' +
'0x3FFFFFFF bytes');
return fs.close(fd, function() {
callback(err);
});

return error(err);
}
buffer = new Buffer(size);
read();
});
});
}

function read() {
if (size === 0) {
Expand All @@ -307,9 +316,7 @@ fs.readFile = function(path, options, callback_) {

function afterRead(er, bytesRead) {
if (er) {
return fs.close(fd, function(er2) {
return callback(er);
});
return error(er);
}

if (bytesRead === 0) {
Expand All @@ -328,7 +335,12 @@ fs.readFile = function(path, options, callback_) {
}

function close() {
fs.close(fd, function(er) {
if (isUserFd)
afterClose(null);
else
fs.close(fd, afterClose);

function afterClose(er) {
if (size === 0) {
// collected the data into the buffers list.
buffer = Buffer.concat(buffers, pos);
Expand All @@ -338,7 +350,17 @@ fs.readFile = function(path, options, callback_) {

if (encoding) buffer = buffer.toString(encoding);
return callback(er, buffer);
});
}
}

function error(er) {
if (isUserFd) {
callback(er);
} else {
fs.close(fd, function() {
callback(er);
});
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good.

}
}
};

Expand All @@ -355,15 +377,16 @@ fs.readFileSync = function(path, options) {
assertEncoding(encoding);

var flag = options.flag || 'r';
var fd = fs.openSync(path, flag, 438 /*=0666*/);
var isUserFd = isFd(path); // file descriptor ownership
var fd = isUserFd ? path : fs.openSync(path, flag, 438 /*=0666*/);

var size;
var threw = true;
try {
size = fs.fstatSync(fd).size;
threw = false;
} finally {
if (threw) fs.closeSync(fd);
if (threw && !isUserFd) fs.closeSync(fd);
}

var pos = 0;
Expand All @@ -378,7 +401,7 @@ fs.readFileSync = function(path, options) {
buffer = new Buffer(size);
threw = false;
} finally {
if (threw) fs.closeSync(fd);
if (threw && !isUserFd) fs.closeSync(fd);
}
}

Expand All @@ -399,14 +422,15 @@ fs.readFileSync = function(path, options) {
}
threw = false;
} finally {
if (threw) fs.closeSync(fd);
if (threw && !isUserFd) fs.closeSync(fd);
}

pos += bytesRead;
done = (bytesRead === 0) || (size !== 0 && pos >= size);
}

fs.closeSync(fd);
if (!isUserFd)
fs.closeSync(fd);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for braces.


if (size === 0) {
// data was collected into the buffers list.
Expand Down Expand Up @@ -1048,12 +1072,10 @@ function writeAll(fd, buffer, offset, length, position, callback) {
// write(fd, buffer, offset, length, position, callback)
fs.write(fd, buffer, offset, length, position, function(writeErr, written) {
if (writeErr) {
fs.close(fd, function() {
if (callback) callback(writeErr);
});
callback(writeErr);
} else {
if (written === length) {
fs.close(fd, callback);
callback(null);
} else {
offset += written;
length -= written;
Expand All @@ -1078,16 +1100,41 @@ fs.writeFile = function(path, data, options, callback) {
assertEncoding(options.encoding);

var flag = options.flag || 'w';
fs.open(path, flag, options.mode, function(openErr, fd) {
var fd;
var isUserFd; // file descriptor ownership

if (isFd(path)) {
fd = path;
isUserFd = true;
writeFd();
return;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: save a level of indentation and just return at the end of the first if () then remove the else.


fs.open(path, flag, options.mode, function(openErr, fd_) {
if (openErr) {
if (callback) callback(openErr);
callback(openErr);
} else {
var buffer = util.isBuffer(data) ? data : new Buffer('' + data,
options.encoding || 'utf8');
var position = /a/.test(flag) ? null : 0;
writeAll(fd, buffer, 0, buffer.length, position, callback);
fd = fd_;
isUserFd = false;
writeFd();
}
});

function writeFd() {
var buffer = util.isBuffer(data) ? data : new Buffer('' + data,
options.encoding || 'utf8');
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you get the new Buffer('' + data, options.encoding || 'utf8') from somewhere else?

Just curious because doing '' + data will forcefully convert the a buffer using UTF8 encoding. So if it is a buffer you should use data.toString(options.encoding || 'utf8') and if it is elsewhere then it needs to be fixed.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@trevnorris I did. It's from line 981 of the original version. Should I fix it in this PR or move it into a new one?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

go ahead and fix in this PR. I'll make a commit to fix it elsewhere. Thanks for letting me know.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if data does not have a toString() method that accepts an encoding? I am not exactly sure what your solution would look like.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@trevnorris Pinging again; did you see my previous comment on this?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

toString() will only be used when util.isBuffer(data) === true. Thus toString() is always expected.

var position = /a/.test(flag) ? null : 0;

writeAll(fd, buffer, 0, buffer.length, position, function(writeErr) {
if (isUserFd) {
callback(writeErr);
} else {
fs.close(fd, function() {
callback(writeErr);
});
}
});
}
};

fs.writeFileSync = function(path, data, options) {
Expand All @@ -1102,7 +1149,9 @@ fs.writeFileSync = function(path, data, options) {
assertEncoding(options.encoding);

var flag = options.flag || 'w';
var fd = fs.openSync(path, flag, options.mode);
var isUserFd = isFd(path);
var fd = isUserFd ? path : fs.openSync(path, flag, options.mode);

if (!util.isBuffer(data)) {
data = new Buffer('' + data, options.encoding || 'utf8');
}
Expand All @@ -1115,7 +1164,7 @@ fs.writeFileSync = function(path, data, options) {
position += written;
}
} finally {
fs.closeSync(fd);
if (!isUserFd) fs.closeSync(fd);
}
};

Expand All @@ -1132,6 +1181,11 @@ fs.appendFile = function(path, data, options, callback_) {

if (!options.flag)
options = util._extend({ flag: 'a' }, options);

// force append behavior when using a supplied file descriptor
if (isFd(path))
options.flag = 'a';

fs.writeFile(path, data, options, callback);
};

Expand All @@ -1143,9 +1197,14 @@ fs.appendFileSync = function(path, data, options) {
} else if (!util.isObject(options)) {
throw new TypeError('Bad arguments');
}

if (!options.flag)
options = util._extend({ flag: 'a' }, options);

// force append behavior when using a supplied file descriptor
if (isFd(path))
options.flag = 'a';

fs.writeFileSync(path, data, options);
};

Expand Down
15 changes: 15 additions & 0 deletions test/simple/test-fs-append-file-sync.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,20 @@ var fileData4 = fs.readFileSync(filename4);
assert.equal(Buffer.byteLength('' + num) + currentFileData.length,
fileData4.length);

// test that appendFile accepts file descriptors
var filename5 = join(common.tmpDir, 'append-sync5.txt');
fs.writeFileSync(filename5, currentFileData);

common.error('appending to ' + filename5);
var filename5fd = fs.openSync(filename5, 'a+');
fs.appendFileSync(filename5fd, data);
fs.closeSync(filename5fd);

var fileData5 = fs.readFileSync(filename5);

assert.equal(Buffer.byteLength(data) + currentFileData.length,
fileData5.length);

//exit logic for cleanup

process.on('exit', function() {
Expand All @@ -99,4 +113,5 @@ process.on('exit', function() {
fs.unlinkSync(filename2);
fs.unlinkSync(filename3);
fs.unlinkSync(filename4);
fs.unlinkSync(filename5);
});
Loading