-
Notifications
You must be signed in to change notification settings - Fork 21
Add a method for cancelling all subscribing streams #26
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
base: main
Are you sure you want to change the base?
Conversation
I think this is missing the ability to cancel the original stream producer. Correct me if I am wrong please, but I think this is the most crucial part of cancellation. |
We're actually using this in production right now and it's working fine. Untitled.mov |
@Khongchai yes, I know the regular flow works fine. But what happens if you cancel. Will the original stream producer terminate? How do you signal termination? |
Yes. In the video, I cancelled the orignal stream producer (i clicked on the stop button bottom right) and both stream stops. I created a new endpoint that when called with the streamId (in our case the id of the chat), invokes my method, which then invokes the originating ReadableStream's cancel method |
I think you are not understading me. Your code DOES stop the stream. So the client will not be getting any new data. But the underlying producer is NOT stopped, it is still producing new data, it just cannot be enqued due to closed reader. So your approach works on surface, but I don't think is the correct one. There is also a race condition, if the stream is cancelled before you start listening for the signal. |
I think I misunderstood what you meant, I thought by "original stream producer", you meant the stream returned from let originalStream = await obtainSomehow();
const makeStream = () => {
return new ReadableStream({
start(controller) {
// ... publish from originalStream to controller, etc
},
cancel() {
originalStream.cancel() // this?
},
});
}
const myStream = await resumableStream.createNewResumableStream(
streamId,
makeStream,
);
Essentially people can just react to
I won't handle this because like the original author, it is unlikely to happen (at least for us). Hope this clears things up for you. Cheers |
This makes sense, thanks! I think it would be great to write some minimal documentation in regards to the discussion we had since many people will be wondering the same thing. |
@tjazsilovsek yeah you're right, most people will probably want to react to the cancellation. I added a short section for the stream cancellation. Do you think that could have answered your question? |
Great, I would just add a sentence or two about the mechanism. How this stream cancel method only closes the reader, so users should also consider aborting the original stream. Maybe it would make sense, to expose onCancelled callback directly on createNewResumableStream? |
Hi, folks! Thanks for the great lib. I had this use case for cancelling multiple subscribing streams -- here's my implementation:
When cancelling, we publish a cancel event that calls
reader.cancel
. This will causereader.read().then(async ({ done, value }) => {
done
to be true here and follows the normal done flow.I also released the reader lock because I needed to assert the state of the original stream in the new test case. This is probably useful for library user in the future too if they wanna do something with it after.