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

Client initiated drain #436

Open
wilaw opened this issue Nov 23, 2022 · 19 comments · May be fixed by #606
Open

Client initiated drain #436

wilaw opened this issue Nov 23, 2022 · 19 comments · May be fixed by #606

Comments

@wilaw
Copy link
Contributor

wilaw commented Nov 23, 2022

As noted in #432 , per IETF a client may send a DRAIN_WEBTRANSPORT_SESSION signal to a server, to indicate that the server SHOULD NOT initiate new WebTransport streams or datagram flows.

Do we need a wt.drain() method to initiate this signal?

Would write() and stream creation methods work after drain() has been called?

@wilaw wilaw added the Discuss at next meeting Flags an issue to be discussed at the next WG working label Nov 23, 2022
@aboba
Copy link
Collaborator

aboba commented Nov 23, 2022

It might be helpful to have a concrete use case that cannot be satisfied by the existing API.

Within the existing API, the client can stop accepting incoming undirectional or bidirectional steams, while continuing to read from streams that were previously accepted. In the IETF issue, there is a question about whether a drain request implies that the server should stop sending datagrams or just stop creating "new work" (however, the server defines that).

It is also possible for an application to define its own messages. For example, an RTP over WebTransport implementation could use the PAUSE indication defined in RFC 7728 to request that its peer pause the sending of an RTP stream, which could be transported in unidirectional stream(s) or datagrams.

@wilaw
Copy link
Contributor Author

wilaw commented Nov 23, 2022

With the existing API, the client can stop accepting incoming streams. However it cannot stop the server from sending data across existing streams. The client can close() the existing streams of course, but then it risks losing data in flight.

@aboba
Copy link
Collaborator

aboba commented Nov 23, 2022

This sounds like a "graceful shutdown" use case, where the client is asking the server to finish the existing work but not initiate new work, so that the client can take care of the data in flight (e.g. write it to stable storage) and then close the session.

It does lead me to wonder how quickly the server is expected to wrap things up. Should the server stop sending immediately, or can it continue to a logical break point (e.g. finish sending the current video, but don't keep sending more videos after that)?

@wilaw
Copy link
Contributor Author

wilaw commented Nov 23, 2022

The IETF definition is intentionally vague on the timing question, with a SHOULD to "gracefully terminate the session as quickly as possible".

To drain a WebTransport session, either endpoint can send a DRAIN_WEBTRANSPORT_SESSION capsule. An endpoint SHOULD NOT initiate new WebTransport streams or datagram flows after sending or receiving a DRAIN_WEBTRANSPORT_SESSION capsule. Both endpoints can continue sending data on existing streams and datagram flows, but SHOULD attempt to gracefully terminate the session as quickly as possible.

We could use similar language and guidance if we do want to implement a drain() method.

@LPardue
Copy link

LPardue commented Nov 23, 2022

I think a lot of this is likely going to be application or deployment specific. So staying vague, or giving non-exhaustive examples, is the appropriate way to phrase it.

While rejecting new things is always possible. Draining helps both sides avoid pointless work. And as per the IETF issue ietf-wg-webtrans/draft-ietf-webtrans-http3#27 it can be used by intermediaries that are not application aware if they have operational needs.

@aboba
Copy link
Collaborator

aboba commented Dec 16, 2022

@vasilvv Do you have an opinion on this?

@jan-ivar
Copy link
Member

Meeting:

  • client can only send DRAIN, server can send DRAIN or GOAWAY
  • server to client, you didn't want to stop new stuff. It's not stopping anything, just giving advanced notice, the client is going to close this session. What is the server going to do? Just wait for the close?
  • Example: a client archiver app might not lose streams in flight, but uses drain() to tell server to stop creating new streams.
  • Ask IETF if this is what you want?

@wilaw wilaw added IETF dependency and removed Discuss at next meeting Flags an issue to be discussed at the next WG working labels Feb 15, 2023
@wilaw
Copy link
Contributor Author

wilaw commented May 11, 2023

At IETF 116, WebTransport meeting, room feedback was to allow client initiated drain.

@wilaw wilaw added the Discuss at next meeting Flags an issue to be discussed at the next WG working label Oct 18, 2023
@jan-ivar
Copy link
Member

jan-ivar commented Oct 25, 2023

Some questions:

  • Is this just a synchronous drain() method that sends the capsule each time?
    • If called a million times does it send 1) one time, 2) a million times? Up to UA?
  • Is the method idempotent or throw if called again?
  • Should we add a new state called "draining"?
  • Should this "draining" state block new stream creation?

