Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid race condition by using fs.open and fs.fstat, also allow file descriptor to be passed #125

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
15 changes: 15 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,14 @@ Enable or disable accepting ranged requests, defaults to true.
Disabling this will not send `Accept-Ranges` and ignore the contents
of the `Range` request header.

##### autoClose

If `autoClose` is `false`, 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 `autoClose` is set to
`true` (default behavior), on `error` or `end` the file descriptor
will be closed automatically.

##### cacheControl

Enable or disable setting `Cache-Control` response header, defaults to
Expand Down Expand Up @@ -85,6 +93,11 @@ in the given order. By default, this is disabled (set to `false`). An
example value that will serve extension-less HTML files: `['html', 'htm']`.
This is skipped if the requested file already has an extension.

##### fd

If `fd` is specified, send will ignore the `path` argument and will use the
specified file descriptor. This means that no `'open'` event will be emitted.

##### immutable

Enable or diable the `immutable` directive in the `Cache-Control` response
Expand Down Expand Up @@ -126,9 +139,11 @@ The `SendStream` is an event emitter and will emit the following events:
- `error` an error occurred `(err)`
- `directory` a directory was requested `(res, path)`
- `file` a file was requested `(path, stat)`
- `open` a file descriptor was opened for streaming `(fd)`
- `headers` the headers are about to be set on a file `(res, path, stat)`
- `stream` file streaming has started `(stream)`
- `end` streaming has completed
- `close` the file descriptor was closed

#### .pipe

Expand Down
208 changes: 140 additions & 68 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
var createError = require('http-errors')
var debug = require('debug')('send')
var deprecate = require('depd')('send')
var destroy = require('destroy')
var encodeUrl = require('encodeurl')
Copy link
Contributor

Choose a reason for hiding this comment

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

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+ ?

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@jcready jcready Jan 23, 2017

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

var escapeHtml = require('escape-html')
var etag = require('etag')
Expand All @@ -24,6 +23,7 @@ var fs = require('fs')
var mime = require('mime')
var ms = require('ms')
var onFinished = require('on-finished')
var isFinished = onFinished.isFinished
var parseRange = require('range-parser')
var path = require('path')
var statuses = require('statuses')
Expand Down Expand Up @@ -62,6 +62,18 @@ var MAX_MAXAGE = 60 * 60 * 24 * 365 * 1000 // 1 year

var UP_PATH_REGEXP = /(?:^|[\\/])\.\.(?:[\\/]|$)/

/**
* Regular expression to match a bad file number error code
* Node on Windows incorrectly sets the error code to `OK`
* instead of `EBADF` because of a bug in libuv
* @type {RegExp}
* @const
* @see {@link https://github.com/nodejs/node/issues/3718}
* @see {@link https://github.com/libuv/libuv/pull/613}
*/

var BAD_FD_REGEXP = /^(EBADF|OK)$/

/**
* Module exports.
* @public
Expand Down Expand Up @@ -161,9 +173,17 @@ function SendStream (req, path, options) {
? resolve(opts.root)
: null

this.fd = typeof opts.fd === 'number'
? opts.fd
: null

this.autoClose = opts.autoClose !== false

if (!this._root && opts.from) {
this.from(opts.from)
}

this.onFileSystemError = this.onFileSystemError.bind(this)
}

/**
Expand Down Expand Up @@ -403,17 +423,17 @@ SendStream.prototype.headersAlreadySent = function headersAlreadySent () {
SendStream.prototype.isCachable = function isCachable () {
var statusCode = this.res.statusCode
return (statusCode >= 200 && statusCode < 300) ||
statusCode === 304
/* istanbul ignore next */ statusCode === 304
}

/**
* Handle stat() error.
* Handle file system error.
*
* @param {Error} error
* @private
*/

