Patch stream2 writable v2

[thomas-riccardi]

Here is a review with fixes and cleanups on top of riaan53/gridfs-stream master, for Writable stream only.

• no more internal buffering, see High memory usage because of unbound internal buffering #58
• finish event is emitted when the file is fully on GridFS (using flushwritable).
• destroy() emits an error (and the error can be specified as destroy(err), because the write to GridFS may be aborted, so the file is corrupted.
• Added documentation and tests for special events open and close (and finish)
• Added documentation and tests for destroy

Pending questions:

I don't know what we should do in case of error:

• Internally: Currently we just call _error() which emits error and then _close().
  It works, but we could do other things: _write and _flush give callbacks that accept an error as first argument. If there is either a _write or a _flush (we cannot have both at the same time) running during the error (for example _store.open() failed, or _store.write() failed, or _store.close() failed), then we could (also?) call that callback with the error.
  From what I read in the code of nodejs v0.10.31 and v0.12.0, both with indirectly emit error, and the _write() callback will call the optional write() callback so the caller will be notified too. It may be useful to notify the caller this way, but it's harder to do it properly: we need to avoid multiple error emitted, and we still need to call _close() (and maybe in a precise order: it may be useful to guarantee that 'error' happens before (or after?) 'close'; for that we could delay _close() to nextTick). I don't know what is the best practice here... Maybe we should ask on nodejs mailing list?
• externally: We always call _store.close(), so we always commit the file to GridFS by writing the document in the files collection. This may not be the best solution in case of error: If an error happened, the file may be corrupted, but we store it like no error happened. Two solutions:
  • keep commiting the file to GridFS, but add an extra metadata field error to notify that this file may be corrupted. It would be up to the caller to remove the file afterward if it wants that, or not.
  • remove the file and the chunks on error, because the file is likely corrupted, but it's an issue if the file was overwriting another file, because we would want to rollback to a previous state (which is what we would expect. It's how AWS S3 works for example), and the current GridStore from nodejs mongodb driver 2.0.15 deletes the old chunks before starting to write the new ones, so we cannot... Maybe it's a bug of the mongodb driver, but I don't see how it could implement what we want without performance issues.

I haven't reviewed the Readable stream yet, I'm less familiar with Readable streams than with Writable ones...

PS: I patched the commit riaan53@c6ac486 in this branch to remove the wrongly committed temporary file test/tmp/1mbBlob.

[riaan53]

Awesome! thank you.

[Reggino]

@triccardi-systran - Sorry for the late reply: thank you for this! Very cool.

Your version has been published as version 1.0.2.

[thomas-riccardi]

@Reggino
What about semver? a581ca9 removed public method destroySoon, so a major version bump should have been done. This PR add new features (destroy() now optionally takes an error parameter), so a minor version bump should have been done.
If we don't want to break the API we could add destroySoon again, as a deprecated alias to destroy(), this is now almost the same behavior as we don't queue internally (the only difference is that the current _write() was written, and now is ignored (hm, this case may lead to a Chunk leak... maybe we should always wait for the _write to finish...).

[thomas-riccardi]

(Nevermind for the Chunk leak in case of destroy(): it's not part of the File data, but it's still associated to the File via its files_id. Luckily on File unlink the MongoDB driver removes all Chunks associated to the File via files_id, so the Chunk will not definitely leak.)

[Reggino]

@triccardi-systran Thanks, you're right. I've added the deprecated destroySoon and publish it as 1.1.0

[thomas-riccardi]

For the error handling questions, I opened a ticket at mongodb:
https://jira.mongodb.org/browse/NODE-372