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

pipeTo should return a cancelable promise #446

Closed
jakearchibald opened this issue May 8, 2016 · 6 comments
Closed

pipeTo should return a cancelable promise #446

jakearchibald opened this issue May 8, 2016 · 6 comments

Comments

@jakearchibald
Copy link

If I wanted to create:

const stream = merge([stream1, stream2, stream3]);

Being able to pipe each stream to an identity stream would make this easy and move a lot of the work out of JS, but the missing bit is being able to react to cancellation. If stream is cancelled while piping stream1, I want to know about that so I can cancel stream2 and stream3.

@domenic
Copy link
Member

domenic commented May 8, 2016

Sure, this has always been the plan. One day, when cancelable promises are ready, they'd be retrofitted here. You'd be able to cancel the piping process in this way.

@jakearchibald
Copy link
Author

Figured you'd have already thought about this. Ta!

@jplaisted
Copy link

domenic's cancelable promise spec appears to have been withdrawn :( Should this still be blocked in the hopes that one day standardized cancelable Promises are a thing? Or should this be revisited in a non-cancelable-Promise way?

@tyoshino
Copy link
Member

I'd like to confirm if I understand the example by Jake in the OP correctly.

The merge function creates an identity stream to represent a readable stream producing the result of merging the provided readable streams. Right?

When stream is cancelled, the identity stream needs to propagate the cancellation to the writable side. So, we need to design the identity stream to allow that.

pipeTo() can already notify the caller of any error on dest. It can be caught to cancel the remaining not-yet-piped readable streams.

function merge(readables) {
  // Configure to propagate cancellation on the readable side to the writable side
  const id = ...;
  function pipeNext() {
    if (readables.length === 0) {
      return;
    }
    const rs = readables.shift();
    rs.pipeTo(id.writable, {preventClose: readables.length === 0}).then(() => {
      pipeNext();
    }, e => {
      for (rs of readables) {
        rs.cancel(e);
      }
    });
  }
  pipeNext();
  return id.readable;
}

IIRC, here cancelling an ongoing pipeTo() isn't needed anywhere. So, I guess I'm misunderstanding the OP.

@ricea
Copy link
Collaborator

ricea commented May 22, 2017

Off topic: I didn't realise the merge() function was so non-trivial. It ought to be placed in some easily-discoverable location. Maybe in the Examples section of the standard?

@domenic
Copy link
Member

domenic commented May 22, 2017

@tyoshino, I think you are right; canceling pipeTo() is not needed for a functional merge().

@ricea, I agree. I wonder if we should consider adding it like we did tee().

domenic added a commit that referenced this issue Nov 7, 2018
Closes #446.

Co-authored-by: Adam Rice <ricea@chromium.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

5 participants