search

jetty / jetty.project

Eclipse Jetty® - Web Container & Clients - supports HTTP/2, HTTP/1.1, HTTP/1.0, websocket, servlets, and more
https://eclipse.dev/jetty
Other
3.86k stars 1.91k forks source link

Improve connection closing when stopping #12435

Open sbordet opened 2 weeks ago

sbordet commented 2 weeks ago

Jetty version(s) 12

Description When stopping a Server, the ManagedSelector is stopped (as part of being a descendant bean of Server).

Currently, ManagedSelector.doStop() submits a CloseConnections update, and then waits for it, then submits a StopSelector update, and waits for it.

Both these updates iterate over the keys to call close(), but likely the close() operation is asynchronous -- it certainly is in HTTP/2 where a GOAWAY frame is submitted to be written, but it is also in HTTP/1 after the changes in #12395.

The idea would be to have a "pre-close" action, and we can use Graceful to shutdown and receive back a CompletableFuture. So, the CloseConnections update would call shutdown() on the Connection, and the StopSelector action would call EndPoint.close(). This would allow for asynchronous actions to happen in Connection.shutdown() and be waited for by ManagedSelector, and for EndPoint.close() to forcibly close.

The alternative would be to make the action in [HTTP1,2,3]Connection.close() to be run synchronously, but it may block.

sbordet commented 2 weeks ago

@lorban @gregw thoughts?

gregw commented 2 weeks ago

An idealized shutdown would be: 1) stop the connectors accepting new connections or requests 2) stop all the contexts and finish the existing requests 3) stop all the connectors

I think this is what we do when we are graceful?

sbordet commented 1 week ago

@gregw, currently:

  1. If stopTimeout > 0, then Graceful.shutdown() is called; typically the only Gracefuls present are the connectors. This would result in the connectors stopping accepting new connections, and waiting until the connector does not have any EndPoint tracked (because all of them have been closed).
  2. Stop all connectors; this results in stopping the SelectorManager, which in turn calls Connection.close() and then EndPoint.close().
  3. Stop all the beans.

So apparently bullet 2 should just be removed, as stopping all the beans happens in reverse order, and therefore contexts are stopped before the connectors.

I'll try and prepare a PR about this to see what breaks.

sbordet commented 1 week ago

A further problem is that, per Servlet Specification, idle timeouts are ignored for suspended requests.

This means if there is a suspended request (e.g. long poll), stopping the Server results in the connector to set the EndPoint.idleTimeout to the AbstractConnector.shutdownIdleTimeout, but this shorter idle timeout is ignored anyway.

The next interaction with the application is when the SelectorManager calls Connection.close() (see first comment), where we are back to the original problem: the application is about to write a 500 to the client, but because this action is asynchronous, the SelectorManager has already closed the EndPoint.

sbordet commented 1 day ago

For HTTP/1 HttpConnection.close() has been restored to be synchronous in https://github.com/jetty/jetty.project/commit/9151518a839d7bdac42ba58227d2fd4db7a31d75.

Still an open issue for HTTP/2 and HTTP/3.