sendError(int, String) behavior change between 9.4.20 and 9.4.21

cstamas commented 4 years ago

I detected the following -- IMHO wrong -- behavior change between releases 9.4.20 and 9.4.21.

Given this trivial servlet:

package org.cstamas.jetty.bug;

import java.io.IOException;

import javax.servlet.http.HttpServlet;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;

public class BugServlet
    extends HttpServlet
{
  @Override
  protected void doGet(final HttpServletRequest req, final HttpServletResponse resp)
      throws IOException
  {
    resp.setHeader("Content-Range", "*/100");
    resp.sendError(416, "bug");
  }
}

The two versions produce different response, 9.4.21 makes the added header and some other to disappear.

Expected output (produced by 9.4.20) -- as expected OK:

cstamas@Urnebes ~/tmp/jetty-bug $ curl -v http://localhost:8080/
*   Trying 127.0.0.1:8080...
* TCP_NODELAY set
* Connected to localhost (127.0.0.1) port 8080 (#0)
> GET / HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/7.66.0
> Accept: */*
> 
* Mark bundle as not supporting multiuse
< HTTP/1.1 416 bug
< Date: Fri, 04 Oct 2019 08:00:36 GMT
< Content-Range: */100
< Cache-Control: must-revalidate,no-cache,no-store
< Content-Type: text/html;charset=iso-8859-1
< Content-Length: 305
< Server: Jetty(9.4.20.v20190813)
< 
<html>
<head>
<meta http-equiv="Content-Type" content="text/html;charset=utf-8"/>
<title>Error 416 bug</title>
</head>
<body><h2>HTTP ERROR 416</h2>
<p>Problem accessing /. Reason:
<pre>    bug</pre></p><hr><a href="http://eclipse.org/jetty">Powered by Jetty:// 9.4.20.v20190813</a><hr/>

</body>
</html>
* Connection #0 to host localhost left intact
cstamas@Urnebes ~/tmp/jetty-bug $

Produced output by 9.4.21 -- unexpected:

cstamas@Urnebes ~/tmp/jetty-bug $ curl -v http://localhost:8080/
*   Trying 127.0.0.1:8080...
* TCP_NODELAY set
* Connected to localhost (127.0.0.1) port 8080 (#0)
> GET / HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/7.66.0
> Accept: */*
> 
* Mark bundle as not supporting multiuse
< HTTP/1.1 416 Range Not Satisfiable
< Cache-Control: must-revalidate,no-cache,no-store
< Content-Type: text/html;charset=iso-8859-1
< Content-Length: 421
< Server: Jetty(9.4.21.v20190926)
< 
<html>
<head>
<meta http-equiv="Content-Type" content="text/html;charset=utf-8"/>
<title>Error 416 bug</title>
</head>
<body><h2>HTTP ERROR 416 bug</h2>
<table>
<tr><th>URI:</th><td>/</td></tr>
<tr><th>STATUS:</th><td>416</td></tr>
<tr><th>MESSAGE:</th><td>bug</td></tr>
<tr><th>SERVLET:</th><td>bug</td></tr>
</table>
<hr><a href="http://eclipse.org/jetty">Powered by Jetty:// 9.4.21.v20190926</a><hr/>

</body>
</html>
* Connection #0 to host localhost left intact
cstamas@Urnebes ~/tmp/jetty-bug $

Problems:

response message not set
the set Content-Range header is gone
the Date is gone

cstamas commented 4 years ago

Seems it has to do with method javax.servlet.http.HttpServletResponse#sendError(int) vs javax.servlet.http.HttpServletResponse#sendError(int, java.lang.String). Former works same way in both 9.4.20 and 9.4.21, while latter produces this issue above.

joakime commented 4 years ago

If anything I would say the 9.4.20 behavior (and earlier) is the bug.

Some things to point out ...

In sendError(int, String) is an oddball, as the custom reason phrase is long ago deprecated.
In sendError(int, String) the reported reason phrase in the HTTP Response Line is now the spec declared phrase.
In sendError(int, String) the given custom reason phrase is now part of the response body (making it compatible to spec)
Both sendError(int, String) and sendError(int) are supposed to clear out any set response headers before it produces the error page. (except Set-Cookie)
Both sendError(int, String) and sendError(int) are supposed to offer the error page handling mechanism on the servlet context an opportunity to customize the error page based on the status code.

Your description of your environment doesn't mention any custom error page handling. That also means the Content-Range is cleared out properly in 9.4.21, and is a bug in 9.4.20.

What's left ...

The fact that the Content-Range header sticks around if using .sendError(int) is a possible bug. The missing Date header is also a curious one.

cstamas commented 4 years ago

Ok, agree with first 3.

But re rest two: if so, then how would one create response like described here https://tools.ietf.org/html/rfc7233#section-4.4?

HTTP/1.1 416 Range Not Satisfiable
Date: Fri, 20 Jan 2012 15:41:54 GMT
Content-Range: bytes */47022

Date more or less, but Content-Range should be added by my code, no? And stick to this example above, no error page, just this one servlet declared in web.xml.

cstamas commented 4 years ago

Also, just FTR, the method in javax.servlet:javax.servelet-api:3.0.1 artifact Jetty uses does NOT have that method deprecated.

https://github.com/javaee/servlet-spec/blob/b48a5f6d7c6762f310f85ada39c3f2a9cb8233a8/src/main/java/javax/servlet/http/HttpServletResponse.java#L200

but yes, javadoc for it really says that "The server will preserve cookies and may clear or update any headers needed to serve the error page as a valid response"

cstamas commented 4 years ago

Currently to achieve desired response with 9.4.21 is only possible with use of response.setStatus(416) instead of response.sendError(416) but this seems wrong to me: 416 is (client) error

joakime commented 4 years ago

But re rest two: if so, then how would one create response like described here https://tools.ietf.org/html/rfc7233#section-4.4?
HTTP/1.1 416 Range Not Satisfiable
Date: Fri, 20 Jan 2012 15:41:54 GMT
Content-Range: bytes */47022

Don't use either of the HttpServletResponse.sendError() methods, those have a declared meaning and behavior (see javadoc) that you don't want.

Use .setStatus(int) and headers directly, this will also not result in a Servlet redispatch on DispatcherType.ERROR, or a custom error page handling on status code 416.

package jetty;

import java.io.IOException;
import java.io.InputStream;
import java.net.HttpURLConnection;
import java.net.URI;
import javax.servlet.ServletException;
import javax.servlet.http.HttpServlet;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;

import org.eclipse.jetty.server.Server;
import org.eclipse.jetty.server.handler.DefaultHandler;
import org.eclipse.jetty.server.handler.HandlerList;
import org.eclipse.jetty.servlet.DefaultServlet;
import org.eclipse.jetty.servlet.ServletContextHandler;
import org.eclipse.jetty.util.IO;

import static java.nio.charset.StandardCharsets.UTF_8;

public class RangeHandlingServletDemo
{
    public static void main(String[] args) throws Exception
    {
        Server server = new Server(9090);
        ServletContextHandler context = new ServletContextHandler();
        context.setContextPath("/");
        context.addServlet(RangeHandlingServlet.class, "/demo");
        context.addServlet(DefaultServlet.class, "/"); // handle static content and errors for this context
        HandlerList handlers = new HandlerList();
        handlers.addHandler(context);
        handlers.addHandler(new DefaultHandler()); // handle non-context errors
        server.setHandler(context);
        server.start();

        try
        {
            demonstrateErrorHandling(server.getURI().resolve("/"));
        }
        finally
        {
            server.stop();
        }
    }

    private static void demonstrateErrorHandling(URI serverBaseUri) throws IOException
    {
        HttpURLConnection http = (HttpURLConnection) serverBaseUri.resolve("/demo").toURL().openConnection();
        dumpRequestResponse(http);
        System.out.println();
        try (InputStream in = http.getInputStream())
        {
            System.out.println(IO.toString(in, UTF_8));
        }
    }

    private static void dumpRequestResponse(HttpURLConnection http) throws IOException
    {
        System.out.println();
        System.out.println("----");
        System.out.printf("%s %s HTTP/1.1%n", http.getRequestMethod(), http.getURL());
        System.out.println("----");
        System.out.printf("%s%n", http.getHeaderField(null));
        http.getHeaderFields().entrySet().stream()
            .filter(entry -> entry.getKey() != null)
            .forEach((entry) -> System.out.printf("%s: %s%n", entry.getKey(), http.getHeaderField(entry.getKey())));
    }

    public static class RangeHandlingServlet extends HttpServlet
    {
        @Override
        protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException
        {
            resp.setStatus(416);
            resp.setHeader("Content-Range", "*/100");
            resp.addHeader("X-Example", "Yeah, your range isn't that great");
        }
    }
}

That results in ...

2019-10-04 09:08:47.151:INFO::main: Logging initialized @206ms to org.eclipse.jetty.util.log.StdErrLog
2019-10-04 09:08:47.227:INFO:oejs.Server:main: jetty-9.4.21.v20190926; built: 2019-09-26T16:41:09.154Z; git: 72970db61a2904371e1218a95a3bef5d79788c33; jvm 1.8.0_202-b08
2019-10-04 09:08:47.269:INFO:oejsh.ContextHandler:main: Started o.e.j.s.ServletContextHandler@36d4b5c{/,null,AVAILABLE}
2019-10-04 09:08:47.284:INFO:oejs.AbstractConnector:main: Started ServerConnector@2a3046da{HTTP/1.1,[http/1.1]}{0.0.0.0:9090}
2019-10-04 09:08:47.284:INFO:oejs.Server:main: Started @340ms

----
GET http://127.0.1.1:9090/demo HTTP/1.1
----
HTTP/1.1 416 Range Not Satisfiable
Server: Jetty(9.4.21.v20190926)
Content-Range: */100
Content-Length: 0
X-Example: Yeah, your range isn't that great
Date: Fri, 04 Oct 2019 14:08:47 GMT

2019-10-04 09:08:47.367:INFO:oejs.AbstractConnector:main: Stopped ServerConnector@2a3046da{HTTP/1.1,[http/1.1]}{0.0.0.0:9090}
2019-10-04 09:08:47.368:INFO:oejsh.ContextHandler:main: Stopped o.e.j.s.ServletContextHandler@36d4b5c{/,null,UNAVAILABLE}
Exception in thread "main" java.io.IOException: Server returned HTTP response code: 416 for URL: http://127.0.1.1:9090/demo
    at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
    at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
    at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
    at java.lang.reflect.Constructor.newInstance(Constructor.java:423)
    at sun.net.www.protocol.http.HttpURLConnection$10.run(HttpURLConnection.java:1944)
    at sun.net.www.protocol.http.HttpURLConnection$10.run(HttpURLConnection.java:1939)
    at java.security.AccessController.doPrivileged(Native Method)
    at sun.net.www.protocol.http.HttpURLConnection.getChainedException(HttpURLConnection.java:1938)
    at sun.net.www.protocol.http.HttpURLConnection.getInputStream0(HttpURLConnection.java:1508)
    at sun.net.www.protocol.http.HttpURLConnection.getInputStream(HttpURLConnection.java:1492)
    at jetty.RangeHandlingServletDemo.demonstrateErrorHandling(RangeHandlingServletDemo.java:52)
    at jetty.RangeHandlingServletDemo.main(RangeHandlingServletDemo.java:38)
Caused by: java.io.IOException: Server returned HTTP response code: 416 for URL: http://127.0.1.1:9090/demo
    at sun.net.www.protocol.http.HttpURLConnection.getInputStream0(HttpURLConnection.java:1894)
    at sun.net.www.protocol.http.HttpURLConnection.getInputStream(HttpURLConnection.java:1492)
    at sun.net.www.protocol.http.HttpURLConnection.getHeaderField(HttpURLConnection.java:3051)
    at jetty.RangeHandlingServletDemo.dumpRequestResponse(RangeHandlingServletDemo.java:64)
    at jetty.RangeHandlingServletDemo.demonstrateErrorHandling(RangeHandlingServletDemo.java:50)
    ... 1 more

Process finished with exit code 1

The .sendError(int, String) is related to .setStatus(int, String), and if you read the javadoc ...

https://docs.oracle.com/javaee/7/api/javax/servlet/http/HttpServletResponse.html#setStatus-int-java.lang.String-

You'll see that the reason phrase is deprecated.

joakime commented 4 years ago

and here's the .sendError(int) approach.

package jetty;

import java.io.IOException;
import java.io.InputStream;
import java.net.HttpURLConnection;
import java.net.URI;
import javax.servlet.DispatcherType;
import javax.servlet.RequestDispatcher;
import javax.servlet.ServletException;
import javax.servlet.http.HttpServlet;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;

import org.eclipse.jetty.server.Server;
import org.eclipse.jetty.server.handler.DefaultHandler;
import org.eclipse.jetty.server.handler.HandlerList;
import org.eclipse.jetty.servlet.DefaultServlet;
import org.eclipse.jetty.servlet.ErrorPageErrorHandler;
import org.eclipse.jetty.servlet.ServletContextHandler;
import org.eclipse.jetty.util.IO;
import org.eclipse.jetty.util.StringUtil;

import static java.nio.charset.StandardCharsets.UTF_8;

public class ServletCustom416ErrorPageDemo
{
    public static void main(String[] args) throws Exception
    {
        Server server = new Server(9090);
        ServletContextHandler context = new ServletContextHandler();
        context.setContextPath("/");
        context.addServlet(RangeHandlingServlet.class, "/demo");
        context.addServlet(ErrorsServlet.class, "/errors");
        context.addServlet(DefaultServlet.class, "/"); // handle static content and errors for this context

        ErrorPageErrorHandler errorPageErrorHandler = new ErrorPageErrorHandler();
        errorPageErrorHandler.addErrorPage(HttpServletResponse.SC_REQUESTED_RANGE_NOT_SATISFIABLE, "/errors");
        context.setErrorHandler(errorPageErrorHandler);

        HandlerList handlers = new HandlerList();
        handlers.addHandler(context);
        handlers.addHandler(new DefaultHandler()); // handle non-context errors
        server.setHandler(context);
        server.start();

        try
        {
            demonstrateErrorHandling(server.getURI().resolve("/"));
        }
        finally
        {
            server.stop();
        }
    }

    private static void demonstrateErrorHandling(URI serverBaseUri) throws IOException
    {
        HttpURLConnection http = (HttpURLConnection)serverBaseUri.resolve("/demo").toURL().openConnection();
        dumpRequestResponse(http);
        System.out.println();
        try (InputStream in = http.getInputStream())
        {
            System.out.println(IO.toString(in, UTF_8));
        }
    }

    private static void dumpRequestResponse(HttpURLConnection http) throws IOException
    {
        System.out.println();
        System.out.println("----");
        System.out.printf("%s %s HTTP/1.1%n", http.getRequestMethod(), http.getURL());
        System.out.println("----");
        System.out.printf("%s%n", http.getHeaderField(null));
        http.getHeaderFields().entrySet().stream()
            .filter(entry -> entry.getKey() != null)
            .forEach((entry) -> System.out.printf("%s: %s%n", entry.getKey(), http.getHeaderField(entry.getKey())));
    }

    public static class RangeHandlingServlet extends HttpServlet
    {
        @Override
        protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException
        {
            if (assertCanHandleRange(req, resp))
            {
                // process range
            }
        }

        private boolean assertCanHandleRange(HttpServletRequest req, HttpServletResponse resp) throws IOException
        {
            // test if range is possible here
            req.setAttribute("Error.Bad-Range", "bytes */100");
            resp.sendError(HttpServletResponse.SC_REQUESTED_RANGE_NOT_SATISFIABLE);
            return false; // for example, we always return false
        }
    }

    public static class ErrorsServlet extends HttpServlet
    {
        @Override
        protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException
        {
            if (req.getDispatcherType() != DispatcherType.ERROR)
            {
                // direct access of errors servlet is a 404.
                // it should only be accessed by ERROR dispatch.
                resp.setStatus(HttpServletResponse.SC_NOT_FOUND);
                return;
            }

            int statusCode = (int)req.getAttribute(RequestDispatcher.ERROR_STATUS_CODE);
            switch (statusCode)
            {
                case HttpServletResponse.SC_REQUESTED_RANGE_NOT_SATISFIABLE:
                    resp.setStatus(statusCode);
                    String badRange = (String)req.getAttribute("Error.Bad-Range");
                    if (StringUtil.isNotBlank(badRange))
                    {
                        resp.setHeader("Content-Range", badRange);
                    }
                    resp.setHeader("X-Example", "Proof that Error Dispatch did what it's designed to do");
                    return;
                default:
                    resp.setStatus(statusCode);
                    return;
            }
        }
    }
}

which results in ...

2019-10-04 09:24:32.232:INFO::main: Logging initialized @190ms to org.eclipse.jetty.util.log.StdErrLog
2019-10-04 09:24:32.305:INFO:oejs.Server:main: jetty-9.4.21.v20190926; built: 2019-09-26T16:41:09.154Z; git: 72970db61a2904371e1218a95a3bef5d79788c33; jvm 1.8.0_202-b08
2019-10-04 09:24:32.348:INFO:oejsh.ContextHandler:main: Started o.e.j.s.ServletContextHandler@5034c75a{/,null,AVAILABLE}
2019-10-04 09:24:32.363:INFO:oejs.AbstractConnector:main: Started ServerConnector@567d299b{HTTP/1.1,[http/1.1]}{0.0.0.0:9090}
2019-10-04 09:24:32.363:INFO:oejs.Server:main: Started @322ms

----
GET http://127.0.1.1:9090/demo HTTP/1.1
----
HTTP/1.1 416 Range Not Satisfiable
Server: Jetty(9.4.21.v20190926)
Content-Range: bytes */100
Content-Length: 0
X-Example: Proof that Error Dispatch did what it's designed to do

2019-10-04 09:24:32.446:INFO:oejs.AbstractConnector:main: Stopped ServerConnector@567d299b{HTTP/1.1,[http/1.1]}{0.0.0.0:9090}
2019-10-04 09:24:32.448:INFO:oejsh.ContextHandler:main: Stopped o.e.j.s.ServletContextHandler@5034c75a{/,null,UNAVAILABLE}
Exception in thread "main" java.io.IOException: Server returned HTTP response code: 416 for URL: http://127.0.1.1:9090/demo
    at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
    at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
    at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
    at java.lang.reflect.Constructor.newInstance(Constructor.java:423)
    at sun.net.www.protocol.http.HttpURLConnection$10.run(HttpURLConnection.java:1944)
    at sun.net.www.protocol.http.HttpURLConnection$10.run(HttpURLConnection.java:1939)
    at java.security.AccessController.doPrivileged(Native Method)

gregw commented 4 years ago

@cstamas We cleaned up the sendError handling significantly in 9.4.21. The main difference between setStatus and sendError is that the later actually provides content for the response - either from a default error page of the container or from an error page dispatch to a defined error page within the application.

So the new implementation of sendError is a much better at working out what headers it has to clear - specifically any headers that are related to the content of the response. In this case it is removing the Content-Range header, because it cannot know if that header was meant to be for partial content that was just about to be sent... or if it is the header that a 416 response is allowed to send. It's a bit of a special case!

So as @joakime has pointed out - you either need to use setStatus OR define an 416 error page handler that will generate the error page for you and put in the Content-Range header as part of generating the error page rather than before calling sendError.

but..... see next comment....

gregw commented 4 years ago

We could perhaps better support the special case that is a 416 response by:

in HttpResponse.resetContent we could keep Content-Range header IFF the status code was 416 and the range starts with '*/'
we make 416 a no body response, so we never dispatch to an error page for it.

This would well handle this special case, but break anybody that is generating a 416 response in an error handler.

I'm in two minds.... our own 416 generation uses setStatus.

@joakime @sbordet thoughts?

cstamas commented 4 years ago

@gregw Thanks. I opted to "adopt jetty way" so change to use setStatus as well :smile:

joakime commented 4 years ago

@gregw I would expect that any request that reaches the context should go through the ErrorHandler of that Context if sendError() is used.

joakime commented 4 years ago

@cstamas the recommendations are more "the servlet way" then the jetty way.

gregw commented 4 years ago

@joakime if sendError is used:

the HttpChannelState is flagged as sendError==true and the current dispatch cycle is allowed to complete.
then the context and server are querried for an ErrorHandler
if no error handler is found OR the error code can't have a body, then a minimal response is sent
if an error page mapping error handler is found it is asked if there is an error page
if an error page is found, then an ERROR dispatch is done via the normal handler path.
otherwise the ErrorHandler handle is directly called to handle

cstamas commented 4 years ago

@joakime well, in my case, Servlet API was always backed by Jetty, and what the code produced so far did change in this respect :smile: I did not change servlet API nor code calling it, just upgraded Jetty from 9.4.20 to 9.4.21. (I agree, that the code I talk about may be wrong in first place, and basically as Joakim says, "I would say the 9.4.20 behavior (and earlier) is the bug:).

martinmeeser commented 4 years ago

My CustomError handler (extending from errorhandler) is broken with 9.4.21.

All reasons send with sendError(i, str) are null.

I do not get the reasoning behind the change at all. Agree that the update is wrong.

I find the 9.4.20 behavior good as it was, made sense, now does not make sense. Where is the string? If i want to show it to the client the way I want it to, how can I?

This seems impossible now?

(unhappy)

martinmeeser commented 4 years ago

In addition: In the jetty docs it says:

"An ErrorHandler may extend the ErrorHandler class and may totally replace the handle method to generate an error page ... "

This is what I do:

public class WebApiErrorHandler extends ErrorHandler {

    @Override
    public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response)
            throws IOException {

        int rc = response.getStatus();
        String title = String.format("%s (%d)", HttpStatus.getMessage(rc), rc);

        org.eclipse.jetty.server.Response jettyResponse = (org.eclipse.jetty.server.Response) response;
        String reason = jettyResponse.getReason();

        String determinedContentType = WebApi.determineContentType(request, response);

        String responseStr;
        switch (determinedContentType) {
            case "text/html": {
                responseStr = _webApi.getHtmlHelp(title, reason);
                break;
            }
            case "application/json": {
                responseStr = String.format("{ 'returnCode': %d, 'reason': '%s', 'greeting': '%s'}", rc, reason, _webApi.getTextGreeting());
                break;
            }
            case "text/plain": {
                responseStr = String.format("(%d) %s\n%s\n", rc, reason, _webApi.getTextGreeting());
                break;
            }
            default: {
                responseStr = String.format("(%d) %s\n%s\n", rc, reason, _webApi.getTextGreeting());
                break;
            }
        }

        response.getWriter().write(responseStr);
    }

    public WebApiErrorHandler(WebApi webApi) {
        _webApi = webApi;
    }

    private final WebApi _webApi;

}

This is not possible anymore. Gets not called at all or reason is null, shows "null" then in the results.

joakime commented 4 years ago

That's not how an error handler works.

You get redispatched to the Error handler, on the DispatcherType.ERROR. The prior dispatch details on why you are in the error handler is available on the HttpServletRequest attributes. See https://docs.oracle.com/javaee/7/api/javax/servlet/RequestDispatcher.html and the constants it has starting with ERROR_ (those are the relevant attribute keys)

paulmillar commented 3 years ago

@joakime @gregw I do wonder why this change was introduced to the stable 9.4 branch, and not left as part of the jetty-10 release. It breaks applications that were expected (however incorrectly) that sendError(int, String) results in a custom status reason phrase. It is also a shame that this breaking change doesn't seem to be described in the v9.4.21 release notes.

These points aside, I would like advice on how an application can specify a custom reason phrase when returning an error.

The following seems to work:

response.setStatus(code, reason);
response.sendError(code, reason);

However, I'm reluctant to use this approach as setStatus(int, String) is deprecated.

joakime commented 3 years ago

(Adding this response here too)

@paulmillar the reason phrase existed for special class of user-agent called interactive http clients (also known as "interactive text client").

Which is a special kind of HTTP user-agent where the user is actually using the protocol directly. (Think of it as a terminal / console tool similar to a SQL tool where the user is entering SQL commands and getting responses)

Examples of interactive textual http clients ...

This also means that no web browser is considered an interactive textual http client.

Over the course of time, the HTTP spec has evolved away from making things easier for interactive textual http clients. Stricter format rules, no more line folding allowed, whitespace rules are now strict, dropping of TEXT rule, delimiters are far more strict, etc ...

The RFC 7231 even tells you about this change in the reason phrase ...

The reason-phrase element exists for the sole purpose of providing a textual description associated with the numeric status code, mostly out of deference to earlier Internet application protocols that were more frequently used with interactive text clients. A client SHOULD ignore the reason-phrase content.

It existed for interactive text clients, and the client should ignore the reason phrase.

RFC 7231 also made the rules for reason phrase stricter over RFC2616 by dropping the TEXT rule. Which pretty much means it only supports 7-bit US-ASCII (with no encoding support). This change to the RFC7230 spec, dropping of the TEXT rule, also made supporting custom reason phrases increasingly impossible.

If you were providing description in the reason phrase for your clients, then you are not probably aware of RFC 7807.

https://tools.ietf.org/html/rfc7807 - Problem Details for HTTP APIs

When the specs for HTTP/2 and HTTP/3 entered the picture, the support this kind of interactive client was dropped entirely. The features present in the HTTP spec specifically for those kinds of clients were dropped. (eg: http version, reason phrase, etc)

gregw commented 3 years ago

@paulmillar we will make such breaking changes in a stable branch if it is a matter of spec compliance and best practises. We can't say to many users for whom an upgrade to 10 is a very major step, that they must do so in order to obtain compliance with an RFC, spec or current best practises. Often when we do so, if there are known usages that we will break, we put in a backwards compliance mode.

In this case, there are two breaking aspects: the reason string and what headers are cleared when sendError is called. Reason strings should not be significant to any application, hence we didn't consider a backwards compatible mode. The range header for a 416 is a bit different matter and you can see we have discussed better supporting it. We've not yet done so because the OP was satisfied with doing it a different way.

As for how to set the status, to keep within the spec, you have to use setStatus(int,String). It is deprecated precisely because implementations cannot guarantee that the reason will actually be set by the underlying HTTP transport. If you are using jetty APIs, we provide setStatusWithReason(int, String) as we do support setting the reason if you really want - it is just that best practise now is to not allow applications to do so.

paulmillar commented 3 years ago

Thanks @joakime @gregw for you quick and detailed replies.

You are correct @joakime : I wasn't aware of RFC 7807. As an initial impression, I like it, and will definitely look further to see how it could be incorporate in the future. However, the major deployment environment for "my" software is shared with several other software implementations, so change can only be achieved through community consensus building (I can't just change something). Rolling out protocol changes can be a multi-year endeavour.

My own use-case is, perhaps slightly different from how you described. Rather than interactive textual clients, the clients making the requests are automated (non-interactive) but they log the complete status line. By including a custom reason phrase, these log files contain information that would otherwise require manual inspection and correlation of log files of services run by different organisations. This can save considerable time when diagnosing problems. Of course, this would also be possible by adopting RFC 7807, but that's not something I can do quickly.

As it happens, the restrictions imposed by RFC 7231 (single line, 7-bit clean) shouldn't be a burden for the kind of messages being sent.

Thanks to your information, I now see how to resolve the problem and continue to provide custom reason phrases, while investigating the adoption of RFC 7807 (or something similar).

jetty / jetty.project

sendError(int, String) behavior change between 9.4.20 and 9.4.21 #4154