use 'close' event on file stream for reliability

[Trott]

Change listened-for event on the output stream in storage/disk.js from
'finish' to 'close' to improve reliability. With 'finish', there is a
race condition such that sometimes the callback runs before the file
exists.

Refs: #238

[Trott]

/ping @mcollina @ronag Does this seem like the correct event to listen for in this situation, or was the previous code correct and this is indicative of a more elusive problem?

[ronag]

That's interesting. 'finish' should be fine. I will dig into this. What version of node?

In the meantime this workaround is not entirely correct either, you might end up invoking the callback twice since 'close' will/should be emitted after 'error'. Also you are not handling the 'error' event from the source stream.

      cb = once(cb)
      var finalPath = path.join(destination, filename)
      var outStream = fs.createWriteStream(finalPath)

      file.stream.pipe(outStream)
      file.stream.on('error', err => {
        outStream.destroy()
        cb(err)
      })
      outStream.on('error', cb)
      outStream.on('close', function () {
        cb(null, {
          destination: destination,
          filename: filename,
          path: finalPath,
          size: outStream.bytesWritten
        })
      })

Also are you aware that _handleFile will not destroy/free the source stream upon failure? I'm not sure if that might cause file descriptor leaks elsewhere.

Furthermore this code assumes that the source stream has not already completed. If it has completed this will never call the callback. But I guess that's a minor.

[ronag]

Yes, I think this might be a bug in node. Streams can emit 'finish' before 'open' if no data has been written before end(), thus causing a race condition.

[ronag]

nodejs/node#29930

[Trott]

That's interesting. 'finish' should be fine. I will dig into this. What version of node?

@ronag I was using Node.js 12.11.1.

[Trott]

nodejs/node#29930

Awesome and thank you! I'll close this.