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

Reconnecting a Stream after creating it - possible? #1004

Closed
alvestrand opened this issue Jul 3, 2019 · 7 comments
Closed

Reconnecting a Stream after creating it - possible? #1004

alvestrand opened this issue Jul 3, 2019 · 7 comments

Comments

@alvestrand
Copy link

Domenic said this might be a good place to ask...
I'm designing an API where I intend to use WHATWG Streams.
I have to make a choice between a model where I show the user a bunch of readable/writable/transferStreams and tell the user "connect them all", and a model where I present a connected network of streams, and tell the user "make the changes you want".

I experimented a little, and found that the code below didn't work - but I could find no obvious API to do what I wanted; once you have connected a stream, there seems to be no convenient API for reconnecting them, short of messing around with the readers and writers explicitly.

Is there something obvious I'm missing?

Thanks in advance!


var dest1 = new WritableStream({
  write(chunk) {
    console.log('Dest 1 got ' + chunk);
  }
});

var dest2 = new WritableStream({
  write(chunk) {
    console.log('Dest 2 got ' + chunk);
  }
});

var src = new ReadableStream({
  start(controller) {
    const producer = () => {
      controller.enqueue('chunk');
      setTimeout(producer, 1000);
    };
    setTimeout(producer, 1000);
  }
});

src.pipeTo(dest1);

const pipeTo1 = () => {
  console.log('Piping to 1');
  src.pipeTo(dest1);
}

const pipeTo2 = async () => {
  console.log('Piping to 2');
  // Here there's something missing
  src.pipeTo(dest2);
}
@MattiasBuelens
Copy link
Collaborator

MattiasBuelens commented Jul 3, 2019

You can pass an AbortSignal to pipeTo() to stop an ongoing pipe (*). You can use preventAbort and preventCancel to prevent the aborted pipe from also cancelling the input and/or aborting the output.

Note that the pipe still waits for any pending writes to complete, and both ends remain locked until everything has been written. That means you still have to wait for the pipeTo() promise to settle (either resolve or reject) before you can start a new pipe.

let currentPipePromise = Promise.resolve();
let currentPipeAbortController = new AbortController();

async function startNewPipe(dest) {
  // Abort the previous pipe
  currentPipeAbortController.abort();
  currentPipeAbortController = new AbortController();
  // Wait for previous pipe to complete
  await currentPipePromise.catch(() => {});
  // Start new pipe, but make sure we can abort it later
  currentPipePromise = src.pipeTo(dest, {
    preventAbort: true,
    preventCancel: true,
    signal: currentPipeAbortController.signal
  });
}

startNewPipe(dest1);

const pipeTo1 = () => {
  console.log('Piping to 1');
  startNewPipe(dest1);
}

const pipeTo2 = async () => {
  console.log('Piping to 2');
  startNewPipe(dest2);
}

(*) Not yet natively supported in any browser, but you could use a polyfill.

@alvestrand
Copy link
Author

alvestrand commented Jul 4, 2019

Thanks for the tips!

Unfortunately abort() didn't cause the currentPipePromise to be rejected - the promise remained unresolved. This was true for both the polyfill version and the native version (Chrome 75).

In the polyfill version, data stopped flowing, though - in native, it just continued. So something happened.

