MattiasBuelens / web-streams-polyfill

Web Streams, based on the WHATWG spec reference implementation
MIT License
289 stars 29 forks source link

signal with pre aborted #75

Closed jimmywarting closed 3 years ago

jimmywarting commented 3 years ago

In one of my test, this hangs indefinitely when using a abort signal that have already been canceled togheter with polyfill (this don't happen when using natives)

  const abortController = new AbortController()
  const signal = abortController.signal
  abortController.abort()
  const promise = rs.pipeTo(ws, { signal })
  err = await promise.catch(e=>e)
  // never gets here

version using 3.0.3 (edit: and 3.0.1 apparently)

MattiasBuelens commented 3 years ago

I can't seem to reproduce this. 😕

I tried adding your code as a unit test here, and I see err is set to a DOMException as expected. There's even a WPT test for this case, and the polyfill passes it without a problem.

jimmywarting commented 3 years ago

Found out what the problem was... i did use two different versions of the ponyfill Somehow everything worked out just fine, everything was interchangeable between different versions, it could be pipe/read/written to different versions. It could basically do everything. Everything except the pre aborted signal

This was the only test that failed when i used two different versions: https://github.com/jimmywarting/native-file-system-adapter/blob/1f0e85ea15d228f06ab56e6dc7ec5940b974e775/example/test.js#L389-L403 the rest worked out just fine

Here is a minimal reproducible test case

import('https://cdn.jsdelivr.net/npm/web-streams-polyfill@3.0.0/dist/ponyfill.es2018.mjs').then(async vers300 => {
  const vers301 = await import('https://cdn.jsdelivr.net/npm/web-streams-polyfill@3.0.1/dist/ponyfill.es2018.mjs')

  const rs = new vers300.ReadableStream()
  const ws = new vers301.WritableStream()
  const abortController = new AbortController()
  abortController.abort()
  rs.pipeTo(ws, { signal: abortController.signal })
})

...And a fiddle

Here are all the test i run: https://jimmywarting.github.io/native-file-system-adapter/example/test.html

jimmywarting commented 3 years ago

Really looking forward to #20 being resolved so i can use that instead. (just patching the missing pieces)

MattiasBuelens commented 3 years ago

Aaaah, I see. The classes from two different versions are seemingly compatible: all public properties and public methods are the same, and even the internal property names like _readableStreamController are unchanged. Except for one small but crucial difference: the private symbols such as [[PullSteps]] and [[ErrorSteps]].

Thus, when version 3.0.0's ReadableStreamPipeTo() wants to abort a WritableStream from version 3.0.1, it eventually tries to call stream._writableStreamController[ErrorSteps]() in WritableStreamFinishErroring. It will look up the ErrorSteps symbol from version 3.0.0, but that won't exist on the WritableStream create with version 3.0.1. So this function unexpectedly errors, and the pipe promise never resolves.

I suppose I could improve IsReadableStream to check that stream._readableStreamController is an instance of our own ReadableStreamDefaultController or ReadableByteStreamController, and similarly for IsWritableStream and our own WritableStreamDefaultController. 🤔

Really looking forward to #20 being resolved so i can use that instead. (just patching the missing pieces)

I know, I know. 😛

jimmywarting commented 3 years ago

That was half Greek/English to me. but Sound like you got it figured out 👍

possible a old duplicate of #56 that i closed and could never figure out what it was