fails-components / webtransport

Http/3 webtransport support for node
Other
149 stars 21 forks source link

Using promises for callbacks may miss promise rejections? #259

Open achingbrain opened 7 months ago

achingbrain commented 7 months ago

From what I can see, the general pattern in this module is:

  1. Userland invokes a JS function
  2. The JS function creates a pendingoperation promise and a pendingres function that will resolve that promise
  3. JS calls out to C++
  4. C++ calls back to an onXXX-style function
  5. That function calls this.pendingres to resolve the promise if it's not null

I see a problem with this approach in that it depends on the user awaiting the result of each promise before interacting with the module again.

The WebTransport spec has an example where this is not the case:

for (const bytes of data) {
  await writer.ready;
  writer.write(bytes).catch(() => {});
}
await writer.close();

Here we call .write on the stream writer, which sets up a new .pendingoperation, crucially we don't wait for the .write promise to resolve - this is the recommended interaction from the streams spec.

We then call .close which sets up a new .pendingoperation promise, but there's no guarantee the one created during the previous .write has been resolved yet.

If there's a write error here the caller will never know because the .close function has overwritten the this.pendingres function that would reject the promise returned from .write.

A more reliable approach might be to pass a classic callback to C++ and do the promise-conversion in the JS layer?

So .close on the stream would become something like:

close: async () => {
  if (this.writeableclosed) {
    return
  }

  return new Promise((resolve, reject) => {
    this.objint.streamFinal((err) => {
      if (err != null) {
        reject(err)
        return
      }

     resolve()
    }
  })
},

This way all promises will resolve/reject and the code becomes a bit simpler to follow because you return to the original JS call site when the C++ function finishes?

martenrichter commented 7 months ago

I am not sure if this is a problem. The close function in the writable is not identical to the close function of the writer.

Also, I have implemented the pendingoperation precisely as it is defined by spec: https://www.w3.org/TR/webtransport/#send-stream-internal-slots https://www.w3.org/TR/webtransport/#send-stream-procedures

So if you see a problem here, please report it to the W3C webtransport folks. As you are right, it is unclear, what happens, if a write is already in progress.

martenrichter commented 7 months ago

Please see: https://developer.mozilla.org/en-US/docs/Web/API/WritableStream/WritableStream it states that close is only called after all queued-up writes are completed. So it should only be a problem, it the WritableStream is not implemented correctly.