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. (#11267)

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 15, 2024
1 parent 090287d commit 0839a20
Show file tree
Hide file tree
Showing 9 changed files with 385 additions and 66 deletions.
31 changes: 20 additions & 11 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 @@ -1935,7 +1935,7 @@ private boolean onIdleTimeout()
goAwayFrame = goAwaySent;
closed = CloseState.CLOSING;
zeroStreamsAction = null;
failure = cause = new TimeoutException("Session idle timeout expired");
failure = cause = newTimeoutException();
}
case REMOTELY_CLOSED ->
{
Expand All @@ -1944,17 +1944,21 @@ private boolean onIdleTimeout()
goAwayFrame = goAwaySent;
closed = CloseState.CLOSING;
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;
failure = cause = newTimeoutException();
}
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(newTimeoutException());
return false;
}

if (notify)
{
boolean confirmed = notifyIdleTimeout(HTTP2Session.this);
Expand All @@ -1973,6 +1977,11 @@ private boolean onIdleTimeout()
return false;
}

private TimeoutException newTimeoutException()
{
return new TimeoutException("Session idle timeout expired");
}

private void onSessionFailure(int error, String reason, Callback callback)
{
GoAwayFrame goAwayFrame;
Expand Down Expand Up @@ -2036,7 +2045,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
66 changes: 23 additions & 43 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 @@ -645,18 +628,15 @@ public void inwardClose(long error, String reason)
failStreams(stream -> true, error, reason, true, new IOException(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);
}

/**
Expand Down
Loading

0 comments on commit 0839a20

Please sign in to comment.