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

Jetty 12 idletimeout #9905

Merged
merged 29 commits into from
Jun 14, 2023
Merged
Show file tree
Hide file tree
Changes from 27 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
95a1af1
IdleTimeout review
gregw Jun 6, 2023
fd113c8
Simpler idle test
gregw Jun 6, 2023
fe989b7
More ServerTests for idletimeout
gregw Jun 7, 2023
18cd25e
More ServerTests for idletimeout
gregw Jun 7, 2023
a792a76
Merge remote-tracking branch 'origin/jetty-12.0.x' into jetty-12-idle…
gregw Jun 7, 2023
f2a5845
Fixed FCGI
gregw Jun 7, 2023
d4f3271
Merge remote-tracking branch 'origin/jetty-12.0.x' into jetty-12-idle…
gregw Jun 9, 2023
eb63a1b
Recreated a ServerTimeoutsTest for multiple transports
gregw Jun 9, 2023
a8544bc
more robust tests
gregw Jun 9, 2023
185f3bd
WIP.
sbordet Jun 9, 2023
b75bbb5
merged work from @sbordet and @gregw
gregw Jun 9, 2023
bf498a0
Various improvements to CyclicTimeouts.
sbordet Jun 10, 2023
9733888
Merge branch 'fix/jetty-12-cyclictimeouts-improvements' into jetty-12…
gregw Jun 10, 2023
538bd5e
Fixed demand with idle timeout true
gregw Jun 10, 2023
6a36479
WIP idleTimeout
gregw Jun 12, 2023
28d63c2
WIP idleTimeout
gregw Jun 12, 2023
c748b54
WIP idleTimeout
gregw Jun 12, 2023
c232547
WIP idleTimeout
gregw Jun 12, 2023
450c03a
fix keystore to please BoringSSL + use correct temp path
lorban Jun 12, 2023
f745d0a
WIP idleTimeout
gregw Jun 12, 2023
1a0ee0b
WIP idleTimeout
gregw Jun 12, 2023
0dbbdf5
WIP idleTimeout
gregw Jun 12, 2023
5ac57c1
Fixed ErrorResponseAndCallback succeeded() and failed() to call super…
sbordet Jun 12, 2023
cf39788
Revert "Fixed ErrorResponseAndCallback succeeded() and failed() to ca…
gregw Jun 12, 2023
2ede51a
WIP idleTimeout
gregw Jun 12, 2023
d014214
WIP idleTimeout
gregw Jun 12, 2023
96a24c2
Merge branch 'jetty-12.0.x' into jetty-12-idletimeout
gregw Jun 13, 2023
d4257e9
Added context wrapper for idle timeout listener
gregw Jun 13, 2023
3aa70ba
updates from review
gregw Jun 13, 2023
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
Original file line number Diff line number Diff line change
Expand Up @@ -150,9 +150,9 @@ public void failed(Throwable x)
}

@Override
public boolean onIdleExpired()
public boolean onIdleExpired(TimeoutException timeout)
{
failed(new TimeoutException("Idle timeout expired"));
failed(timeout);
return false;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -185,9 +185,9 @@ private void fail(Throwable x)
}

@Override
public boolean onIdleExpired()
public boolean onIdleExpired(TimeoutException timeout)
{
fail(new TimeoutException("Idle timeout expired"));
fail(timeout);
return false;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -173,12 +173,12 @@ public Object getAttachment()
}

@Override
public boolean onIdleExpired()
public boolean onIdleExpired(TimeoutException timeout)
{
long idleTimeout = getEndPoint().getIdleTimeout();
boolean close = onIdleTimeout(idleTimeout);
if (close)
close(new TimeoutException("Idle timeout " + idleTimeout + " ms"));
close(timeout);
return false;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.instanceOf;
import static org.hamcrest.Matchers.sameInstance;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNull;
Expand Down Expand Up @@ -186,10 +187,12 @@ public void testReceiveResponseContentIdleTimeout(HttpCompliance compliance) thr
// ByteArrayEndPoint has an idle timeout of 0 by default,
// so to simulate an idle timeout is enough to wait a bit.
Thread.sleep(100);
connection.onIdleExpired();
TimeoutException timeoutException = new TimeoutException();
connection.onIdleExpired(timeoutException);

ExecutionException e = assertThrows(ExecutionException.class, () -> listener.get(5, TimeUnit.SECONDS));
assertThat(e.getCause(), instanceOf(TimeoutException.class));
assertThat(e.getCause(), sameInstance(timeoutException));
}

