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

Using promises for callbacks may miss promise rejections? #259

Open
achingbrain opened this issue Mar 4, 2024 · 2 comments
Open

Using promises for callbacks may miss promise rejections? #259

achingbrain opened this issue Mar 4, 2024 · 2 comments

Comments

@achingbrain
Copy link
Contributor

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
Copy link
Member

martenrichter commented Mar 4, 2024

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants