Test or specification problem with TransformStream readable.cancel and calling controller.error

[evilpie]

What is the issue with the Streams Standard?

There is a test for TansformStream cancel (#1283) called

readable.cancel() and a parallel writable.close() should reject if a transformer.cancel() calls controller.error()

https://github.com/web-platform-tests/wpt/blob/4ab37a2aa733a03162ce2d5e3782d5da7940c330/streams/transform-streams/cancel.any.js#L79

I think that test is wrong or the specification for TransformStreamDefaultSourceCancelAlgorithm is wrong.
We can observe in that test that the promise for the cancel callback is going to be fulfilled. So we are in

Step 7.1. If cancelPromise was fulfilled, then:

The next step is

If writable.[[state]] is "errored", reject controller.[[finishPromise]] with writable.[[storedError]].

I think this step is not going to be taken, because the [[state]] is "erroring". As set by controller.error, via WritableStreamStartErroring.

The whole path is

• TransformStreamDefaultControllerError
• TransformStreamError
• TransformStreamErrorWritableAndUnblockWrite
• WritableStreamDefaultControllerErrorIfNeeded
• WritableStreamDefaultControllerError
• WritableStreamStartErroring

So this could be fixed by changing the condition to

If writable.[[state]] is "errored" or "erroring"

Or of course adjusting the test to except a different promise resolution.

@lucacasonato @MattiasBuelens

[lucacasonato]

That's interesting. This test passes in the reference implementation and in Deno's implementation. Is it possible your implementation diverges from the spec elsewhere?

[lucacasonato]

I'll investigate which exact code path is taken here in the reference implementation next week!

[debadree25]

Faced something similar in the nodejs implementation too have described it here nodejs/node#50126 (comment) it weird that the for the reference implementation and deno the promises for start and cancel resolve in the following order

• Start promise
• cancel promise
• cancel promise resolved
• start promise resolved

and hence the same error isnt faced, I thought it must be because of some of the promise transformations maybe? but cant put my finger on it

[saschanaz]

My observation on Gecko side matches what @debadree25 saw: the start promise in SetUpWritableStreamDefaultController is created first but the cancel promise in TransformStreamDefaultSourceCancelAlgorithm responds first. I'm unsure why is that 🤔

[debadree25]

My hunch is maybe we missed some promise transformations but I tried applying all the promise transformation methods used in the spec like uponPromise, etc. but didn't make a difference 😅

[saschanaz]

Our investigation so far found that:

1. InitializeTransformStream defines a start algorithm that returns startPromise.
2. That start algorithm is called by SetUpWritableStreamDefaultController and is used to create another startPromise at step 15 and 16
3. The step 15 uses "a promise resolved with", which creates a new promise that waits for the input promise in this case.
4. So the listener of startPromise of WritableStream takes two round to run here.
5. Since the listener of cancelPromise of TransformStreamDefaultSourceCancelAlgorithm only takes one round, it runs earlier than the start promise.

But I haven't found why the start promise listener still runs earlier than the cancel promise listener in the reference implementation.

[saschanaz]

Hmm, logging transformerDict.cancel at

streams/reference-implementation/lib/abstract-ops/transform-streams.js

Line 143 in 007d729

cancelAlgorithm = reason => transformerDict.cancel.call(transformer, reason);

shows this:

  function invokeTheCallbackFunction(reason) {
    const thisArg = utils.tryWrapperForImpl(this);
    let callResult;

    try {
      callResult = Reflect.apply(value, thisArg, [reason]);

      callResult = new Promise(resolve => resolve(callResult));
      return callResult;
    } catch (err) {
      return Promise.reject(err);
    }
  }

And the returned promise is unresolved, which means listening on this promise takes an extra round. That's webidl2js layer, not sure why it's implemented like this, I should check the spec and also try updating webidl2js to v17 (#1297, but it didn't change the behavior).

[MattiasBuelens]

@saschanaz The WebIDL specification (as currently written) requires this.

In step 13 of invoke a callback function type, the call result is converted to an IDL value. This must be an IDL Promise, whose conversion is defined as:

1. Let promiseCapability be ? NewPromiseCapability(%Promise%).
2. Perform ? Call(promiseCapability.[[Resolve]], undefined, « V »).
3. Return promiseCapability.

It would be nice if this could use the PromiseResolve abstract op instead, but then the "shape" of an IDL Promise type would change. Right now, it's a Promise Capability Record, with [[Promise]], [[Resolve]] and [[Reject]] internal slots. But PromiseResolve only returns the promise... Maybe we could give it no-op resolve and reject functions, since they won't be used anyway?

[debadree25]

I tried updating the usage of Promise.resolve() -> new Promise(r => r(value)) for setupWritableStreamDefaultController that resolves the present test cases but breaks another new one readable.cancel() and a parallel writable.close() should reject if a transformer.cancel() calls controller.error() 🫤 so i guess would have no other option than to find out all instances of wrong usage of promise and align it to how the spec wants 🤔