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.

Sign up for GitHub

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 #11259 - HTTP/2 connection not closed after idle timeout when TCP congested. #11267

Merged
merged 3 commits into from
Jan 15, 2024
Merged

Fixes #11259 - HTTP/2 connection not closed after idle timeout when TCP congested. #11267

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fa14d99
Fixes #11259 - HTTP/2 connection not closed after idle timeout when T…
sbordet Jan 12, 2024
b19dc8a
Cosmetics.
sbordet Jan 12, 2024
d724208
Updates after review.
sbordet Jan 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 13 additions & 9 deletions ...re/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Session.java
View file
Open in desktop
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());
Copy link
Contributor

Choose a reason for hiding this comment

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

All branches of the switch above result in a new TimeoutException, so just do it once before the switch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uhm, no. The TimeoutException is only allocated if terminate=true which only happens if the session is already in CLOSED state.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is also allocated in locally and remotely closed cases. At least give it the same message as the others.

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
Open in desktop
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
Open in desktop
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
Loading