@ParameterizedTest
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ private void shutdown()
}

@Override
public boolean onIdleExpired()
public boolean onIdleExpired(TimeoutException timeoutException)
{
long idleTimeout = getEndPoint().getIdleTimeout();
TimeoutException failure = new TimeoutException("Idle timeout " + idleTimeout + " ms");
lachlan-roberts marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

import java.nio.ByteBuffer;
import java.util.Locale;
import java.util.concurrent.TimeoutException;

import org.eclipse.jetty.fcgi.FCGI;
import org.eclipse.jetty.fcgi.generator.Flusher;
Expand Down Expand Up @@ -301,6 +302,18 @@ private void generateResponseContent(ByteBufferPool.Accumulator accumulator, boo
_generator.generateResponseContent(accumulator, _id, buffer, last, _aborted);
}

@Override
public long getIdleTimeout()
{
return _connection.getEndPoint().getIdleTimeout();
}

@Override
public void setIdleTimeout(long idleTimeoutMs)
{
_connection.getEndPoint().setIdleTimeout(idleTimeoutMs);
}

@Override
public boolean isCommitted()
{
Expand Down Expand Up @@ -328,9 +341,9 @@ public void failed(Throwable x)
_connection.onCompleted(x);
}

public boolean onIdleTimeout(Throwable timeout)
public boolean onIdleTimeout(TimeoutException timeout)
{
Runnable task = _httpChannel.onFailure(timeout);
Runnable task = _httpChannel.onIdleTimeout(timeout);
if (task != null)
execute(task);
return false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import java.net.SocketAddress;
import java.nio.ByteBuffer;
import java.util.Set;
import java.util.concurrent.TimeoutException;

import org.eclipse.jetty.fcgi.FCGI;
import org.eclipse.jetty.fcgi.generator.Flusher;
Expand Down Expand Up @@ -291,7 +292,7 @@ private int fillInputBuffer()
}

@Override
protected boolean onReadTimeout(Throwable timeout)
protected boolean onReadTimeout(TimeoutException timeout)
{
if (stream != null)
return stream.onIdleTimeout(timeout);
Expand Down Expand Up @@ -323,6 +324,15 @@ void onCompleted(Throwable failure)
getFlusher().shutdown();
}

@Override
public boolean onIdleExpired(TimeoutException timeoutException)
{
Runnable task = stream.getHttpChannel().onIdleTimeout(timeoutException);
if (task != null)
getExecutor().execute(task);
return false;
}

private class ServerListener implements ServerParser.Listener
{
@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -522,6 +522,8 @@ public void testConnectionIdleTimeout() throws Exception
@Override
public boolean handle(org.eclipse.jetty.server.Request request, org.eclipse.jetty.server.Response response, Callback callback) throws Exception
{
// Handler says it will handle the idletimeout
request.addIdleTimeoutListener(t -> false);
TimeUnit.MILLISECONDS.sleep(2 * idleTimeout);
callback.succeeded();
return true;
Expand All @@ -530,7 +532,7 @@ public boolean handle(org.eclipse.jetty.server.Request request, org.eclipse.jett

connector.setIdleTimeout(idleTimeout);

// Request does not fail because idle timeouts while dispatched are ignored.
// Request does not fail because handler says it will handle it.
ContentResponse response1 = client.newRequest("localhost", connector.getLocalPort())
.scheme(scheme)
.idleTimeout(4 * idleTimeout, TimeUnit.MILLISECONDS)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@

package org.eclipse.jetty.http2.client.transport.internal;

import java.util.concurrent.TimeoutException;

import org.eclipse.jetty.http2.HTTP2Channel;
import org.eclipse.jetty.http2.HTTP2Stream;
import org.eclipse.jetty.http2.HTTP2StreamEndPoint;
Expand All @@ -38,13 +40,13 @@ public void onDataAvailable()
}

@Override
public void onTimeout(Throwable failure, Promise<Boolean> promise)
public void onTimeout(TimeoutException timeout, Promise<Boolean> promise)
{
if (LOG.isDebugEnabled())
LOG.debug("idle timeout on {}: {}", this, failure);
LOG.debug("idle timeout on {}", this, timeout);
Connection connection = getConnection();
if (connection != null)
promise.succeeded(connection.onIdleExpired());
promise.succeeded(connection.onIdleExpired(timeout));
else
promise.succeeded(true);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@

package org.eclipse.jetty.http2.client.transport.internal;

import java.util.concurrent.TimeoutException;

import org.eclipse.jetty.client.Result;
import org.eclipse.jetty.client.transport.HttpChannel;
import org.eclipse.jetty.client.transport.HttpExchange;
Expand Down Expand Up @@ -200,7 +202,7 @@ public void onReset(Stream stream, ResetFrame frame, Callback callback)
}

@Override
public void onIdleTimeout(Stream stream, Throwable x, Promise<Boolean> promise)
public void onIdleTimeout(Stream stream, TimeoutException x, Promise<Boolean> promise)
{
HTTP2Channel.Client channel = (HTTP2Channel.Client)((HTTP2Stream)stream).getAttachment();
channel.onTimeout(x, promise);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
package org.eclipse.jetty.http2.client.transport.internal;

import java.io.IOException;
import java.util.concurrent.TimeoutException;
import java.util.function.BiFunction;

import org.eclipse.jetty.client.HttpUpgrader;
Expand Down Expand Up @@ -220,7 +221,7 @@ void onReset(ResetFrame frame)
}

@Override
public void onTimeout(Throwable failure, Promise<Boolean> promise)
public void onTimeout(TimeoutException failure, Promise<Boolean> promise)
{
HttpExchange exchange = getHttpExchange();
if (exchange != null)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

package org.eclipse.jetty.http2;

import java.util.concurrent.TimeoutException;
import java.util.function.BiConsumer;

import org.eclipse.jetty.http2.frames.HeadersFrame;
Expand All @@ -34,7 +35,7 @@ public interface Client
{
public void onDataAvailable();

public void onTimeout(Throwable failure, Promise<Boolean> promise);
public void onTimeout(TimeoutException failure, Promise<Boolean> promise);

public void onFailure(Throwable failure, Callback callback);
}
Expand All @@ -54,7 +55,7 @@ public interface Server
// TODO: review the signature because the serialization done by HttpChannel.onError()
// is now failing the callback which fails the HttpStream, which should decide whether
// to reset the HTTP/2 stream, so we may not need the boolean return type.
public void onTimeout(Throwable failure, BiConsumer<Runnable, Boolean> consumer);
public void onTimeout(TimeoutException timeout, BiConsumer<Runnable, Boolean> consumer);

// TODO: can it be simplified? The callback seems to only be succeeded, which
// means it can be converted into a Runnable which may just be the return type
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import java.util.ArrayDeque;
import java.util.Queue;
import java.util.concurrent.Executor;
import java.util.concurrent.TimeoutException;
import java.util.concurrent.atomic.AtomicLong;

import org.eclipse.jetty.http2.api.Stream;
Expand Down Expand Up @@ -174,7 +175,7 @@ private int fill(EndPoint endPoint, ByteBuffer buffer)
}

@Override
public boolean onIdleExpired()
public boolean onIdleExpired(TimeoutException timeoutException)
{
boolean idle = isFillInterested();
if (idle)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -839,7 +839,7 @@ private void notifyReset(Stream stream, ResetFrame frame, Callback callback)
}
}

private void notifyIdleTimeout(Stream stream, Throwable failure, Promise<Boolean> promise)
private void notifyIdleTimeout(Stream stream, TimeoutException failure, Promise<Boolean> promise)
{
Listener listener = this.listener;
if (listener != null)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
package org.eclipse.jetty.http2.api;

import java.util.concurrent.CompletableFuture;
import java.util.concurrent.TimeoutException;

import org.eclipse.jetty.http2.frames.DataFrame;
import org.eclipse.jetty.http2.frames.HeadersFrame;
Expand Down Expand Up @@ -218,7 +219,7 @@ public default CompletableFuture<Void> reset(ResetFrame frame)
/**
* @param idleTimeout the stream idle timeout
* @see #getIdleTimeout()
* @see Stream.Listener#onIdleTimeout(Stream, Throwable, Promise)
* @see Stream.Listener#onIdleTimeout(Stream, TimeoutException, Promise)
*/
public void setIdleTimeout(long idleTimeout);

Expand Down Expand Up @@ -369,7 +370,7 @@ public default void onReset(Stream stream, ResetFrame frame, Callback callback)
* @param promise the promise to complete
* @see #getIdleTimeout()
*/
public default void onIdleTimeout(Stream stream, Throwable x, Promise<Boolean> promise)
public default void onIdleTimeout(Stream stream, TimeoutException x, Promise<Boolean> promise)
{
promise.succeeded(true);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ private void onFailure(Stream stream, Throwable failure, Callback callback)
}

@Override
public void onIdleTimeout(Stream stream, Throwable x, Promise<Boolean> promise)
public void onIdleTimeout(Stream stream, TimeoutException x, Promise<Boolean> promise)
{
getConnection().onStreamTimeout(stream, x, promise);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import java.util.List;
import java.util.Objects;
import java.util.Set;
import java.util.concurrent.TimeoutException;

import org.eclipse.jetty.http.BadMessageException;
import org.eclipse.jetty.http.HttpField;
Expand Down Expand Up @@ -155,14 +156,14 @@ public void onTrailers(Stream stream, HeadersFrame frame)
}
}

public void onStreamTimeout(Stream stream, Throwable failure, Promise<Boolean> promise)
public void onStreamTimeout(Stream stream, TimeoutException timeout, Promise<Boolean> promise)
{
if (LOG.isDebugEnabled())
LOG.debug("Idle timeout on {}", stream, failure);
LOG.debug("Idle timeout on {}", stream, timeout);
HTTP2Channel.Server channel = (HTTP2Channel.Server)((HTTP2Stream)stream).getAttachment();
if (channel != null)
{
channel.onTimeout(failure, (task, timedOut) ->
channel.onTimeout(timeout, (task, timedOut) ->
{
if (task != null)
offerTask(task, true);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
package org.eclipse.jetty.http2.server.internal;

import java.nio.ByteBuffer;
import java.util.concurrent.TimeoutException;
import java.util.function.BiConsumer;
import java.util.function.Supplier;

Expand Down Expand Up @@ -451,6 +452,18 @@ private boolean isTunnel(MetaData.Request request, MetaData.Response response)
return MetaData.isTunnel(request.getMethod(), response.getStatus());
}

@Override
public long getIdleTimeout()
{
return _stream.getIdleTimeout();
}

@Override
public void setIdleTimeout(long idleTimeoutMs)
{
_stream.setIdleTimeout(idleTimeoutMs);
}

@Override
public void push(MetaData.Request resource)
{
Expand Down Expand Up @@ -558,9 +571,9 @@ public Throwable consumeAvailable()
}

@Override
public void onTimeout(Throwable failure, BiConsumer<Runnable, Boolean> consumer)
public void onTimeout(TimeoutException timeout, BiConsumer<Runnable, Boolean> consumer)
{
Runnable task = _httpChannel.onFailure(failure);
Runnable task = _httpChannel.onIdleTimeout(timeout);
boolean idle = !_httpChannel.isRequestHandled();
consumer.accept(task, idle);
}
Expand Down
Loading