Define WritableStreamDefaultController.signal

[yutakahirano]

Define the property to signal abort to UnderlyingSink even
when it has a pending write or close operation.

Fixes #1015.

• At least two implementers are interested (and none opposed):
  • Chrome
  • Firefox
• Tests are written and can be reviewed and commented upon at:
  • Add tests for WritableStreamDefaultController.signal web-platform-tests/wpt#29310
• Implementation bugs are filed:
  • Chrome: https://bugs.chromium.org/p/chromium/issues/detail?id=1215992
  • Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1714341
  • Safari: https://bugs.webkit.org/show_bug.cgi?id=226575

(See WHATWG Working Mode: Changes for more details.)

---

Preview | Diff

[ricea]

We're going to need some tests.

[ricea]

I believe that this won't throw an exception. @domenic can you confirm?

We need to be careful about re-entrancy here, but it looks safe to me.

[domenic]

I guess technically it could throw if some spec added a throwing algorithm to the "abort algorithms" list, but that seems unlikely. I filed whatwg/dom#984 on making it impossible.

[domenic]

This kind of switching on error type is a bit unusual. What is the intention here? What kind of test would pass with this change that would fail without it?

[yutakahirano]

let reject;
close_promise = new Promise((res, rej) => reject = rej);
const ws = new WritableStream({
  start(c) {
    c.signal.addEventListener('abort', () => {
      const abort_error = new DOMException('foobar', 'AbortError');
      reject(abort_error);
    });
  }, close() {
    return close_promise;
  }
});

const writer = ws.getWriter();
writer.close();
const reason = Exception('hoge');
writer.abort(reason);

In this example, I think it's natural that writer.closed will be rejected by reason, but without this clause it'll be rejected with abort_error.

Another solution would be to expose pendingAbortRequest.reason in the controller so that the underlying sink can reject the result of close() with that.

[domenic]

In this example, I think it's natural that writer.closed will be rejected by reason, but without this clause it'll be rejected with abort_error.

Hmm, I'm not sure I agree. The close() underlying sink hook explicitly said it failed to close (i.e. it returned a promise rejected with an "AbortError" DOMException, indicating that it tried to close but closing got aborted). I think that failure should be propagated to the closed promise...

[yutakahirano]

But in this case, the operation fails because of the abort call coming from the producer, so I think it should be rejected with the abort reason. With the other solution, that code would look like:

let reject;
close_promise = new Promise((res, rej) => reject = rej);
const ws = new WritableStream({
  start(c) {
    c.signal.addEventListener('abort', () => {
      reject(c.abortReason);
    });
  }, close() {
    return close_promise;
  }
});

const writer = ws.getWriter();
writer.close();
const reason = Exception('hoge');
writer.abort(reason);

[MattiasBuelens]

I think I prefer reject(c.abortReason).

[MattiasBuelens]

Hmm. So you'd do resolve() instead? Or something like return close_promise.catch(() => {})? 🤔

[domenic]

I think I need a non-artificial example. Why did you reject the closed promise in the first place? I would think that the signal would have no impact on the promise returned from close, but instead would be used while opening a file or writing to it.

E.g.

let fd;
const ws = new WritableStream({
  start(c) {
    fd = await fs.open(filePath, { signal: c.signal });
  },
  write(chunk, c) {
    await fd.write(chunk, { signal: c.signal });
  }
  close() {
    await fd.close(); // no signal; closing isn't abortable
  }
});

If you changed the line to await fd.close({ signal: c.signal }), i.e. you let someone abort in the middle of closing---then yes, I'd expect ws.closed to reject with an "AbortError" DOMException, because we failed to close the stream due to the closing process being aborted!

[yutakahirano]

Let me quote @vasilvv's example for WebTransport.

Imagine I have a web app where I can do drawing online. Every 100ms or so, I send the entirety of the drawing on a dedicated WebTransport stream. Note that if a network connection becomes spotty for a short period of time, this will cause a lot of streams to be open at once, so I want to keep only the three most recent streams open, and reset all streams that are older than the last three.

Here we want to map the reset operation to abort(). We are planning to resolve the promise returned by close() when the the last chunk is ack-ed, and until then the close operation is always cancellable - by just sending a RESET_STREAM frame.

[domenic]

OK, so in that case, it seems like you can avoid the problem by not rejecting the promise returned by close when the reset operation is done. Returning a rejected promise from close indicates the closing failed, which it doesn't sound like is accurate in that case.

[yutakahirano]

Reverted the edit. I'm not sure if we don't need to access abortReason.

[yutakahirano]

Tests are available at web-platform-tests/wpt#29310.

[yutakahirano]

Thank you! @domenic, do you have contact of streams implementers for Gecko/WebKit?

[domenic]

I suspect you might have better contacts than me at this point, given your work on WebTransport with Gecko and WebKit? But usually I ping @youennf and @jorendorff :)

[yutakahirano]

Thank you, I'll talk about this at the next WebTransport WG meeting.

[ricea]

Do we need an operation to add an algorithm to signal for use by other standards?

[yutakahirano]

I was planning to write something like: [=AbortSignal/add=] |algorithm| to |stream|.[[controller]].[[signal]].

Isn't that enough?

[ricea]

Isn't that enough?

It's certainly enough for now. We might want to change it in future so that other standards don't have to make references to WritableStreamDefaultController's internal slots.

[yutakahirano]

I realized that we need abortReason, as it may contain the code of RESET_STREAM. Hence I added WritableStreamDefaultController.abortReason in the last commit. @domenic, can you take a look again? I'll update the tests accordingly.

[yutakahirano]

@jan-ivar Could you express an implementer interest (Mozilla)? This is needed for w3c/webtransport#259 and w3c/webtransport#260.

[domenic]

LGTM

[jan-ivar]

Yes we have implementer interest in WebTransport, and therefore this issue.

[domenic]

Great!

The reference implementation is failing the tests at web-platform-tests/wpt#29310 indicating a problem either with the reference implementation or the tests. Let's get that straightened out, and then we have to do the usual merge-tests-update-submodule-merge spec dance.

[yutakahirano]

Fixed. Could you double-check?