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

Fixes #11679 - Jetty 12.0.8 seems to leak connection when it encounters earlyEOF. #11719

Merged
merged 7 commits into from
May 3, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ public Runnable onIdleTimeout(TimeoutException t)
}

// If a write call is pending, take the writeCallback to fail below.
Runnable invokeWriteFailure = _response.lockedFailWrite(t, false);
Runnable invokeWriteFailure = _response.lockedFailWrite(t);

// If there was a pending IO operation, deliver the idle timeout via them.
if (invokeOnContentAvailable != null || invokeWriteFailure != null)
Expand Down Expand Up @@ -432,7 +432,7 @@ public Runnable onFailure(Throwable x)
_onContentAvailable = null;

// If a write call is in progress, take the writeCallback to fail below.
Runnable invokeWriteFailure = _response.lockedFailWrite(x, true);
Runnable invokeWriteFailure = _response.lockedFailWrite(x);

// Notify the failure listeners only once.
Consumer<Throwable> onFailure = _onFailure;
Expand Down Expand Up @@ -1120,12 +1120,12 @@ private boolean lockedIsWriting()
return _writeCallback != null;
}

private Runnable lockedFailWrite(Throwable x, boolean fatal)
private Runnable lockedFailWrite(Throwable x)
sbordet marked this conversation as resolved.
Show resolved Hide resolved
{
assert _request._lock.isHeldByCurrentThread();
Callback writeCallback = _writeCallback;
_writeCallback = null;
if (writeCallback != null || fatal)
if (writeCallback != null)
sbordet marked this conversation as resolved.
Show resolved Hide resolved
{
if (_writeFailure == null)
_writeFailure = x;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1057,7 +1057,7 @@ public void earlyEOF()
HttpStreamOverHTTP1 stream = _stream.get();
if (stream != null)
{
BadMessageException bad = new BadMessageException("Early EOF");
EofException bad = new EofException("Early EOF");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like that this sends a 500. I accept that perhaps the request is not bad and it is a connection problem, but it is certainly not a server problem as the 500 suggests.
How about:

Suggested change
EofException bad = new EofException("Early EOF");
BadMessageException bad = new BadMessageException(null, new EofException("Early EOF"));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be a type of EOF exception because that's what it is.
I have introduced an HttpEofException so it is converted to a 400, but it is-a IOException, and it's quiet because it's a client close, so won't clutter the logs.

Content.Chunk chunk = stream._chunk;

if (Content.Chunk.isFailure(chunk))
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
//
// ========================================================================
// Copyright (c) 1995 Mort Bay Consulting Pty Ltd and others.
//
// This program and the accompanying materials are made available under the
// terms of the Eclipse Public License v. 2.0 which is available at
// https://www.eclipse.org/legal/epl-2.0, or the Apache License, Version 2.0
// which is available at https://www.apache.org/licenses/LICENSE-2.0.
//
// SPDX-License-Identifier: EPL-2.0 OR Apache-2.0
// ========================================================================
//

package org.eclipse.jetty.ee10.servlet;

import java.io.IOException;
import java.net.InetSocketAddress;
import java.nio.ByteBuffer;
import java.nio.channels.SocketChannel;

import jakarta.servlet.AsyncContext;
import jakarta.servlet.ServletException;
import jakarta.servlet.ServletInputStream;
import jakarta.servlet.ServletOutputStream;
import jakarta.servlet.http.HttpServlet;
import jakarta.servlet.http.HttpServletRequest;
import jakarta.servlet.http.HttpServletResponse;
import org.eclipse.jetty.http.HttpHeader;
import org.eclipse.jetty.http.HttpStatus;
import org.eclipse.jetty.http.HttpTester;
import org.eclipse.jetty.io.EofException;
import org.eclipse.jetty.server.Server;
import org.eclipse.jetty.server.ServerConnector;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.Test;

import static java.nio.charset.StandardCharsets.UTF_8;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.is;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;

public class EarlyEOFTest
{
private Server server;
private ServerConnector connector;

private void start(HttpServlet servlet) throws Exception
{
server = new Server();
connector = new ServerConnector(server);
server.addConnector(connector);

ServletContextHandler context = new ServletContextHandler("/ctx");
context.addServlet(servlet, "/path/*");
server.setHandler(context);

server.start();
}

@AfterEach
public void dispose() throws Exception
{
server.stop();
}

@Test
public void testEarlyEOF() throws Exception
{
start(new HttpServlet()
{
@Override
protected void service(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException
{
AsyncContext asyncContext = request.startAsync();

ServletInputStream input = request.getInputStream();
assertEquals('0', input.read());

// Early EOF.
assertThrows(EofException.class, input::read);

// Must be able to send a response.
response.setStatus(HttpStatus.ACCEPTED_202);
ServletOutputStream output = response.getOutputStream();
output.print("out");
output.close();

asyncContext.complete();
}
});

try (SocketChannel channel = SocketChannel.open(new InetSocketAddress("localhost", connector.getLocalPort())))
{
String request = """
POST /ctx/path/early HTTP/1.1\r
Host: localhost\r
Content-Length: 10\r

0""";
channel.write(UTF_8.encode(request));
// Close output before sending the whole content.
channel.shutdownOutput();

HttpTester.Response response = HttpTester.parseResponse(channel);

assertThat(response.getStatus(), is(HttpStatus.ACCEPTED_202));
assertTrue(response.contains(HttpHeader.CONNECTION, "close"));
assertEquals("out", response.getContent());

// Connection must be closed by server.
assertEquals(-1, channel.read(ByteBuffer.allocate(512)));
}
}
}