fix: undefined stream.destroy call

[electrachong]

To end streaming in case of error, we were calling an unofficial method of the stream API which was removed and does not exist in the version of node we use. The method is re-added officially in node v.8 but until we upgrade we need to destroy the streams manually, by pushing null for readables and calling stream.end() for writables.

See: nodejs/readable-stream#124
and nodejs/node#12925

[electrachong]

@ironman-machine try

[ironman-machine]

Hello @electrachong

"try": Success: Try build successfully launched on 'http://ci.ironmann.io/gh/scality/Integration/16653' with the following env. args:

{
    "SCALITY_INTEGRATION_BRANCH": "ultron/master",
    "SCALITY_ARSENAL_BRANCH": "fix/stream-destroy",
    "REPO_NAME": "Arsenal",
    "DEFAULT_BRANCH": "master"
}

[ironman-machine]

☀️ 👍 circleCI test succeeded!

[rahulreddy]

I remember the first time when we thought of using this in the context of ReadySetStream module. Good to know that this has been deprecated now.

[jonathan-gramain]

Is this._sourceStream always a read and write stream? In the tests you use a pass through stream which is both, but in real deployment it could be only a Readable? That's what is said in the SubStreamInterface constructor. In such case I think we don't want to call push() or end() on it.

[jonathan-gramain]

Actually I'm wrong, the push function is meant for readables actually, but is it allowed to call it outside the stream implementation class? May be but I remember having seen such a warning, feel free to ignore if it's safe.

[electrachong]

as used so far it's only been a transform stream (md5sum) but I guess theoretically we could use this class with a source stream that is only a readable which is why I only specified readable in the constructor jsdoc. Maybe we can be more specific in the jsdoc until/unless we want to extend the class for a different scenario, which I don't see as likely anyway.

I see the warning you are referencing from your memory in the node docs 😛

Note: The readable.push() method is intended be called only by Readable Implementers, and only from within the readable._read() method.

I feel like this case may be an exception because pushing null is supposed to signify the end of the data, and we are just doing it to signal an early external termination but maybe it deserves more thought.

By the way, I'm not sure if I need to think about cleaning up event listeners before / after ending the stream as well, do you have any thoughts on that?

[jonathan-gramain]

If you own the implementation of the readable stream, you can use a flag that you set when you want to destroy the stream, which the _read implementation checks and sends null if set, then you are following the docs recommendation. I would think the previous destroy() implementation was doing something along those lines (may be worth checking the original code?)

If you don't control how the readable stream fetches its data, it's likely not a good idea to interfere with its processing from outside generally speaking except with clearly supported calls (e.g. pause), specifically when the docs recommend not to do so (push). I don't know how to deal with it though, is pausing and/or unpiping the stream not good enough? There may not be the need to do a nuke cleanup as long as the read stream control flow eventually stops reading from its source.

I don't have a clear view about the event listeners, I don't think it matters much, except if you want your listeners to be notified when the stream is closed abruptly so that they stay registered at least until they reach an appropriate event (say, 'close'), though I don't know/remember if anything else than 'end' event is part of the node stream specs to signal the stream is aborted.

[LaurenSpiegel]

As discussed:

1. The currentStream class definition will listen for "stopStreamingToAzure" and on that event will push "null" into the stream alerting Azure that there will be no more data.
2. In the stopStreaming function emit "stopStreamingToAzure".
3. Just unpipe the piper (which is the actual request piped to the md5 transform to tell the transform to stop getting data.

[electrachong]

Hey Lauren, I updated based on the above steps

Do it again human slave!:point_right: :runner: (Oh and the pull request has been updated, by the way.)

[ironman-machine]

PR has been updated. Reviewers, please be cautious.

[ironman-machine]

CONFLICT (add/add): Merge conflict in lib/s3middleware/azureHelpers/mpuUtils.js
CONFLICT (add/add): Merge conflict in lib/s3middleware/azureHelpers/SubStreamInterface.js

[electrachong]

@ironman-machine try

[ironman-machine]

Hello @electrachong

"try": Success: Try build successfully launched on 'http://ci.ironmann.io/gh/scality/Integration/16833' with the following env. args:

{
    "SCALITY_INTEGRATION_BRANCH": "ultron/master",
    "SCALITY_ARSENAL_BRANCH": "fix/stream-destroy",
    "REPO_NAME": "Arsenal",
    "DEFAULT_BRANCH": "master"
}

[ironman-machine]

☀️ 👍 circleCI test succeeded!

[rahulreddy]

@ironman-machine r+

[ironman-machine]

Hello @rahulreddy

"r+": Success

[ironman-machine]

⌛ PR is now pending. CI build url: http://ci.ironmann.io/gh/scality/Integration/16845

[ironman-machine]

☀️ 👍 circleCI test succeeded!