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

Chained otherwise never ends #104

Merged
merged 3 commits into from
Jul 12, 2014
Merged

Conversation

apaleslimghost
Copy link
Collaborator

When you chain 3 or more .otherwises, i.e. σ.otherwise(ρ).otherwise(τ), and ρ is the only nonempty stream, the resulting stream doesn't ever send any data.

This pull request so far just adds a failing test, I'm having a look at fixing it.

@apaleslimghost
Copy link
Collaborator Author

Ok, a minimal test case is that any stream that does a redirect can't be otherwise'd, for example:

var σ = require('./lib');

var xs = σ(function(push, next) {
    next(σ([1, 2, 3]));
});

xs.otherwise(σ([])).toArray(σ.log);

You'd expect [1,2,3] but nothing ever gets logged.

@caolan
Copy link
Owner

caolan commented Jul 1, 2014

@quarterto thanks for reporting. I can confirm I'm seeing this behaviour.

@vqvu
Copy link
Collaborator

vqvu commented Jul 8, 2014

I think what's happening here is that the second redirect that's done by otherwise is redirecting to the wrong stream (xs instead of <the stream to which xs redirects>). Streams probably need to keep track of the streams to which they delegate for possible later use.

Something like this:

diff --git a/lib/index.js b/lib/index.js
index c5060f1..379e8b6 100755
--- a/lib/index.js
+++ b/lib/index.js
@@ -371,6 +371,7 @@ function Stream(/*optional*/xs, /*optional*/ee, /*optional*/mappingHint) {
     this._observers = [];
     this._destructors = [];
     this._send_events = false;
+    this._delegate = null;
     this.source = null;

     // Old-style node Stream.pipe() checks for this
@@ -847,6 +848,10 @@ Stream.prototype._redirect = function (to) {
     // coerce to Stream
     to = _(to);

+    while (to._delegate) {
+        to = to._delegate;
+    }
+
     to._consumers = this._consumers.map(function (c) {
         c.source = to;
         return c;
@@ -866,6 +871,8 @@ Stream.prototype._redirect = function (to) {
         this.pause();
         to._checkBackPressure();
     }
+
+    this._delegate = to;
 };

 /**

@apaleslimghost
Copy link
Collaborator Author

@vqvu your patch passes the tests, want me to commit it, or you wanna start a new pull request?

@vqvu
Copy link
Collaborator

vqvu commented Jul 8, 2014

Feel free to incorporate it into your pull request.
On Jul 8, 2014 2:38 AM, "Matt Brennan" notifications@github.com wrote:

@vqvu https://github.com/vqvu your patch passes the tests, want me to
commit it, or you wanna start a new pull request?


Reply to this email directly or view it on GitHub
#104 (comment).

@apaleslimghost
Copy link
Collaborator Author

Tests fail on Node 0.11 but it's not related to this pull request.

caolan pushed a commit that referenced this pull request Jul 12, 2014
Chained otherwise never ends
@caolan caolan merged commit 59c9c63 into caolan:master Jul 12, 2014
@caolan
Copy link
Owner

caolan commented Jul 12, 2014

Wow, great work you two, not an easy one to spot ;) 🍻

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.

3 participants