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

Cannot abort a stream that already has a writer #92

Open
JounQin opened this issue Sep 20, 2021 · 9 comments
Open

Cannot abort a stream that already has a writer #92

JounQin opened this issue Sep 20, 2021 · 9 comments
Labels

Comments

@JounQin
Copy link

JounQin commented Sep 20, 2021

Hi, thanks for this great polyfill first.

I want to close the WritableStream when the user cancels the download process, but it throws as title, is there any solution?

I'm using the hacky way currently.

if (evt.data.aborted) {
  if (stream._writer) {
    stream._writer.abort()
    stream._writer = undefined
  }
  stream.abort()
}
@MattiasBuelens
Copy link
Owner

You must not use stream._writer, or any properties with an underscore for that matter. These are internal properties for managing the internal state of the WritableStream, and they are not part of the public API.

If you acquired a writer (by calling writableStream.getWriter()), it is your responsibility to keep a reference to it around (i.e. store it in a variable). You shouldn't use stream._writer to access it, since that breaks the encapsulation. (For example, if you do readableStream.pipeTo(writableStream), then the pipe will create an internal writer which must not be exposed to developers.)

To abort a WritableStream, you should either call writer.abort() or writableStream.abort(), but not both. Use the former if you currently have an active writer, and the latter if you don't. For example:

const writableStream = createWritableStreamSomehow();
const writer = writableStream.getWriter();
// stream is now locked
writer.write("hello");
writer.write("world");
writer.abort();

or

const writableStream = createWritableStreamSomehow();
// note: no getWriter(), so stream is unlocked
writableStream.abort();

Remember that you can use releaseLock() to release the active writer:

const writableStream = createWritableStreamSomehow();
const writer = writableStream.getWriter();
writer.write("hello");
writer.releaseLock();
// stream is now unlocked again
writableStream.abort();

@JounQin
Copy link
Author

JounQin commented Sep 20, 2021

it is your responsibility to keep a reference to it around

Maybe, but it seems not possible when writing a library.

// lib
export const createStream = () => {
  const stream = new WritableStream({
    // ...
  })

  on('abort', () => {
    // we need to abort `stream` here and notify `writer` through error
    stream.abort()
    emit('aborted')
  })

  return stream
}

// user codes
import { createStream } from 'lib'

export const download = () => {
  const writer = createStream().getWriter()
  try {
    await writer.write(xxx)
  } catch(err) {
    if (!err) {
      // aborted
    }
  }
}

See aslo jimmywarting/StreamSaver.js#13 (comment)

@MattiasBuelens
Copy link
Owner

MattiasBuelens commented Sep 20, 2021

It seems like you want to error the stream instead. Try this:

export const createStream = () => {
  let controller;
  const stream = new WritableStream({
    start(c) {
      controller = c
    },
    // ...
  })

  on('abort', () => {
    controller.error(new DOMException('Aborted', 'AbortError')) // or some other error
    emit('aborted')
  })

  return stream
}

If you want to do some logic when the user aborts the WritableStream (e.g. through writer.abort()), then you should implement an abort() method on the sink:

const stream = new WritableStream({
  start(c) {
    controller = c
  },
  abort(reason) {
    // stop the download somehow
  }
  // ...
})

@JounQin
Copy link
Author

JounQin commented Sep 20, 2021

Thanks for your reply first.

It seems like you want to error the stream instead. Try this:

I tried, but I don't know why when I add logic like let controller; controller = c; then Firefox does not run abort callback as you mentioned below anymore at all. 😂

If you want to do some logic when the user aborts the WritableStream (e.g. through writer.abort()), then you should implement an abort() method on the sink:

The user may abort by canceling downloading, so we need to notify the user's reader to cancel and writer to abort in the cancel callback in ReadableStream, instead of reverse. 🤣

See https://github.com/jimmywarting/StreamSaver.js/blob/master/sw.js#L67

@MattiasBuelens
Copy link
Owner

I tried, but I don't know why when I add logic like let controller; controller = c; then Firefox does not run abort callback as you mentioned below anymore at all. 😂

