Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

_checkBackPressure should cancel any repeated consume (Fixes #325). #326

Merged
merged 1 commit into from
Jun 29, 2015

Conversation

vqvu
Copy link
Collaborator

@vqvu vqvu commented Jun 28, 2015

This seems to fix the issue @svozza found in #325, but I'm not too confident in it.

Fortunately, the 3.x engine doesn't have this bug, so we should be OK long-term.

@vqvu vqvu mentioned this pull request Jun 28, 2015
@svozza
Copy link
Collaborator

svozza commented Jun 28, 2015

Poor v2, really creaking at the seams now! Out of interest, why are you unsure of how well this will work?

@vqvu
Copy link
Collaborator Author

vqvu commented Jun 28, 2015

Because the fix that I wanted to make was

diff --git a/lib/index.js b/lib/index.js
index 5ce4fd9..297cd6e 100755
--- a/lib/index.js
+++ b/lib/index.js
@@ -824,7 +824,8 @@ Stream.prototype.resume = function () {
                 this.emit('drain');
             }
         }
-    } while (this._repeat_resume);
+    } while (this._repeat_resume && !this.paused);
+    this._repeat_resume = false;
     this._resume_running = false;
 };

But this broke stream redirection for some reason, and that's the one part of the v2 engine that I don't fully understand. v3 was written because the interplay between _checkBackPressure and stream redirection was too complex for me to grok.

@vqvu
Copy link
Collaborator Author

vqvu commented Jun 28, 2015

Also, the fact that you need to mess with _repeat_resume outside of the resume function seems a bit sketchy to me.

@svozza
Copy link
Collaborator

svozza commented Jun 29, 2015

Well, it might not be the optimal solution but it's still preferable to what we have now.

vqvu added a commit that referenced this pull request Jun 29, 2015
_checkBackPressure should cancel any repeated consume (Fixes #325).
@vqvu vqvu merged commit ade706b into caolan:master Jun 29, 2015
@vqvu vqvu added this to the v2.6.0 milestone Nov 18, 2015
@vqvu vqvu deleted the resume-loop-fix branch November 18, 2015 05:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants