Avoid race condition by using fs.open and fs.fstat, also allow file descriptor to be passed #125
Conversation
jcready
changed the title
option is respected in node.js < 0.10. Also remove the `destroy` dependency.
| @@ -15,7 +15,6 @@ | |||
| var createError = require('http-errors') | |||
| var debug = require('debug')('send') | |||
| var deprecate = require('depd')('send') | |||
| var destroy = require('destroy') | |||
There was a problem hiding this comment.
The usage of the destroy module is to work-around a fd leak in Node.js 0.10 and lower. I don't see the work-around reproduced in here from the removal, so does this mean that this PR would require Node.js 0.12+ ?
There was a problem hiding this comment.
Or I guess it's no longer relevant since I think the changes here are to manually manage the fd instead of letting Node.js's read stream do so?
There was a problem hiding this comment.
@dougwilson that is correct. I've even added afterEach assertions to make sure that all open file descriptors have been closed after every test has run. Although, to be honest, supporting node < 0.10 does make working with streams much more tricky. Node >= 0.10 has the Stream2 implementation and supports the autoClose parameter when using fs.ReadStream. To support the autoClose option I'm exposing in send() I have to overwrite the fs.ReadStream instance's destroy() method to ensure that it won't close the file descriptor.
There was a problem hiding this comment.
Yea, makes sense. And to be clear, the fd leak is in Node.js 0.10 and lower, which includes 0.10, not sure if there was some confusion around that, so just checking :)
All modules in our organization will be adopting 0.10 as the minimum version as they get their major versions bumped currently. Some may get an even higher minimum version if warranted.
There was a problem hiding this comment.
@dougwilson yeah the fd leak shouldn't be a problem since we're now opening and closing the fd ourselves instead of relying on fs.createReadStream() to do it for us. The "destroy" module was essentially just adding an .on('open', close) handler to the stream.
Regarding the min Node version being supported, when do you see that happening? Even bumping the min version to 0.10 would be handy since it will respect the autoClose option by default.
There was a problem hiding this comment.
Gotcha. Regarding min Node.js, the current plan is to hold it until the next major version bump of this module (required by semver, though this is 0.x so technically not, I try to use that as the 1.x bump). That timeline is still TBD, but likely will correspond to around the same time Express 5.0 is slated to be released. Since Express is run by volunteer, there is no date until everything is done, at which point a date is added. I foresee at least a few more months.
There was a problem hiding this comment.
@dougwilson so after you mentioned the fd leak I took a hard look at all the logic and it turned out you were right to bring it up. There were two situations where the fd would be leaked: 1) the res stream ends before it is passed to sendStream.pipe() and 2) the res ends after being passed to sendStream.pipe() but before sendStream.send() is called. I have accounted for these two situations in my latest commit and have added a few tests for them.
of returning an error code of `EBADF` it returns an error code of `OK` which makes zero sense
| PartStream.prototype.destroy = PartStream.prototype.close = function closePartStream () { | ||
| this.readable = !(this.destroyed = this.closed = !(this.fd = null)) | ||
| } | ||
|
|
There was a problem hiding this comment.
This class can be eliminated if support for node < 0.10 is dropped. Please do let me know if that change is going to be happening soon because it will have a much higher impact on the multipart/byteranges support I'm working on.
There was a problem hiding this comment.
Hi @jcready sorry I didn't reply right away, was on a trip and then forgot :) I just replied to your previous question on Node.js support and the TL;DR version is at least a few more months of 0.8.
1. The `res` ends before it is passed to `send().pipe()` 2. The `res` ends after pipe but before the `open` event
# Conflicts: # index.js # test/send.js
# Conflicts: # README.md # package.json # test/send.js
Adds two new options:
fd- If specified, send will ignore thepathargument and will use the specified file descriptor. This means that no'open'event will be emitted.autoClose- Iffalse, then the file descriptor won't be closed, even if there's an error. It is your responsibility to close it and make sure there's no file descriptor leak. If set totrue(default behavior), onerrororendthe file descriptor will be closed automatically.It also uses
fs.open,fs.fstatand passes the file descriptor tofs.createReadStreamto ensure there is no race condition where the the file located at the specifiedpathis unlinked, renamed, or replaced in between the current calls tofs.statandfs.createReadStreamwhich would result in an incorrect response.I've also renamed
onStatErrortoonFileSystemErrorto more accurately reflect what types of errors it is responsible for handling.Resolves #122
Resolves #123