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

Graceful close of HTTP/2 Connection #2788

Closed
gregw opened this issue Aug 9, 2018 · 10 comments · Fixed by #4554
Closed

Graceful close of HTTP/2 Connection #2788

gregw opened this issue Aug 9, 2018 · 10 comments · Fixed by #4554
Assignees

Comments

@gregw
Copy link
Contributor

gregw commented Aug 9, 2018

The Connection: close header is not supported by the HTTP/2 protocol, so currently if it is set in a request or a response, it is simply not sent and ignored.
However, adding Connection: close header to a response is used by Jetty and applications to indicate that the connection should be closed (due to errors, do to shutdown etc.)

So should we also provide a mechanism that will note the Connection:close headers as they are stripped out and switch the h2 connection into a shutdown state, where it will go_away the next time the stream count reaches 0?
Or should we provide some other shutdown mechanism?

@sbordet
Copy link
Contributor

sbordet commented Aug 13, 2018

@gregw if Connection: close is added to headers for a HTTP/2 stream, that in no way should affect the state of the HTTP/2 session. The Connection header must be stripped out as per specification.

If the server is shutting down, the HTTP/2 implementation stops all HTTP/2 sessions so that they queue a GOAWAY and flush it. There is no Graceful.shutdown() mechanism where we wait for streams to complete. There is a built-in mechanism in GOAWAY (via lastStreamId) that tells the client what was the last stream processed by the server.

Other than implementing Graceful I don't think we should do anything with the Connection header.

@gregw gregw changed the title Connection: close is ignored by HTTP/2 Graceful close of HTTP/2 Connection Aug 14, 2018
@gregw
Copy link
Contributor Author

gregw commented Aug 14, 2018

So I have changed the name of this issue. We probably need the ability to gracefully close a HTTP/2 connection (aka session).
For HTTP/1, graceful shutdown currently uses Connection:close to inform the client and the server that the connection should be closed after the response. So that header could be used as a hint to invoke a http/2 graceful mechanism.... whatever that is. But I guess the first problem is to come up with what the mechanism is?

@jjestrel
Copy link

@gregw is this on the roadmap? do you have suggestions for implementation?

@sbordet
Copy link
Contributor

sbordet commented Aug 1, 2019

@jjestrel we have not looked at it, it's more of a low priority feature request that is hard to implement, so little gain for big effort.
What is exactly your use case?

@gregw I imagine a case where a browser will request a number of resources concurrently to render a page, and in only one of those requests it puts a Connection: close under the (old) assumption that it was using HTTP/1.1 so it has a number of connections already opened and one of them may be closed.

In HTTP/2 the fact that one request has Connection: close does not mean that I want to close the multiplexed connection. In fact in the example above the browser would keep a number of HTTP/1.1 connections opened to the server, just not all of them.

If it is the client initiating the close, I don't see why it cannot stop sending request on that connection and then close it.

If it is the server initiating the close, then perhaps we could have some kind of gracefulness, but while we can reset new incoming requests, how long are we going to wait for requests that are being processed? That seems to be more of an application logic rather than a container logic.

We could introduce a Jetty specific error code so that for a new request that is reset with this new error code the client will know that the server is closing the connection and won't issue other requests, but clients typically multiplex a batch of requests that have meaning together: what if some of them are processed and some are failed due to graceful close?

Seems to me that the complication of handling well all cases is way larger than the small benefits that it brings - I could be convinced otherwise with a detailed use case.

@jjestrel
Copy link

jjestrel commented Aug 1, 2019

We are using h2 where few clients send many requests to the server. When we do a new deploy we try to gracefully shutdown the server but currently observe timeouts from the client side. We suspect it's because we don't stop accepting requests. I've confirmed StatisticsHandler is installed and the graceful shutdown code in there is getting run

@swankjesse
Copy link
Contributor

HTTP/2 intends for GOAWAY frames to be used for graceful shutdown.

The GOAWAY frame (type=0x7) is used to initiate shutdown of a
connection or to signal serious error conditions. GOAWAY allows an
endpoint to gracefully stop accepting new streams while still
finishing processing of previously established streams. This enables
administrative actions, like server maintenance.

Http2Connection.close() attempts to initiate a graceful shutdown:

@Override
public void close()
{
    // We don't call super from here, otherwise we close the
    // endPoint and we're not able to read or write anymore.
    session.close(ErrorCode.NO_ERROR.code, "close", Callback.NOOP);
}

But there’s a problem in HTTP2Session.java:

case GO_AWAY:
{
    // We just sent a GO_AWAY, only shutdown the
    // output without closing yet, to allow reads.
    getEndPoint().shutdownOutput();
    break;
}

Shutting down the output stream prevents Jetty from shutting down gracefully! In particular, in-flight requests cannot be responded to. Similarly, inbound pings cannot be responded to. As soon as the output is shutdown the game is lost.

@sbordet
Copy link
Contributor

sbordet commented Sep 3, 2019

@swankjesse so what if a in-flight request takes 20 minutes because it's stuck in a RDBMS query?

The wish of "finish the processing of previously established streams" now requires a timeout - how long are you willing to wait for the streams to finish?
So while I understand what the spec is aiming to, I think it's a bit naive as the server should protect itself from nasty clients that may leverage this behavior.

I think this is an application behavior rather than a container behavior, as different applications may apply different logics.

@swankjesse
Copy link
Contributor

I completely agree that 20-minute calls need not be shut down gracefully.

I’m working on a set of microservices that make lots of ~10 ms calls. Some constraints I’m trying to satisfy:

  • Predictable, low latency. A call that takes 10 ms should almost always take 10 ms and never 20 ms.
  • Frequent redeploys. We want to roll out new software many times a day.
  • Dynamic cluster size. Our server gives connections a lifetime of 5–15 minutes. Occasional disconnects cause clients to automatically distribute load across the cluster.

Abrupt HTTP/2 shutdowns mean that clients need to retry calls, which wrecks predictable latency. Graceful HTTP/2 shutdowns would allow servers to restart and redistribute load without any failed calls.

@gregw
Copy link
Contributor Author

gregw commented Sep 5, 2019

@sbordet I disagree. If an application is taking 20 minutes to process a request, then it will take that 20 minutes regardless of if the output stream is open or we can actually use the results of those spent resources. I can't see how allowing an existing session to send a response after a GO_AWAY has been sent is a DOS vector at all.

The server should be able to send a GO_AWAY, then new streams are ignored and existing streams left to complete. When the stream count goes to 0 the connection is closed.

@gregw
Copy link
Contributor Author

gregw commented Sep 9, 2019

@sbordet let's hangout on this issue this week.

swankjesse pushed a commit to cashapp/misk that referenced this issue Sep 10, 2019
Waiting on graceful shutdown in Jetty
jetty/jetty.project#2788
sbordet added a commit that referenced this issue Sep 15, 2019
Implemented HTTP2Session.shutdown() to shutdown the session
gracefully with a timeout.
It's a draft because there are a number of issues to be resolved.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
sbordet added a commit that referenced this issue Feb 6, 2020
Made HTTP2SessionContainer implement Graceful, so that it can be found
in the component hierarchy and can shutdown all sessions.
Modified HTTP2ServerSession to reject requests if already closed/shutdown.
Implemented shutdown in HTTP2Session.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet sbordet removed the Question label Feb 6, 2020
sbordet added a commit that referenced this issue Feb 7, 2020
Updates after review.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
sbordet added a commit that referenced this issue Mar 10, 2020
Updates after review.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
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.

4 participants