trafficserver
trafficserver copied to clipboard
apache
Make sure the grace shutdown of HTTP/2 connections are completed
This PR addresses a few issues:
- If a
HTTP2_SESSION_EVENT_FINIis received after aHTTP2_SESSION_EVENT_SHUTDOWN_INIT, theGOAWAYframe with the correctlast_stream_idisn't sent out. - Correctly set and remove the
Connection: closeheader for HTTP/2 (this header is only used as a internal flag to indicate a grace shutdown is needed). - Initiate a grace shutdown when status is 429.
If a HTTP2_SESSION_EVENT_FINI is received after a HTTP2_SESSION_EVENT_SHUTDOWN_INIT, the GOAWAY frame with the correct last_stream_id isn't sent out. If a HTTP/2 session timed out, no grace shutdown is triggered.
Aren't these immediate close? Why do we want to initiate grace shutdown on timeout?
If a HTTP2_SESSION_EVENT_FINI is received after a HTTP2_SESSION_EVENT_SHUTDOWN_INIT, the GOAWAY frame with the correct last_stream_id isn't sent out. If a HTTP/2 session timed out, no grace shutdown is triggered.
Aren't these immediate close? Why do we want to initiate grace shutdown on timeout?
We got a complaint from a property saying they don't have a way to determine whether a connection is closed, since the HTTP/2 doesn't have the Connection: close header, sending a GOAWAY when that happens should give them what they want.
The problem with the HTTP2_SESSION_EVENT_FINI is , if we started the grace shutdown, I think we should finish the that process even if HTTP2_SESSION_EVENT_FINI is received, since the first step is sending a GOAWAY with INT32_MAX.
About that timeout part, was doing some testing and wasn't sure whether we need that, left it in there to get some comments.
Ok, I wonder why the property can't check if the connection is closed on TCP layer, but let's leave it aside.
The RFC says:
A server that is attempting to gracefully shut down a connection SHOULD send an initial GOAWAY frame with the last stream identifier set to 231-1 and a NO_ERROR code. This signals to the client that a shutdown is imminent and that initiating further requests is prohibited. After allowing time for any in-flight stream creation (at least one round-trip time), the server MAY send another GOAWAY frame with an updated last stream identifier. This ensures that a connection can be cleanly shut down without losing requests.
and
If a connection terminates without a GOAWAY frame, the last stream identifier is effectively the highest possible stream identifier.
So, a client cannot expect a GOAWAY. A connection close without GOAWAY can happen. I don't think current behavior of ATS is wrong.
That said, the RFC also says:
An endpoint MAY send multiple GOAWAY frames if circumstances change. For instance, an endpoint that sends GOAWAY with NO_ERROR during graceful shutdown could subsequently encounter a condition that requires immediate termination of the connection. The last stream identifier from the last GOAWAY frame received indicates which streams could have been acted upon.
I'd take this as "A server can cancel graceful shutdown and send a GOAWAY without waiting for the 1 RTT of grace time, to close a connection immediately". This may be just a different way of saying what you do on this PR, but I think this is what we should do, and I'd take a different approach.
In most cases, send_goaway_frame() is called right before we schedule HTTP2_SESSION_EVENT_FINI event. So if ATS receives HTTP2_SESSION_EVENT_FINI, GOAWAY should be sent. I don't think we need to forcibly progress the shutdown process (we can simply cancel it) to send the second GOAWAY. The only exception is Http2ClientSession::do_io_close. It schedules the event without calling send_goaway_frame(). But if the event is scheduled in do_io_close, it's probably too late to send something anyway.
TLDR; Isn't canceling graceful shutdown process enough?
@maskit forgot to mention one thing, when doing a curl command, if no GOAWAY frame is sent, it reports connection left intact even when ATS says the H2 session is destroyed. I'm not sure that is the correct behavior, or should we even care about that?
Closing a connection without sending GOAWAY is not polite, but I don't think it's a violation. Depends on why the connection was closed.
Endpoints SHOULD always send a GOAWAY frame before closing a connection so that the remote peer can know whether a stream has been partially processed or not. For example, if an HTTP client sends a POST at the same time that a server closes a connection, the client cannot know if the server started to process that POST request if the server does not send a GOAWAY frame to indicate what streams it might have acted on.
An endpoint might choose to close a connection without sending a GOAWAY for misbehaving peers.
The AuTest dns_host_down failed:
file /tmp/sandbox/dns_host_down/ts/log/error.log : DNS lookup should fail - Failed
Reason: Contents of /tmp/sandbox/dns_host_down/ts/log/error.log did not contains expression: "DNS Error: no valid server http://resolve.this.com/"
```
