fs: Add file descriptor support to *File() funcs #3163
Conversation
This one also includes a test for |
@@ -101,6 +101,10 @@ function nullCheck(path, callback) { | |||
return true; | |||
} | |||
|
|||
function isFd(path) { | |||
return (path >>> 0) === path && path >= 0; |
ChALkeR
Oct 4, 2015
Member
path >= 0
can never be false
here.
path >= 0
can never be false
here.
trevnorris
Oct 6, 2015
Contributor
To clarify, if the user passes readFile('0xff')
, would that be interpreted as a file name or file descriptor?
To clarify, if the user passes readFile('0xff')
, would that be interpreted as a file name or file descriptor?
jwueller
Oct 6, 2015
Author
Contributor
This implementation would treat '0xff'
as a file name.
This implementation would treat '0xff'
as a file name.
trevnorris
Oct 6, 2015
Contributor
Will all values where typeof === 'number'
be treated as a number, all other values will be coerced to a string?
Will all values where typeof === 'number'
be treated as a number, all other values will be coerced to a string?
jwueller
Oct 6, 2015
Author
Contributor
The original implementation does not seem to coerce at all. It errors with TypeError: path must be a string
instead.
Nevertheless, >>>
should always produce a number type, so the strict equality test can only ever succeed for typeof input === 'number'
, making everything else go down the file name branch, even if that results in another TypeError
caused by the path
argument not being a string. This behavior remains unchanged.
The original implementation does not seem to coerce at all. It errors with TypeError: path must be a string
instead.
Nevertheless, >>>
should always produce a number type, so the strict equality test can only ever succeed for typeof input === 'number'
, making everything else go down the file name branch, even if that results in another TypeError
caused by the path
argument not being a string. This behavior remains unchanged.
trevnorris
Oct 7, 2015
Contributor
I'm pondering how much coercion is allowed. e.g. should we throw on NaN
? my instinct is yes. all non uint32 values should throw. Which is exactly what you have now. :)
I'm pondering how much coercion is allowed. e.g. should we throw on NaN
? my instinct is yes. all non uint32 values should throw. Which is exactly what you have now. :)
jwueller
Oct 8, 2015
Author
Contributor
I generally avoid coercion as much as possible. It usually leads to rather unclear behavior. Throwing on NaN
sounds like it would be expected. There is no sensible default we can choose in that case.
I generally avoid coercion as much as possible. It usually leads to rather unclear behavior. Throwing on NaN
sounds like it would be expected. There is no sensible default we can choose in that case.
This is at least var num = 1;
…
fs.writeFile(num++, …); Also, the logic is not very clear:
Understandable behavior IMO would be to check if the file name is a string, and if it's not a string nor a valid file descriptor — throw an Not sure what the others think. Note: I left aside the actual feature introduced by this PR as I have no opinion on that, and covered only the API changes. |
@ChALkeR First of all, thanks for reviewing this!
@trevnorris actually proposed the Additionally, maybe @chrisdickinson could shed some light on the originally envisioned behavior as well (original issue link). |
What are potential blockers here? |
Incompatibilities with existing code would definitely be a blocker. Those should not be possible, though. Tests for the original behavior still pass and remain unchanged. Also, I'll need to do another rebase (and remove the redundant |
Ah, I overlooked that. So then there should be no backwards-incompatibility here, and this is indeed a |
@ChALkeR This looks like an additive change. All previously accepted inputs are the same, with the addition of supporting uint32's for file descriptors. I'd say this is a |
@ChALkeR @trevnorris Rebased and updated! |
0o666, | ||
req); | ||
if (context.isUserFd) { | ||
req.oncomplete.call(req, null, path); |
trevnorris
Oct 13, 2015
Contributor
Put this in a nextTick
. It's an expected async call, and must always run async.
Put this in a nextTick
. It's an expected async call, and must always run async.
stringToFlags(flag), | ||
0o666, | ||
req); | ||
} |
trevnorris
Oct 13, 2015
Contributor
return after the if then drop the else.
return after the if then drop the else.
jwueller
Oct 13, 2015
Author
Contributor
I will adjust this where appropriate.
I will adjust this where appropriate.
binding.close(this.fd, req); | ||
|
||
if (this.isUserFd) { | ||
req.oncomplete.call(req, null); |
trevnorris
Oct 13, 2015
Contributor
Clarify this with me. If an fd
is passed then it isn't automatically closed?
Clarify this with me. If an fd
is passed then it isn't automatically closed?
jwueller
Oct 13, 2015
Author
Contributor
Correct. No fd
ownership transfer takes place. We did not create the fd
, so we should not close it IMHO. Doing so would prevent users from re-using existing file descriptors more than once, pretty much defeating the point of this PR.
Correct. No fd
ownership transfer takes place. We did not create the fd
, so we should not close it IMHO. Doing so would prevent users from re-using existing file descriptors more than once, pretty much defeating the point of this PR.
trevnorris
Oct 13, 2015
Contributor
This needs to be clearly documented. Possibly with big bold text. I can see users thinking they're handing off the fd.
This needs to be clearly documented. Possibly with big bold text. I can see users thinking they're handing off the fd.
jwueller
Oct 13, 2015
Author
Contributor
Yes, I agree that this is important information. The current patch adds the phrase
Specified file descriptors will not be closed automatically.
to all relevant locations. Do you think the whole thing should be bolded, or just the "not"? I am currently leaning towards emphasizing the whole phrase.
Yes, I agree that this is important information. The current patch adds the phrase
Specified file descriptors will not be closed automatically.
to all relevant locations. Do you think the whole thing should be bolded, or just the "not"? I am currently leaning towards emphasizing the whole phrase.
trevnorris
Oct 13, 2015
Contributor
It should be in its own paragraph and prefixed with Note:
. No need for that to be bold. But having that is common in documentation.
Thanks for working w/ me on this.
It should be in its own paragraph and prefixed with Note:
. No need for that to be bold. But having that is common in documentation.
Thanks for working w/ me on this.
@@ -556,7 +564,10 @@ If `options` is a string, then it specifies the encoding. Example: | |||
|
|||
fs.appendFile('message.txt', 'data to append', 'utf8', callback); | |||
|
|||
## fs.appendFileSync(filename, data[, options]) | |||
Note that any specified file descriptor needs to be opened for appending. | |||
Specified file descriptors will not be closed automatically. |
trevnorris
Oct 13, 2015
Contributor
Right here. And also in every location where this is true. documentation duplication in all relevant locations is cool.
Right here. And also in every location where this is true. documentation duplication in all relevant locations is cool.
jwueller
Oct 13, 2015
Author
Contributor
Oops, I missed this comment. Please check my response above. How should this be modified, exactly?
Oops, I missed this comment. Please check my response above. How should this be modified, exactly?
@trevnorris Updated again with the discussed changes. |
|
||
_Note: Specified file descriptors will not be closed automatically._ | ||
|
||
## fs.appendFileSync(file, data[, options]) |
trevnorris
Oct 13, 2015
Contributor
I see that you have the note in fs.writeFileSync
, but not here in fs.appendFileSync
. Is it necessary here?
I see that you have the note in fs.writeFileSync
, but not here in fs.appendFileSync
. Is it necessary here?
jwueller
Oct 13, 2015
Author
Contributor
I am not sure what you are asking, exactly. The note is present in all asynchronous versions, while being omitted in the synchronous ones, since the latter segments only refer to the asynchronous documentation anyway.
I am not sure what you are asking, exactly. The note is present in all asynchronous versions, while being omitted in the synchronous ones, since the latter segments only refer to the asynchronous documentation anyway.
trevnorris
Oct 14, 2015
Contributor
Whoop. Sorry. Got lost in the diff. Can see that now.
Whoop. Sorry. Got lost in the diff. Can see that now.
process.nextTick(function() { | ||
req.oncomplete.call(req, null, path); | ||
}); | ||
|
trevnorris
Oct 13, 2015
Contributor
can remove this extra line.
can remove this extra line.
process.nextTick(function() { | ||
req.oncomplete.call(req, null); | ||
}); | ||
|
trevnorris
Oct 13, 2015
Contributor
ditto.
ditto.
@@ -528,13 +530,21 @@ If `options` is a string, then it specifies the encoding. Example: | |||
|
|||
fs.writeFile('message.txt', 'Hello Node.js', 'utf8', callback); | |||
|
|||
## fs.writeFileSync(filename, data[, options]) | |||
Any specified file descriptor has to support writing. |
trevnorris
Oct 13, 2015
Contributor
Mind adding that data written to fd will be appended?
Guess we don't expose an lseek()
in fs so users couldn't reposition where data is written. Hm. Future feature.
Mind adding that data written to fd will be appended?
Guess we don't expose an lseek()
in fs so users couldn't reposition where data is written. Hm. Future feature.
trevnorris
Oct 13, 2015
Contributor
nm. i see what's happening.
nm. i see what's happening.
jwueller
Oct 13, 2015
Author
Contributor
Whether data will be appended depends on the flags that the fd
was created with. The a
flags will obviously append, but the w
flags will create/truncate instead.
Whether data will be appended depends on the flags that the fd
was created with. The a
flags will obviously append, but the w
flags will create/truncate instead.
trevnorris
Oct 13, 2015
Contributor
Yup. I crossed the following code from fs.append
:
if (isFd(path))
options.flag = 'a';
Sorry
Yup. I crossed the following code from fs.append
:
if (isFd(path))
options.flag = 'a';
Sorry
Almost there! |
After some further digging, I noticed that we have – in fact – several different approaches to dealing with file descriptor arguments available right now:
This makes the PR in it's current form differ from existing functionality. It seems to me we should discuss which of these approaches is the right way to go here. The latter one does not seem to have existed at the time of writing the original PR (and I did not want to make things more complicated than necessary at the time), but is definitely the most flexible option now that it exists "in the wild". The behavior of |
I believe how the functionality works in this PR is most appropriate of the differences. So I'd say we proceed with this then work on fixing the other implementations where necessary. |
That seems reasonable. Updated again. |
Looks great. Running CI one last time for sanity then will land this thing: https://ci.nodejs.org/job/node-test-pull-request/508/ |
Thank you very much for reviewing everything! |
var req = new FSReqWrap(); | ||
req.context = context; | ||
req.oncomplete = readFileAfterOpen; | ||
|
||
if (context.isUserFd) { | ||
process.nextTick(function() { | ||
req.oncomplete.call(req, null, path); |
thefourtheye
Oct 15, 2015
Contributor
Why not req.oncomplete(null, path)
?
Why not req.oncomplete(null, path)
?
jwueller
Oct 15, 2015
Author
Contributor
It would not behave correctly. This call is supposed to emulate the behavior of binding.open()
. Omitting the context would lead to incompatibilities with following code (ex: readFileAfterOpen()
, where the context is extracted from this
).
It would not behave correctly. This call is supposed to emulate the behavior of binding.open()
. Omitting the context would lead to incompatibilities with following code (ex: readFileAfterOpen()
, where the context is extracted from this
).
trevnorris
Oct 15, 2015
Contributor
the default context is the object to the left of the .
so I think @thefourtheye is right in this case.
the default context is the object to the left of the .
so I think @thefourtheye is right in this case.
jwueller
Oct 15, 2015
Author
Contributor
Of course he is. I actually tripped myself there. Sorry! I'll clean that up immediately.
Of course he is. I actually tripped myself there. Sorry! I'll clean that up immediately.
@trevnorris One more fix. Thanks @thefourtheye for reviewing! |
These changes affect the following functions and their synchronous counterparts: * fs.readFile() * fs.writeFile() * fs.appendFile() If the first parameter is a uint32, 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.
Final CI for sanity: https://ci.nodejs.org/job/node-test-pull-request/518/ If CI is good let's land this thing! LGTM |
@trevnorris Something seems to have failed, but it is not obvious to me what is happening, exactly. Can you help me out? |
@jwueller None of the failing tests are related to your PR. Everything seems fine. |
These changes affect the following functions and their synchronous counterparts: * fs.readFile() * fs.writeFile() * fs.appendFile() If the first parameter is a uint32, 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. PR-URL: #3163 Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Landed on 0803962. Thanks! Note: This is labelled as |
@trevnorris Great! Thanks again! |
These changes affect the following functions and their synchronous counterparts: * fs.readFile() * fs.writeFile() * fs.appendFile() If the first parameter is a uint32, 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. PR-URL: #3163 Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Notable changes: * buffer: (Breaking) Removed both 'raw' and 'raws' encoding types from Buffer, these have been deprecated for a long time (Sakthipriyan Vairamani) nodejs#2859. * console: (Breaking) Values reported by console.time() now have 3 decimals of accuracy added (Michaël Zasso) nodejs#3166. * fs: - fs.readFile*(), fs.writeFile*(), and fs.appendFile*() now also accept a file descriptor as their first argument (Johannes Wüller) nodejs#3163. - (Breaking) In fs.readFile(), if an encoding is specified and the internal toString() fails the error is no longer thrown but is passed to the callback (Evan Lucas) nodejs#3485. - (Breaking) In fs.read() (using the fs.read(fd, length, position, encoding, callback) form), if the internal toString() fails the error is no longer thrown but is passed to the callback (Evan Lucas) nodejs#3503. * http: - Fixed a bug where pipelined http requests would stall (Fedor Indutny) nodejs#3342. - (Breaking) When parsing HTTP, don't add duplicates of the following headers: Retry-After, ETag, Last-Modified, Server, Age, Expires. This is in addition to the following headers which already block duplicates: Content-Type, Content-Length, User-Agent, Referer, Host, Authorization, Proxy-Authorization, If-Modified-Since, If-Unmodified-Since, From, Location, Max-Forwards (James M Snell) nodejs#3090. - (Breaking) The callback argument to OutgoingMessage#setTimeout() must be a function or a TypeError is thrown (James M Snell) nodejs#3090. - (Breaking) HTTP methods and header names must now conform to the RFC 2616 "token" rule, a list of allowed characters that excludes control characters and a number of separator characters. Specifically, methods and header names must now match /^[a-zA-Z0-9_!#$%&'*+.^`|~-]+$/ or a TypeError will be thrown (James M Snell) nodejs#2526. * node: - (Breaking) Deprecated the _linklist module (Rich Trott) nodejs#3078. - (Breaking) Removed require.paths and require.registerExtension(), both had been previously set to throw Error when accessed (Sakthipriyan Vairamani) nodejs#2922. * npm: Upgraded to version 3.3.6 from 2.14.7, see https://github.com/npm/npm/releases/tag/v3.3.6 for more details. This is a major version bump for npm and it has seen a significant amount of change. Please see the original npm v3.0.0 release notes for a list of major changes (Rebecca Turner) nodejs#3310. * src: (Breaking) Bumped NODE_MODULE_VERSION to 47 from 46, this is necessary due to the V8 upgrade. Native add-ons will need to be recompiled (Rod Vagg) nodejs#3400. * timers: Attempt to reuse the timer handle for setTimeout().unref(). This fixes a long-standing known issue where unrefed timers would perviously hold beforeExit open (Fedor Indutny) nodejs#3407. * tls: - Added ALPN Support (Shigeki Ohtsu) nodejs#2564. - TLS options can now be passed in an object to createSecurePair() (Коренберг Марк) nodejs#2441. - (Breaking) The default minimum DH key size for tls.connect() is now 1024 bits and a warning is shown when DH key size is less than 2048 bits. This a security consideration to prevent "logjam" attacks. A new minDHSize TLS option can be used to override the default. (Shigeki Ohtsu) nodejs#1831. * util: - (Breaking) util.p() was deprecated for years, and has now been removed (Wyatt Preul) nodejs#3432. - (Breaking) util.inherits() can now work with ES6 classes. This is considered a breaking change because of potential subtle side-effects caused by a change from directly reassigning the prototype of the constructor using `ctor.prototype = Object.create(superCtor.prototype, { constructor: { ... } })` to using `Object.setPrototypeOf(ctor.prototype, superCtor.prototype)` (Michaël Zasso) nodejs#3455. * v8: (Breaking) Upgraded to 4.6.85.25 from 4.5.103.35 (Ali Ijaz Sheikh) nodejs#3351. - Implements the spread operator, see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Spread_operator for further information. - Implements new.target, see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/new.target for further information. * zlib: Decompression now throws on truncated input (e.g. unexpected end of file) (Yuval Brik) nodejs#2595. PR-URL: nodejs#3466
Notable changes: * buffer: (Breaking) Removed both 'raw' and 'raws' encoding types from Buffer, these have been deprecated for a long time (Sakthipriyan Vairamani) #2859. * console: (Breaking) Values reported by console.time() now have 3 decimals of accuracy added (Michaël Zasso) #3166. * fs: - fs.readFile*(), fs.writeFile*(), and fs.appendFile*() now also accept a file descriptor as their first argument (Johannes Wüller) #3163. - (Breaking) In fs.readFile(), if an encoding is specified and the internal toString() fails the error is no longer thrown but is passed to the callback (Evan Lucas) #3485. - (Breaking) In fs.read() (using the fs.read(fd, length, position, encoding, callback) form), if the internal toString() fails the error is no longer thrown but is passed to the callback (Evan Lucas) #3503. * http: - Fixed a bug where pipelined http requests would stall (Fedor Indutny) #3342. - (Breaking) When parsing HTTP, don't add duplicates of the following headers: Retry-After, ETag, Last-Modified, Server, Age, Expires. This is in addition to the following headers which already block duplicates: Content-Type, Content-Length, User-Agent, Referer, Host, Authorization, Proxy-Authorization, If-Modified-Since, If-Unmodified-Since, From, Location, Max-Forwards (James M Snell) #3090. - (Breaking) The callback argument to OutgoingMessage#setTimeout() must be a function or a TypeError is thrown (James M Snell) #3090. - (Breaking) HTTP methods and header names must now conform to the RFC 2616 "token" rule, a list of allowed characters that excludes control characters and a number of separator characters. Specifically, methods and header names must now match /^[a-zA-Z0-9_!#$%&'*+.^`|~-]+$/ or a TypeError will be thrown (James M Snell) #2526. * node: - (Breaking) Deprecated the _linklist module (Rich Trott) #3078. - (Breaking) Removed require.paths and require.registerExtension(), both had been previously set to throw Error when accessed (Sakthipriyan Vairamani) #2922. * npm: Upgraded to version 3.3.6 from 2.14.7, see https://github.com/npm/npm/releases/tag/v3.3.6 for more details. This is a major version bump for npm and it has seen a significant amount of change. Please see the original npm v3.0.0 release notes for a list of major changes (Rebecca Turner) #3310. * src: (Breaking) Bumped NODE_MODULE_VERSION to 47 from 46, this is necessary due to the V8 upgrade. Native add-ons will need to be recompiled (Rod Vagg) #3400. * timers: Attempt to reuse the timer handle for setTimeout().unref(). This fixes a long-standing known issue where unrefed timers would perviously hold beforeExit open (Fedor Indutny) #3407. * tls: - Added ALPN Support (Shigeki Ohtsu) #2564. - TLS options can now be passed in an object to createSecurePair() (Коренберг Марк) #2441. - (Breaking) The default minimum DH key size for tls.connect() is now 1024 bits and a warning is shown when DH key size is less than 2048 bits. This a security consideration to prevent "logjam" attacks. A new minDHSize TLS option can be used to override the default. (Shigeki Ohtsu) #1831. * util: - (Breaking) util.p() was deprecated for years, and has now been removed (Wyatt Preul) #3432. - (Breaking) util.inherits() can now work with ES6 classes. This is considered a breaking change because of potential subtle side-effects caused by a change from directly reassigning the prototype of the constructor using `ctor.prototype = Object.create(superCtor.prototype, { constructor: { ... } })` to using `Object.setPrototypeOf(ctor.prototype, superCtor.prototype)` (Michaël Zasso) #3455. * v8: (Breaking) Upgraded to 4.6.85.25 from 4.5.103.35 (Ali Ijaz Sheikh) #3351. - Implements the spread operator, see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Spread_operator for further information. - Implements new.target, see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/new.target for further information. * zlib: Decompression now throws on truncated input (e.g. unexpected end of file) (Yuval Brik) #2595. PR-URL: #3466
This is a re-targeted submission of the archived PR #8522, implementing the archived issue #8471.
The original description follows.
These changes affect the following functions and their synchronous counterparts:
fs.readFile()
fs.writeFile()
fs.appendFile()
If the first parameter is a
uint32
, 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.