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

Can we feed the [[OutgoingDatagramsQueue]] without prematurely resolving write promises? #400

Open
jan-ivar opened this issue May 24, 2022 · 7 comments
Labels
hard Things to discuss and think about

Comments

@jan-ivar
Copy link
Member

jan-ivar commented May 24, 2022

@yutakahirano wrote in #21 (comment):

pkt_departure — I believe await writer.write(data); gives you this?

... the promise returned by writeDatagrams gives you effectively nothing.

This seems like an algorithm bug. [[OutgoingDatagramsQueue]] says: "A queue of tuples of an outgoing datagram, a timestamp and a promise which is resolved when the datagram is sent or discarded."

But that last description gels poorly with what we've written writeDatagrams to actually do: "If the length of datagrams.[[OutgoingDatagramsQueue]] is less than datagrams.[[OutgoingDatagramsHighWaterMark]], then resolve promise with undefined."

I sort of remember this: we were trying to feed the [[OutgoingDatagramsQueue]] by prematurely resolving the write promises. But is this the correct/only way? I worry we've created our own HighwaterMark algorithm, and should instead be using something in the streams spec here, but I don't know what that is or that the correct spec hooks exist. @ricea or @MattiasBuelens do you know?

In theory, there are two promises it seems a sink should be able to control 1) the one from .ready and 2) the one from .write:

  for (const datagram of datagrams) {
    await writer.ready; // 1
    writer.write(datagram).catch(() => {}); // 2
  }

If we could control 1 here, it might leave 2 to retain its original meaning of "when the datagram is sent or discarded."

@MattiasBuelens
Copy link

At the moment, writer.ready resolves if and only if writer.desiredSize > 0, i.e. if [[queueTotalSize]] < [[strategyHWM]]. By default, [[strategyHWM]] = 1, but this means that the writable will always try to fill its queue with at least one chunk, which may not be desirable. But if you set [[strategyHWM]] = 0, then writer.ready can never resolve...

I'm not too familiar with WebTransport, but maybe you can try passing datagrams.[[OutgoingDatagramsHighWaterMark]] as highWaterMark when setting up the WritableStream? 🤷‍♂️

If that doesn't work, then you'll have to wait for whatwg/streams#1190. With that, you'll be able to set highWaterMark = 0, and use controller.releaseBackpressure() (name to be decided) to manually resolve writer.ready whenever the sink considers itself ready to accept more chunks.

@jan-ivar
Copy link
Member Author

Meeting:

  • We're wondering, rn the write promise resolves with undefined. Could it resolve with, say, a timestamp and a boolean saying whether it was sent or discarded? Or does the streams spec not allow for this?

@ricea
Copy link
Contributor

ricea commented May 25, 2022

The streams spec requires the promise from writer.write() to resolve with undefined. I don't know what the original motivation for this was.

The reason we implement our own outgoing queue for datagrams is that we need to able to remove expired datagrams from the queue. If we used the WritableStream built-in queue then new datagrams could be stuck behind arbitrarily stale datagrams.

@yutakahirano
Copy link
Contributor

Yes, WebTransport datagrams is an unreliable sink, which is different from a typical WritableStream sink. Adding an option to remove the queue in WritableStream may be acceptable (to whatwg/streams), and that may be useful for other sinks - for example if the sink wants to batch chunks.

Even with this, we need to care about the distinction between the return value of writer.write() and writer.ready. Currently we recommend web developers to use the latter.

  for (const datagram of datagrams) {
    await writer.ready;
    writer.write(datagram).catch(() => {});
  }

We need more than releaseBackpressure() to maintain the recommendation.

See also: wpt/webtransport/datagrams.https.any.js

@wilaw wilaw added the Discuss at next meeting Flags an issue to be discussed at the next WG working label Jun 8, 2022
@jan-ivar
Copy link
Member Author

The reason we implement our own outgoing queue for datagrams is that we need to able to remove expired datagrams from the queue.

Yes, though in theory at least, this seems like it could have been done at the point of sending. E.g.

  1. we're ready to send, so pull one datagram off the send queue (affecting upstream desiredSize)
  2. if it's expired, discard it (resolving any related promise) and goto 1
  3. send it (resolving any related promise once sent)

I suspect the reason has more to do with needing a send queue on a different thread/process and feeding it efficiently.

Adding an option to remove the queue in WritableStream may be acceptable (to whatwg/streams)

Maybe, or could we get away with relying on the whatwg queue in the meantime and adding a big spec note saying that "oh by the way, this sink's queue is special and should be optimized to be on a different thread/process?"

Even with this, we need to care about the distinction between the return value of writer.write() and writer.ready.

Yes, I think this is the important point, that the outcome we want seems desirable: the observable behavior of the promises from writer.write() and writer.ready are consistent with WHATWG queues having been used.

@jan-ivar
Copy link
Member Author

jan-ivar commented Jun 21, 2022

Meeting:

  • Victor to check that for datagrams at least the Chrome implementation resolves write() when datagram is put on network send queue. Answer: it resolves when it leaves the network send queue (succeed writing into UDP socket)

@jan-ivar
Copy link
Member Author

Meeting:

  • Datagrams are using the streams spec in an unusual way to feed our send queue.
  • Solving this without help from the streams spec may be hard

@wilaw wilaw removed the Discuss at next meeting Flags an issue to be discussed at the next WG working label Aug 17, 2022
@wilaw wilaw added this to the No milestone milestone Aug 17, 2022
@jan-ivar jan-ivar added the hard Things to discuss and think about label Mar 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hard Things to discuss and think about
Projects
None yet
Development

No branches or pull requests

5 participants