SendStream.prototype.onStatError = function onStatError (error) {
SendStream.prototype.onFileSystemError = function onFileSystemError (error) {
switch (error.code) {
case 'ENAMETOOLONG':
case 'ENOENT':
Expand Down Expand Up @@ -500,7 +520,35 @@ SendStream.prototype.redirect = function redirect (path) {
}

/**
* Pipe to `res.
* End the stream.
*
* @private
*/

SendStream.prototype.end = function end () {
this.send = this.close
if (this._stream) this._stream.destroy()
if (this.autoClose) this.close()
}

/**
* Close the file descriptor.
*
* @private
*/

SendStream.prototype.close = function close () {
if (typeof this.fd !== 'number') return
var self = this
fs.close(this.fd, function (err) { /* istanbul ignore next */
if (err && !BAD_FD_REGEXP.test(err.code)) return self.onFileSystemError(err)
self.fd = null
self.emit('close')
})
}

/**
* Pipe to `res`.
*
* @param {Stream} res
* @return {Stream} res
Expand All @@ -510,10 +558,28 @@ SendStream.prototype.redirect = function redirect (path) {
SendStream.prototype.pipe = function pipe (res) {
// root path
var root = this._root
var self = this

// references
this.res = res

// response finished, done with the fd
if (isFinished(res)) {
this.end()
return res
}

onFinished(res, this.end.bind(this))

if (typeof this.fd === 'number') {
fs.fstat(this.fd, function (err, stat) {
if (err) return self.onFileSystemError(err)
self.emit('file', self.path, stat)
self.send(self.path, stat)
})
return res
}

// decode the path
var path = decode(this.path)
if (path === -1) {
Expand Down Expand Up @@ -619,7 +685,7 @@ SendStream.prototype.send = function send (path, stat) {
return
}

debug('pipe "%s"', path)
debug('pipe fd "%d" for path "%s"', this.fd, path)

// set header fields
this.setHeader(path, stat)
Expand Down Expand Up @@ -705,7 +771,7 @@ SendStream.prototype.send = function send (path, stat) {
return
}

this.stream(path, opts)
this.stream(opts)
}

/**
Expand All @@ -717,34 +783,42 @@ SendStream.prototype.send = function send (path, stat) {
SendStream.prototype.sendFile = function sendFile (path) {
var i = 0
var self = this

debug('stat "%s"', path)
fs.stat(path, function onstat (err, stat) {
if (err && err.code === 'ENOENT' && !extname(path) && path[path.length - 1] !== sep) {
// not found, check extensions
return next(err)
}
if (err) return self.onStatError(err)
if (stat.isDirectory()) return self.redirect(path)
self.emit('file', path, stat)
self.send(path, stat)
var redirect = this.redirect.bind(this, path)

debug('open "%s"', path)
fs.open(path, 'r', function onopen (err, fd) {
if (!err) return sendStats(path, fd, self.onFileSystemError, redirect)
return err.code === 'ENOENT' && !extname(path) && path[path.length - 1] !== sep
? next(err) // not found, check extensions
: self.onFileSystemError(err)
})

function next (err) {
if (self._extensions.length <= i) {
return err
? self.onStatError(err)
? self.onFileSystemError(err)
: self.error(404)
}

var p = path + '.' + self._extensions[i++]

debug('stat "%s"', p)
fs.stat(p, function (err, stat) {
debug('open "%s"', p)
fs.open(p, 'r', function (err, fd) {
if (err) return next(err)
if (stat.isDirectory()) return next()
self.emit('file', p, stat)
self.send(p, stat)
sendStats(p, fd, next, next)
})
}

function sendStats (path, fd, onError, onDirectory) {
debug('stat fd "%d" for path "%s"', fd, path)
fs.fstat(fd, function onstat (err, stat) {
if (err || stat.isDirectory()) {
return fs.close(fd, function (e) { /* istanbul ignore next */
return (err || e) ? onError(err || e) : onDirectory()
})
}
self.fd = fd
self.emit('file', path, stat)
self.emit('open', fd)
self.send(path, stat)
})
}
}
Expand All @@ -755,72 +829,59 @@ SendStream.prototype.sendFile = function sendFile (path) {
* @param {String} path
* @api private
*/

SendStream.prototype.sendIndex = function sendIndex (path) {
var i = -1
var self = this

function next (err) {
return (function next (err) {
if (++i >= self._index.length) {
if (err) return self.onStatError(err)
if (err) return self.onFileSystemError(err)
return self.error(404)
}

var p = join(path, self._index[i])

debug('stat "%s"', p)
fs.stat(p, function (err, stat) {
fs.open(p, 'r', function onopen (err, fd) {
if (err) return next(err)
if (stat.isDirectory()) return next()
self.emit('file', p, stat)
self.send(p, stat)
debug('stat fd "%d" for path "%s"', fd, p)
fs.fstat(fd, function (err, stat) {
if (err || stat.isDirectory()) {
return fs.close(fd, function (e) {
next(err || e)
})
}
self.fd = fd
self.emit('file', p, stat)
self.emit('open', fd)
self.send(p, stat)
})
})
}

next()
})()
}

/**
* Stream `path` to the response.
* Stream to the response.
*
* @param {String} path
* @param {Object} options
* @api private
*/

SendStream.prototype.stream = function stream (path, options) {
// TODO: this is all lame, refactor meeee
var finished = false
var self = this
var res = this.res
SendStream.prototype.stream = function stream (options) {
options.fd = this.fd
options.autoClose = false

// pipe
var stream = fs.createReadStream(path, options)
this.emit('stream', stream)
stream.pipe(res)

// response finished, done with the fd
onFinished(res, function onfinished () {
finished = true
destroy(stream)
})
var stream = this._stream = new PartStream(options)

// error handling code-smell
stream.on('error', function onerror (err) {
// request already finished
if (finished) return

// clean up stream
finished = true
destroy(stream)

// error
self.onStatError(err)
})
// error
stream.on('error', this.onFileSystemError)

// end
stream.on('end', function onend () {
self.emit('end')
})
stream.on('end', this.emit.bind(this, 'end'))

// pipe
this.emit('stream', stream)
stream.pipe(this.res)
}

/**
Expand Down Expand Up @@ -892,6 +953,17 @@ SendStream.prototype.setHeader = function setHeader (path, stat) {
}
}

util.inherits(PartStream, fs.ReadStream)

function PartStream (opts) {
fs.ReadStream.call(this, null, opts)
this.bufferSize = opts.highWaterMark || this.bufferSize
}

PartStream.prototype.destroy = PartStream.prototype.close = function closePartStream () {
this.readable = !(this.destroyed = this.closed = !(this.fd = null))
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

/**
* Clear all headers from a response.
*
Expand Down
1 change: 0 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
"dependencies": {
"debug": "2.6.9",
"depd": "~1.1.1",
"destroy": "~1.0.4",
"encodeurl": "~1.0.1",
"escape-html": "~1.0.3",
"etag": "~1.8.1",
Expand Down
Loading