clever-js icon indicating copy to clipboard operation
clever-js copied to clipboard

Add test to verify end is called at the appropriate time.

Open jloveridge opened this issue 10 years ago • 2 comments

Add a testcase which demonstrates that 'end' is being called while a stream is paused. This can be problematic if you are doing async processing of data. In my case I use streams to process Section data and retrieve the teacher and student data related to the section. (I do this in batches only requesting information not yet seen to avoid unnecessary API requests.) As end is being called while the stream is supposed to be paused, something the Node.js docs for Readable streams indicates should not happen, it caused me to early exit the function resulting in reports of data syncs being inaccurate, and worse could cause other data not to fully sync as those tasks were still in process.

I have worked around the issue by using an additional promise which only resolves once I am done with the async processing and have resumed the stream. This, however, should not be necessary. The current behavior is incorrect and can lead to unwitting errors in implementations relying on the streaming behavior.

jloveridge avatar Sep 06 '15 03:09 jloveridge

Hi @jloveridge, My name is Ryan, I'm an engineer at Clever. Thanks for the unittest! I've create a internal ticket to track the bug your test catches. As soon as we have a fix, we'll let you know. We are currently in the middle of back-to-school right now, so it may take a little time before we are able to look at it, but we really appreciate your contribution.

burnsed avatar Sep 08 '15 23:09 burnsed

Hey @jloveridge,

I've looked into your suggestion and am having trouble finding the node documentation you're referring to. Would you mind pointing me to it?

Something to keep in mind is clever-js uses version 1.0.x of readable-stream. Looking through, it appears that once all data is consumed in a readable-stream "end" may be emitted. In the unit test provided, the final pause happens on an empty stream, which does not prevent an "end" emission in accordance to our node version.

Let me know what you think. We're happy to work through this with you.

christiangriset avatar Sep 28 '15 19:09 christiangriset