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.84k stars 1.91k forks source link

Jetty 12 - Response.writeError should close the connection on terminal responses (like 503) #9176

Closed joakime closed 1 month ago

joakime commented 1 year ago

Jetty version(s) Jetty 12

Description Some code snippets look like this ...

if (!response.isCommitted())
{
    if (LOG.isDebugEnabled())
        LOG.debug("Service Unavailable (before wrapped process): {}", request.getHttpURI());
    response.getHeaders().put(HttpHeader.CONNECTION, HttpHeaderValue.CLOSE);
    Response.writeError(request, response, callback, HttpStatus.SERVICE_UNAVAILABLE_503);
}

In order to not have to specify Connection: close for things like HTTP/2 or HTTP/3, some errors should be terminal and close the connection.

gregw commented 1 year ago

It is difficult to define "terminal". In this case, there is nothing about a 503 that is terminal from a protocol point of view. However, it is likely that this is being done as part of a graceful shutdown, so from an operational point of view the connection should be closed. I can't see how this distinction can be made at a lower level.

gregw commented 1 year ago

Probably not needed as a Connector should know if it is in shutdown mode and do a protocol specific shutdown mechanism. No need for application layer to say close.

joakime commented 1 year ago

How about altering the core API to include a boolean for closeConnection?

boolean closeConnection = true;
Response.writeError(request, response, callback, closeConnection, HttpStatus.SERVICE_UNAVAILABLE_503);
sbordet commented 1 year ago

@joakime no, as @gregw says, it is the implementation (by looking at how the request has been processed, for example whether the content has been read) and the Connector (when it is shutting down) that knows whether to close the connection.

Otherwise, for a 503 response, we can keep the connection open in HTTP/1.1.

Adding a parameter that only works for HTTP/1.1 does not seem a good idea.

joakime commented 1 year ago

The problem still exists, and still needs a http version neutral solution. Don't get hung up on "close connection", that's just draft terminology for the desire to terminate the exchange. It needs better language to fit this http version neutral need.

It can easily be ...

boolean terminateResponse = true; // on http/1.1 it will close the connection, on http/2, and http/3 it will close the stream.
Response.writeError(request, response, callback, terminateResponse, HttpStatus.SERVICE_UNAVAILABLE_503);
sbordet commented 1 year ago

I'm saying that there is no need for an application to "terminate the exchange". What would that mean in HTTP/2?

gregw commented 1 year ago

I also do not think this is needed. The application should be mostly neutral with regards to any connection semantics. The writeError method itself will terminate the connection (reset) if necessary.

github-actions[bot] commented 1 month ago

This issue has been automatically marked as stale because it has been a full year without activity. It will be closed if no further activity occurs. Thank you for your contributions.

sbordet commented 1 month ago

Closing as not needed.