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

Shouldn't WritableStreamDefaultWriter.write() reject if underlyingSink.write calls controller.error()? #1332

Open
jedwards1211 opened this issue Nov 21, 2024 · 2 comments

Comments

@jedwards1211
Copy link

jedwards1211 commented Nov 21, 2024

What is the issue with the Streams Standard?

I admit that I don't fully understand what the standard says should happen here. But Chrome and Node both have the same behavior in this case and it seems wrong. In both, WritableStreamDefaultWriter.write() resolves when underlyingSink.write() calls controller.error(), but rejects when underlyingSink.write() throws or returns a Promise that rejects.

Why shouldn't WritableStreamDefaultWriter.write() reject in all cases?

{
  const stream = new WritableStream({
    write(chunk, controller) {
      controller.error(new Error('test'))
    },
  })
  const writer = stream.getWriter()
  writer.write().then(
    () => console.error('with controller.error: resolved'), // this is what happens
    (error) => console.error('with controller.error: rejected', error)
  )
}

{
  const stream = new WritableStream({
    write(chunk, controller) {
      throw new Error('test')
    },
  })
  const writer = stream.getWriter()
  writer.write().then(
    () => console.error('with throw: resolved'),
    (error) => console.error('with throw: rejected', error) // this is what happens
  )
}

{
  const stream = new WritableStream({
    async write(chunk, controller) {
      throw new Error('test')
    },
  })
  const writer = stream.getWriter()
  writer.write().then(
    () => console.error('with reject: resolved'),
    (error) => console.error('with reject: rejected', error) // this is what happens
  )
}

Node

with controller.error: resolved
with throw: rejected Error: test
    at Object.write (/Users/andy/gh/web-streams-finally/src/temp.js:17:13)
    at invokePromiseCallback (node:internal/webstreams/util:162:10)
    at node:internal/webstreams/util:167:23
    at writableStreamDefaultControllerProcessWrite (node:internal/webstreams/writablestream:1129:5)
    at writableStreamDefaultControllerAdvanceQueueIfNeeded (node:internal/webstreams/writablestream:1244:5)
    at node:internal/webstreams/writablestream:1318:7
with reject: rejected Error: test
    at Object.write (/Users/andy/gh/web-streams-finally/src/temp.js:30:13)
    at invokePromiseCallback (node:internal/webstreams/util:162:10)
    at node:internal/webstreams/util:167:23
    at writableStreamDefaultControllerProcessWrite (node:internal/webstreams/writablestream:1129:5)
    at writableStreamDefaultControllerAdvanceQueueIfNeeded (node:internal/webstreams/writablestream:1244:5)
    at node:internal/webstreams/writablestream:1318:7

Browser

VM172:9 with controller.error: resolved
VM172:23 with throw: rejected Error: test
VM172:36 with reject: rejected Error: test
@jedwards1211 jedwards1211 changed the title Shouldn't WritableStreamDefaultWriter.write() reject if underlyingSink calls controller.error()? Shouldn't WritableStreamDefaultWriter.write() reject if underlyingSink.write calls controller.error()? Nov 21, 2024
@ricea
Copy link
Collaborator

ricea commented Nov 22, 2024

I think the motivation for this behaviour was that the user shouldn't be punished for something out of their control. In your example, controller.error() is called from within write() but its main purpose is to signal errors that happen asynchronously. The following is an example of the intended use case:

const stream = new WritableStream({
  start(controller) {
    this.handle = await openFile(file);
    this.handle.onerror = evt => { controller.error(new Error(evt.reason); };
  },
  write(chunk) {
    await this.handle.write(chunk);
  },
});

If the onerror event fires while a write is in progress, we don't actually know whether the write succeeded or failed (but if we also get a promise rejection then that will be returned from writer.write()). So returning a rejection from writer.write() would not be actionable.

@jedwards1211
Copy link
Author

But the next write would reject, and that has to be actionable...why would it be safer for code to believe the previous write succeeded when it may not have?

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

2 participants