Attaching a zip with the current code (Github doesn't want .html or .js files)
streammove.zip

@alvestrand
Copy link
Author

If I removed the "preventAbort" from the pipeTo operation, the first attempt to change the destination of the output stream worked with the shim, but an attempt to move the stream back resulted in a DOMError.

It seems that reattaching streams is an area that hasn't been explored much.

@domenic
Copy link
Member

domenic commented Jul 12, 2019

I agree something is going wrong here, either in the spec or polyfill. All the pieces (AbortController, preventAbort/preventCancel) are in place, but when we put them together, the desired effect isn't showing up.

My demo is at https://boom-bath.glitch.me/stream-reconnect.html. If @MattiasBuelens or @ricea could dig in to this and let us know where things are going wrong, I'd be very grateful. Otherwise, I'll try to make some time next week.

@MattiasBuelens
Copy link
Collaborator

MattiasBuelens commented Jul 12, 2019

Very interesting.

Since both preventAbort and preventCancel are true, the list of actions in ReadableStreamPipeTo's abortAlgorithm is empty. The action passed to "shutdown with an action" consists of "waiting for all of the actions in actions". This is defined here.

However, the steps in that algorithm do not account for the case where promises is an empty list! It attaches a fulfillmentHandler to every promise in promises, and checks whether we're done only inside that handler. But if no promises were passed in, then no handlers are attached and we never check whether we're done.

The fix is obvious: if the list of promises passed to the "wait for all" algorithm is empty, then it should immediately call successSteps with an empty list. I'll make a PR for the reference implementation and the polyfill, and I'll try to write a spec change for the promises guide.

EDIT: Oh, and I guess this would be a good case for a web platform test as well. Even more PRs! 😅

@MattiasBuelens
Copy link
Collaborator

One more thing: since we're aborting the pipe, the pipe promise will reject with an AbortError instead of resolving. So we need to use .catch() instead of .then():

document.querySelector("#switch").onclick = () => {
  controller.abort();
  pipePromise.catch(() => {
    rsA.pipeTo(wsB);
  });
};

(You should probably check whether the rejection reason was actually an AbortError, but this should be good enough for demo purposes. 😛)

domenic pushed a commit to w3ctag/promises-guide that referenced this issue Jul 16, 2019
domenic pushed a commit to web-platform-tests/wpt that referenced this issue Jul 16, 2019
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Jul 24, 2019
…eventCancel and preventAbort works, a=testonly

Automatic update from web-platform-tests
Streams: verify that aborting a pipe with both preventCancel and preventAbort works

See whatwg/streams#1004 and w3ctag/promises-guide#58.
--

wpt-commits: 0da3476dcd5fd3148041d090d2330cf8d412d7f9
wpt-pr: 17816
xeonchen pushed a commit to xeonchen/gecko that referenced this issue Jul 25, 2019
…eventCancel and preventAbort works, a=testonly

Automatic update from web-platform-tests
Streams: verify that aborting a pipe with both preventCancel and preventAbort works

See whatwg/streams#1004 and w3ctag/promises-guide#58.
--

wpt-commits: 0da3476dcd5fd3148041d090d2330cf8d412d7f9
wpt-pr: 17816
domenic pushed a commit that referenced this issue Jul 29, 2019
When both the preventCancel and preventAbort flags are set, the abortAlgorithm passes an empty list of actions to "wait for all". This helper should immediately resolve the returned promise when no promises are given, but instead the promise remained pending forever.

This fixes the reference implementation of the "wait for all" helper to resolve correctly when given an empty list of promises. This matches the spec change in w3ctag/promises-guide#58.

Originally reported in #1004 (comment).
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 4, 2019
…eventCancel and preventAbort works, a=testonly

Automatic update from web-platform-tests
Streams: verify that aborting a pipe with both preventCancel and preventAbort works

See whatwg/streams#1004 and w3ctag/promises-guide#58.
--

wpt-commits: 0da3476dcd5fd3148041d090d2330cf8d412d7f9
wpt-pr: 17816

UltraBlame original commit: 40dd37dfbbfbc0659c81c2a386f5fe966b3c8203
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 4, 2019
…eventCancel and preventAbort works, a=testonly

Automatic update from web-platform-tests
Streams: verify that aborting a pipe with both preventCancel and preventAbort works

See whatwg/streams#1004 and w3ctag/promises-guide#58.
--

wpt-commits: 0da3476dcd5fd3148041d090d2330cf8d412d7f9
wpt-pr: 17816

UltraBlame original commit: 40dd37dfbbfbc0659c81c2a386f5fe966b3c8203
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 4, 2019
…eventCancel and preventAbort works, a=testonly

Automatic update from web-platform-tests
Streams: verify that aborting a pipe with both preventCancel and preventAbort works

See whatwg/streams#1004 and w3ctag/promises-guide#58.
--

wpt-commits: 0da3476dcd5fd3148041d090d2330cf8d412d7f9
wpt-pr: 17816

UltraBlame original commit: 40dd37dfbbfbc0659c81c2a386f5fe966b3c8203
@domenic
Copy link
Member

domenic commented Jun 26, 2020

This is now fixed in the reference implementation and polyfill, and web platform tests exist too!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants