Skip to content

Commit

Permalink
Fixes #11259 - HTTP/2 connection not closed after idle timeout when T…
Browse files Browse the repository at this point in the history 
…CP congested.

Now upon the second idle timeout, the connection is forcibly closed.
Fixed also similar problem in HTTP/3.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
  • Loading branch information
@sbordet
sbordet committed Jan 12, 2024
1 parent a9e564a commit fa14d99
Show file tree
Hide file tree
Showing 9 changed files with 378 additions and 60 deletions.
22 changes: 13 additions & 9 deletions ...re/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Session.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1909,6 +1909,7 @@ private boolean onIdleTimeout()
{
String reason = "idle_timeout";
boolean notify = false;
boolean terminate = false;
boolean sendGoAway = false;
GoAwayFrame goAwayFrame = null;
Throwable cause = null;
Expand All @@ -1923,10 +1924,9 @@ private boolean onIdleTimeout()
return false;
notify = true;
}

// Timed out while waiting for closing events, fail all the streams.
case LOCALLY_CLOSED ->
{
// Timed out while waiting for closing events, fail all the streams.
if (goAwaySent.isGraceful())
{
goAwaySent = newGoAwayFrame(ErrorCode.NO_ERROR.code, reason);
Expand All @@ -1946,15 +1946,19 @@ private boolean onIdleTimeout()
zeroStreamsAction = null;
failure = cause = new TimeoutException("Session idle timeout expired");
}
default ->
{
if (LOG.isDebugEnabled())
LOG.debug("Already closed, ignored idle timeout for {}", HTTP2Session.this);
return false;
}
default -> terminate = true;
}
}

if (terminate)
{
if (LOG.isDebugEnabled())
LOG.debug("Already closed, ignored idle timeout for {}", HTTP2Session.this);
// Writes may be TCP congested, so termination never happened.
flusher.abort(new TimeoutException());
return false;
}

if (notify)
{
boolean confirmed = notifyIdleTimeout(HTTP2Session.this);
Expand Down Expand Up @@ -2036,7 +2040,7 @@ private void onWriteFailure(Throwable x)

private void sendGoAwayAndTerminate(GoAwayFrame frame, GoAwayFrame eventFrame)
{
sendGoAway(frame, Callback.from(Callback.NOOP, () -> terminate(eventFrame)));
sendGoAway(frame, Callback.from(() -> terminate(eventFrame)));
}

private void sendGoAway(GoAwayFrame frame, Callback callback)
Expand Down
61 changes: 58 additions & 3 deletions ...-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/IdleTimeoutTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,11 @@

package org.eclipse.jetty.http2.tests;

import java.net.InetSocketAddress;
import java.nio.ByteBuffer;
import java.nio.channels.SelectionKey;
import java.nio.channels.SocketChannel;
import java.time.Duration;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.TimeoutException;
Expand All @@ -33,8 +37,11 @@
import org.eclipse.jetty.http2.frames.GoAwayFrame;
import org.eclipse.jetty.http2.frames.HeadersFrame;
import org.eclipse.jetty.http2.frames.ResetFrame;
import org.eclipse.jetty.http2.server.HTTP2CServerConnectionFactory;
import org.eclipse.jetty.http2.server.HTTP2ServerConnectionFactory;
import org.eclipse.jetty.io.Content;
import org.eclipse.jetty.io.ManagedSelector;
import org.eclipse.jetty.io.SocketChannelEndPoint;
import org.eclipse.jetty.server.Handler;
import org.eclipse.jetty.server.HttpConfiguration;
import org.eclipse.jetty.server.Request;
Expand All @@ -50,6 +57,7 @@

import static org.awaitility.Awaitility.await;
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.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNull;
Expand Down Expand Up @@ -747,10 +755,10 @@ public void onReset(Stream stream, ResetFrame frame, Callback callback)
await().atMost(5, TimeUnit.SECONDS).until(() -> ((HTTP2Session)client).updateSendWindow(0), Matchers.greaterThan(0));

// Wait for the server to finish serving requests.
await().atMost(5, TimeUnit.SECONDS).until(handled::get, Matchers.is(0));
assertThat(requests.get(), Matchers.is(count - 1));
await().atMost(5, TimeUnit.SECONDS).until(handled::get, is(0));
assertThat(requests.get(), is(count - 1));

await().atMost(5, TimeUnit.SECONDS).until(responses::get, Matchers.is(count - 1));
await().atMost(5, TimeUnit.SECONDS).until(responses::get, is(count - 1));
}

@Test
Expand Down Expand Up @@ -837,6 +845,53 @@ public void onHeaders(Stream stream, HeadersFrame frame)
assertTrue(responseLatch.await(5, TimeUnit.SECONDS));
}

@Test
public void testIdleTimeoutWhenCongested() throws Exception
{
long idleTimeout = 1000;
HTTP2CServerConnectionFactory h2c = new HTTP2CServerConnectionFactory(new HttpConfiguration());
prepareServer(h2c);
server.removeConnector(connector);
connector = new ServerConnector(server, 1, 1, h2c)
{
@Override
protected SocketChannelEndPoint newEndPoint(SocketChannel channel, ManagedSelector selectSet, SelectionKey key)
{
SocketChannelEndPoint endpoint = new SocketChannelEndPoint(channel, selectSet, key, getScheduler())
{
@Override
public boolean flush(ByteBuffer... buffers)
{
// Fake TCP congestion.
return false;
}

@Override
protected void onIncompleteFlush()
{
// Do nothing here to avoid spin loop,
// since the network is actually writable,
// as we are only faking TCP congestion.
}
};
endpoint.setIdleTimeout(getIdleTimeout());
return endpoint;
}
};
connector.setIdleTimeout(idleTimeout);
server.addConnector(connector);
server.start();

prepareClient();
httpClient.start();

InetSocketAddress address = new InetSocketAddress("localhost", connector.getLocalPort());
// The connect() will complete exceptionally.
http2Client.connect(address, new Session.Listener() {});

await().atMost(Duration.ofMillis(5 * idleTimeout)).until(() -> connector.getConnectedEndPoints().size(), is(0));
}

private void sleep(long value)
{
try
Expand Down
62 changes: 23 additions & 39 deletions ...re/jetty-http3/jetty-http3-common/src/main/java/org/eclipse/jetty/http3/HTTP3Session.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -218,8 +218,7 @@ private CompletableFuture<Void> goAway(GoAwayFrame frame)
long error = HTTP3ErrorCode.REQUEST_CANCELLED_ERROR.code();
String reason = "go_away";
failStreams(stream -> true, error, reason, true, new ClosedChannelException());
terminate();
outwardDisconnect(error, reason);
terminateAndDisconnect(error, reason);
}
return CompletableFuture.completedFuture(null);
}
Expand Down Expand Up @@ -489,18 +488,12 @@ public void onGoAway(GoAwayFrame frame)
goAwaySent = newGoAwayFrame(false);
GoAwayFrame goAwayFrame = goAwaySent;
zeroStreamsAction = () -> writeControlFrame(goAwayFrame, Callback.from(() ->
{
terminate();
outwardDisconnect(HTTP3ErrorCode.NO_ERROR.code(), "go_away");
}));
terminateAndDisconnect(HTTP3ErrorCode.NO_ERROR.code(), "go_away")
));
}
else
{
zeroStreamsAction = () ->
{
terminate();
outwardDisconnect(HTTP3ErrorCode.NO_ERROR.code(), "go_away");
};
zeroStreamsAction = () -> terminateAndDisconnect(HTTP3ErrorCode.NO_ERROR.code(), "go_away");
failStreams = true;
}
}
Expand Down Expand Up @@ -561,34 +554,24 @@ public void onGoAway(GoAwayFrame frame)
public boolean onIdleTimeout()
{
boolean notify = false;
boolean terminate = false;
try (AutoLock ignored = lock.lock())
{
switch (closeState)
{
case NOT_CLOSED:
{
notify = true;
break;
}
case LOCALLY_CLOSED:
case REMOTELY_CLOSED:
{
break;
}
case CLOSING:
case CLOSED:
{
if (LOG.isDebugEnabled())
LOG.debug("already closed, ignored idle timeout for {}", this);
return false;
}
default:
{
throw new IllegalStateException();
}
case NOT_CLOSED -> notify = true;
case CLOSING, CLOSED -> terminate = true;
}
}

if (terminate)
{
if (LOG.isDebugEnabled())
LOG.debug("already closed, ignored idle timeout for {}", this);
terminateAndDisconnect(HTTP3ErrorCode.NO_ERROR.code(), "idle_timeout");
return false;
}

boolean confirmed = true;
if (notify)
confirmed = notifyIdleTimeout();
Expand Down Expand Up @@ -646,19 +629,20 @@ public void inwardClose(long error, String reason)

if (goAwayFrame != null)
{
writeControlFrame(goAwayFrame, Callback.from(() ->
{
terminate();
outwardDisconnect(error, reason);
}));
writeControlFrame(goAwayFrame, Callback.from(() -> terminateAndDisconnect(error, reason)));
}
else
{
terminate();
outwardDisconnect(error, reason);
terminateAndDisconnect(error, reason);
}
}

private void terminateAndDisconnect(long error, String reason)
{
terminate();
outwardDisconnect(error, reason);
}

/**
* <p>Calls {@link #outwardClose(long, String)}, then notifies
* {@link Session.Listener#onDisconnect(Session, long, String)}.</p>
Expand Down
Loading

0 comments on commit fa14d99

Please sign in to comment.