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

ShutdownOutput for non-persistent HTTP/1 connections #12212

Closed
gregw opened this issue Aug 30, 2024 · 2 comments · Fixed by #12213 or #12216
Closed

ShutdownOutput for non-persistent HTTP/1 connections #12212

gregw opened this issue Aug 30, 2024 · 2 comments · Fixed by #12213 or #12216
Assignees
Labels
Bug For general bugs on Jetty side High Priority Sponsored This issue affects a user with a commercial support agreement

Comments

@gregw
Copy link
Contributor

gregw commented Aug 30, 2024

Jetty version(s)
12

Enhancement Description

Currently a needless exception is thrown by the HttpConnection class after every connection is closed:

2024-08-30 12:00:37.908:DEBUG:oejsi.HttpConnection:qtp1212899836-30: caught exception HttpConnection@28edcae7::SocketChannelEndPoint@35c31367[{l=/127.0.0.1:8080,r=/127.0.0.1:33516,OPEN,fill=-,flush=-,to=3/30000}{io=0/0,kio=0,kro=1}]->[HttpConnection@28edcae7[p=HttpParser{s=CLOSE,0 of -1},g=HttpGenerator@7c03111a{s=START}]=>HttpChannelState@72c4f0ea{handling=null, handled=false, send=SENDING, completed=false, request=null}] HttpChannelState@72c4f0ea{handling=null, handled=false, send=SENDING, completed=false, request=null}
org.eclipse.jetty.io.RuntimeIOException: Parser is terminated
	at org.eclipse.jetty.server.internal.HttpConnection.parseRequestBuffer(HttpConnection.java:559)
	at org.eclipse.jetty.server.internal.HttpConnection.onFillable(HttpConnection.java:384)
	at org.eclipse.jetty.io.AbstractConnection$ReadCallback.succeeded(AbstractConnection.java:322)
	at org.eclipse.jetty.io.FillInterest.fillable(FillInterest.java:99)
	at org.eclipse.jetty.io.SelectableChannelEndPoint$1.run(SelectableChannelEndPoint.java:53)
	at org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.runTask(AdaptiveExecutionStrategy.java:478)
	at org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.consumeTask(AdaptiveExecutionStrategy.java:441)
	at org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.tryProduce(AdaptiveExecutionStrategy.java:293)
	at org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.produce(AdaptiveExecutionStrategy.java:195)
	at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:979)
	at org.eclipse.jetty.util.thread.QueuedThreadPool$Runner.doRunJob(QueuedThreadPool.java:1209)
	at org.eclipse.jetty.util.thread.QueuedThreadPool$Runner.run(QueuedThreadPool.java:1164)
	at java.base/java.lang.Thread.run(Thread.java:1570)

This can be seen if debug is turned on for the following test server

    public static void main(String... args) throws Exception
    {
        Server server = new Server();

        ServerConnector connector = new ServerConnector(server);
        server.addConnector(connector);
        connector.setPort(8080);
        connector.setIdleTimeout(30000);
        HttpConfiguration httpConfiguration = connector.getConnectionFactory(HttpConnectionFactory.class).getHttpConfiguration();
        httpConfiguration.setHttpCompliance(HttpCompliance.RFC2616);
        httpConfiguration.setIdleTimeout(30000);

        ContextHandler context = new ContextHandler();
        server.setHandler(context);

        context.setHandler(new Handler.Abstract()
        {
            @Override
            public boolean handle(Request request, Response response, Callback callback) throws Exception
            {
                byte[] bytes = "Now is time for all good men to come to the aid of the party!\n".getBytes(StandardCharsets.UTF_8);
                response.write(true, BufferUtil.toBuffer(bytes), callback);
                return true;
            }
        });

        server.start();
        server.join();
    }

and then requests like the following are sent:

ardesia: ~/src/jetty-12.0.x/jetty-home/target/jetty-home/base (jetty-12.0.x)
[1991] telnet localhost 8080
Trying 127.0.0.1...
Connected to localhost.
Escape character is '^]'.
GET /
Now is time for all good men to come to the aid of the party!
Connection closed by foreign host.

ardesia: ~/src/jetty-12.0.x/jetty-home/target/jetty-home/base (jetty-12.0.x)
[1991] telnet localhost 8080
Trying 127.0.0.1...
Connected to localhost.
Escape character is '^]'.
GET / HTTP/1.0

HTTP/1.1 200 OK
Server: Jetty(12.0.13-SNAPSHOT)
Date: Fri, 30 Aug 2024 01:53:41 GMT
Content-Length: 62

Now is time for all good men to come to the aid of the party!
Connection closed by foreign host.

ardesia: ~/src/jetty-12.0.x/jetty-home/target/jetty-home/base (jetty-12.0.x)
[1991] telnet localhost 8080
Trying 127.0.0.1...
Connected to localhost.
Escape character is '^]'.
GET / HTTP/1.1
Host: localhost:8080
Connection: close

HTTP/1.1 200 OK
Server: Jetty(12.0.13-SNAPSHOT)
Date: Fri, 30 Aug 2024 01:54:18 GMT
Content-Length: 62
Connection: close

Now is time for all good men to come to the aid of the party!
Connection closed by foreign host.
@gregw
Copy link
Contributor Author

gregw commented Aug 30, 2024

@sbordet This issue was added in #9287 and it suppressed a real problem added with https://github.com/jetty/jetty.project/pull/9684/files#r1192999081 where we are now not always shutting down the output. Instead we now rely on the thrown exception

@gregw gregw added High Priority Bug For general bugs on Jetty side Sponsored This issue affects a user with a commercial support agreement and removed Enhancement labels Aug 30, 2024
@gregw gregw self-assigned this Aug 30, 2024
@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in Jetty 12.0.13 - FROZEN Aug 30, 2024
@gregw gregw changed the title Needless exceptions thrown for non-persistent HTTP/1 connections ShutdownOutput for non-persistent HTTP/1 connections Aug 30, 2024
@gregw
Copy link
Contributor Author

gregw commented Aug 30, 2024

I've reopened this issue, as I think there is a further problem with shutdownOutput.

In previous versions of jetty, we did the shutdown as the last write completed. We are now doing it as the handling callback is completed, which may be very much later than when the response is sent.
I think we should do the shutdown as soon as possible.

@sbordet thoughts?

@gregw gregw reopened this Aug 30, 2024
gregw added a commit that referenced this issue Sep 2, 2024
* Improve shutdown of non-persistent HTTP/1 connections

 + shutdown in SendCallback

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

---------

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Co-authored-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
Labels
Bug For general bugs on Jetty side High Priority Sponsored This issue affects a user with a commercial support agreement
Projects
No open projects
Status: ✅ Done
1 participant