That doesn't make any sense. 😕 It doesn't sound like a problem with the polyfill, but rather with your code. I suggest you debug it and step through those lines of code to see what's going wrong.

The user may abort by canceling downloading, so we need to notify the user's reader to cancel and writer to abort in the cancel callback in ReadableStream, instead of reverse. 🤣

No, the cancel callback inside ReadableStream shouldn't call cancel on a reader. It's always the other way around: calling reader.cancel() will call the cancel callback of the provided source (the object passed to new ReadableStream).

I don't know what your code looks like, but most of this cancel and abort stuff should be propagated automatically:

  • If you abort the writable end of a TransformStream, then the readable end will become errored.
  • If you error the writable end of a pipe chain (readable.pipeTo(writable)), then the readable end will become canceled.

I don't understand why your code would even have a reader and a writer at the same time? 😕

@JounQin
Copy link
Author

JounQin commented Sep 20, 2021

That doesn't make any sense. 😕 It doesn't sound like a problem with the polyfill, but rather with your code. I suggest you debug it and step through those lines of code to see what's going wrong.

Sorry, a typo, I mean cancel callback in the ReadStream from lib streamsaver is not called when the user cancels download, actually. I'm not sure whether it is a Firefox bug or something else.

No, the cancel callback inside ReadableStream shouldn't call cancel on a reader. It's always the other way around: calling reader.cancel() will call the cancel callback of the provided source (the object passed to new ReadableStream).

I don't know what your code looks like, but most of this cancel and abort stuff should be propagated automatically:

I don't understand why your code would even have a reader and a writer at the same time? 😕

The first ReadableStream comes from fetch's res.body, it is a native ReadableStream, and ReadableStream.prototype.pipeTo and TransformStream are both unavailable in Firefox. So we choose to use streamsaver as fallback, it uses iframe + serviceworker to pipe streams, and I have to pipe reader to writer manually.

What we want is to cancel the fetch's reader on user aborts download.

export const pipeStream = async <T = unknown>(
  reader: ReadableStreamDefaultReader<T>,
  writer: WritableStreamDefaultWriter<T>,
  signal?: AbortSignal,
) => {
  let chunkResult: ReadableStreamDefaultReadResult<T>

  let aborted: boolean | undefined

  while (!signal?.aborted && !(chunkResult = await reader.read()).done) {
    try {
      await writer.write(chunkResult.value)
    } catch (err) {
      if (signal?.aborted) {
        break
      }

      // we need some way to get notified when user aborts download
      if (!err) {
        aborted = true
        break
      }

      throw err
    }
  }

  if (signal?.aborted || aborted) {
    await Promise.all([reader.cancel(), writer.abort()])
    throw new DOMException('aborted', 'AbortError')
  }

  return writer.close()
}

const download = (readStream: ReadableStream<T>, fileName: string, signal?: AbortSignal) => {
  const writeStream = streamSaver.createWriteStream(fileName)
  pipeStream(readStream.getReader(), writeStream.getWriter(), signal)
}

@MattiasBuelens
Copy link
Owner

MattiasBuelens commented Sep 20, 2021

So basically you've built your own version of ReadableStream.pipeTo() outside of this polyfill. The problem is that your version doesn't implement all of the requirements for pipeTo, specifically these:

  • Errors must be propagated forward: if source is or becomes errored, then abort the destination.
  • Errors must be propagated backward: if destination is or becomes errored, then cancel the source.

This polyfill does implement these requirements, see the source.

May I suggest a different approach? Instead of building a separate version of pipeTo that works with Firefox's native streams, adapt the native stream into a polyfill stream first using e.g. web-streams-adapter. Then you can use the full API of the polyfill again, including pipeTo. 😉

EDIT: Forget to mention, it's published as @mattiasbuelens/web-streams-adapter.

@JounQin
Copy link
Author

JounQin commented Sep 20, 2021

Thanks, will give it a try!

@shakamd
Copy link

shakamd commented Sep 21, 2021

@JounQin, curious to see if you are able to solve this. I'm also trying to support user download cancellation in my app with streamsaver.

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

No branches or pull requests

3 participants