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

Properly exert backpressure for pipeline-constructed streams. #372

Merged
merged 3 commits into from
Sep 20, 2015

Conversation

vqvu
Copy link
Collaborator

@vqvu vqvu commented Sep 10, 2015

Noticed in #371 (comment).

pipeline overrides the write method but doesn't return anything. For node streams, this has the effect of disabling backpressure, since they do a check for false === write(...). For others, it may have the effect of always enabling backpressure.

We need to return the correct value. This also means supporting the drain event, which it didn't before.

My solution was to

  1. construct the result stream via _(),
  2. override write to return src.write(x), and
  3. pipe dest to the result stream.

In the process, I refactored the logic for pipe out into a separate function that is used by both pipeline and pipe (with slightly different behavior).

This was actually more complicated than I thought, so I'd like some eyes on this besides my own.

@svozza
Copy link
Collaborator

svozza commented Sep 19, 2015

I won't pretend to fully understand what's going on here because I find the messing around with write a bit confusing but this looks good. It also has the side effect of closing our oldest open PR! #101

@vqvu
Copy link
Collaborator Author

vqvu commented Sep 20, 2015

Oh, cool. I didn't realize we had an open PR for this. TBH, I'm not too fond of messing with write either, but I don't think there's any other way.

vqvu added a commit that referenced this pull request Sep 20, 2015
Properly exert backpressure for pipeline-constructed streams.
@vqvu vqvu merged commit b245986 into caolan:master Sep 20, 2015
@vqvu vqvu added this to the v2.6.0 milestone Nov 18, 2015
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