@vasilvv
Copy link
Contributor

vasilvv commented Oct 27, 2023

Is this just a synchronous drain() method that sends the capsule each time?

We only handle incoming DRAIN capsule once, so it would make to send one only once. We could make it return a promise, but I am not sure what users would do with it.

Is the method idempotent or throw if called again?

Idempotent should have better ergonomics.

Should we add a new state called "draining"?

Unlike HTTP, we don't really alter connection behavior upon draining signal, so I'm not sure that's necessary.

Should this "draining" state block new stream creation?

HTTP does that. I think we should avoid that, since that makes some assumptions about how the application works that are not necessary.

@jan-ivar
Copy link
Member

This seems like something that would be nice but not necessary, but It exists in IETF so we should probably expose it.

@jan-ivar
Copy link
Member

We only handle incoming DRAIN capsule once, so it would make to send one only once. ...

If DRAIN is only sent once, what guarantee is there that the server received it?

@jan-ivar
Copy link
Member

Meeting:

  • Drain is over the control stream which is reliable.
  • Ready for PR

@jan-ivar jan-ivar assigned wilaw and unassigned vasilvv Feb 13, 2024
@wilaw wilaw removed the Discuss at next meeting Flags an issue to be discussed at the next WG working label Feb 14, 2024
@wilaw wilaw assigned nidhijaju and unassigned wilaw Mar 27, 2024
@wilaw wilaw added this to the Candidate Recommendation milestone Mar 27, 2024
@jan-ivar
Copy link
Member

jan-ivar commented Jun 5, 2024

Based on #436 (comment) and "either endpoint can send a DRAIN_WEBTRANSPORT_SESSION capsule" the likely API need here is something like:

wt.drain(); // void, no promise
await wt.draining; // will resolve in the next task
// we are now in "draining" state.

...probably with something like:

@nidhijaju do you have cycles to add something like this? Or we can reassign.

@nidhijaju nidhijaju linked a pull request Jun 19, 2024 that will close this issue
@ekinnear
Copy link
Contributor

ekinnear commented Jun 20, 2024

Should we add a new state called "draining"?

Unlike HTTP, we don't really alter connection behavior upon draining signal, so I'm not sure that's necessary.

Should this "draining" state block new stream creation?

HTTP does that. I think we should avoid that, since that makes some assumptions about how the application works that are not necessary.

It feels weird that we don't block new stream creation upon draining. I know in the IETF spec we said this was "up to the application", which I guess is fine, but did we mean the website itself or did we mean the browser/W3C spec here?

If we mean the actual website itself, why can't it just send its own message to signal draining, i.e. why specify a capsule/API at all?

@aboba
Copy link
Collaborator

aboba commented Jun 21, 2024

It feels weird that we don't block new stream creation upon draining. I know in the IETF spec we said this was "up to the application", which I guess is fine, but did we mean the website itself or did we mean the browser/W3C spec here?

IMHO, "application" refers to both the client's Javascript application as well as server-side code, for either the server -> client or client -> server cases.

If we mean the actual website itself, why can't it just send its own message to signal draining, i.e. why specify a capsule/API at all?

The application can send its own message, so it's a good question. The stream that the message would be sent over will differ but does that make much difference?

Perhaps this is about convenience, saving the JS client application from needing to write their own drain() method, and allowing a node application to react to receipt of DRAIN_WEBTRANSPORT_SESSION ?

@martinthomson
Copy link
Member

I'm with @ekinnear here. HTTP has a GOAWAY frame because HTTP is an application protocol and people use HTTP directly. However, we didn't add that capability to QUIC, so why would we add it here? (CONNECTION_CLOSE is not a graceful shutdown mechanism, it's an abrupt termination signaling mechanism, for error conditions.)

That line of thinking is making me less than enthusiastic about DRAIN_WEBTRANSPORT_SESSION. If the intermediate stack layers (the browser) can't act on the signal in a meaningful way, then it's not a useful signal at that layer.

@aboba
Copy link
Collaborator

aboba commented Jun 21, 2024

@jan-ivar
Copy link
Member

jan-ivar commented Jul 2, 2024

Meeting:

  • No strong interest in this meeting
  • Might a use cases be proxies, need a way to reboot? Folks to reach out to @afrind

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

Successfully merging a pull request may close this issue.

8 participants