fix stream.js to allow pausing when there are many blocks #24
Conversation
staltz
changed the title
|
Will review this one now :) |
| @@ -149,8 +152,10 @@ Stream.prototype.abort = function (err) { | |||
| this.ended = err || true | |||
| var i = this.blocks.streams.indexOf(this) | |||
| if (~i) this.blocks.streams.splice(i, 1) | |||
| if (!this.sink.ended && this.sink.end) | |||
| if (!this.sink.ended && this.sink.end) { | |||
| this.sink.ended = true | |||
| Stream.prototype._handleBlock = function(block) { | ||
| while (true) { | ||
| if (this.sink.paused) return false | ||
| if (this.sink.paused) return null |
There was a problem hiding this comment.
I'm wondering why exactly this is different from what it was before? I mean after was a this.sink.paused check instead of the null check, so was the a concurrency bug where the value of this.sink.paused was changed between the return and the else if check?
There was a problem hiding this comment.
This was so tricky, and it was so late at night, that I can't quite remember the reason. Wanna try changing it to false and seeing what happens?
There was a problem hiding this comment.
Interesting it didn't work before. Wow tricky bug!
| else if (this.sink.end) | ||
| return this.sink.end(this.ended === true ? null : this.ended) | ||
| } | ||
| return |
|
Nice work on this one. The module wasn't at all tested for the pause so no wonder you run into a few of those. It's really tricky business implementing that correctly. |
|
Yep! I'm also curious if this will improve some things elsewhere. Like, the new test would basically end up in an infinite loop with the old implementation. Maybe this infinite loop was happening elsewhere and slowing things down? (Well not just slowing, but deadlocking them) |
1st test, 2nd fix
IF the stream has many blocks AND the sink pauses frequently, THEN the previous implementation has a bug where it gets stuck at the last record, giving this last record over and over to the sink, in a loop.
The culprit was that
_handleBlockneeds 3 different return values, we can't signal "pause" on neithertruenorfalse(I tried many times, all sorts of tricks).