Skip to content

Commit 8eda565

Browse files
Merge remote-tracking branch 'upstream/main' into feature/absolute-request-deadline-standalone
# Conflicts: # client/src/main/java/org/asynchttpclient/AsyncHttpClientConfig.java # client/src/main/java/org/asynchttpclient/netty/timeout/TimeoutsHolder.java
2 parents fc910b7 + c1af3a4 commit 8eda565

12 files changed

Lines changed: 557 additions & 34 deletions

File tree

‎client/src/main/java/org/asynchttpclient/AsyncHttpClientConfig.java‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,34 @@ public interface AsyncHttpClientConfig {
118118
*/
119119
Duration getRequestTimeout();
120120

121+
/**
122+
* Whether request and read timeouts are armed on an event loop rather than on {@link #getNettyTimer()}.
123+
* <p>
124+
* The timer is a hashed wheel: it fires on the first tick at or after the deadline, so a deadline near or
125+
* below {@link #getHashedWheelTimerTickDuration()} is rounded up to it, and one thread carries every expiry
126+
* for the whole client. An event loop instead schedules by deadline and derives its own select timeout from
127+
* the nearest one, so nothing is rounded up, and the loops share the load rather than funnelling it through
128+
* a single thread. Both effects matter most to short deadlines, where a tick is a large fraction of the
129+
* budget and a burst of expiries has no headroom to absorb.
130+
* <p>
131+
* The cost is where the expiry runs. On the timer it runs on the timer thread; on an event loop it runs on
132+
* an I/O thread, and so does whatever the caller chained onto the response future, because that future is
133+
* completed from there. Blocking an I/O thread stalls every connection it serves, so a caller enabling this
134+
* should hand its own work off with {@code handleAsync} or an {@code AsyncHandler} that does the same.
135+
* That is why this is opt-in rather than the default.
136+
* <p>
137+
* The loop is always the one that owns the exchange's channel. Until there is a channel -- while an address
138+
* is being resolved and a connection made -- the timer carries the timeout, and the exchange moves it onto
139+
* the loop once the connection succeeds.
140+
* <p>
141+
* The connection-pool cleaner stays on the timer either way.
142+
*
143+
* @return {@code true} to arm request and read timeouts on an event loop
144+
*/
145+
default boolean isUseEventLoopTimeouts() {
146+
return false;
147+
}
148+
121149
/**
122150
* Whether {@link #getRequestTimeout()} is a deadline for the whole exchange rather than for each attempt
123151
* within it.

‎client/src/main/java/org/asynchttpclient/DefaultAsyncHttpClientConfig.java‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,7 @@
9898
import static org.asynchttpclient.config.AsyncHttpClientConfigDefaults.defaultStrict302Handling;
9999
import static org.asynchttpclient.config.AsyncHttpClientConfigDefaults.defaultTcpNoDelay;
100100
import static org.asynchttpclient.config.AsyncHttpClientConfigDefaults.defaultThreadPoolName;
101+
import static org.asynchttpclient.config.AsyncHttpClientConfigDefaults.defaultUseEventLoopTimeouts;
101102
import static org.asynchttpclient.config.AsyncHttpClientConfigDefaults.defaultUseInsecureTrustManager;
102103
import static org.asynchttpclient.config.AsyncHttpClientConfigDefaults.defaultUseLaxCookieEncoder;
103104
import static org.asynchttpclient.config.AsyncHttpClientConfigDefaults.defaultUseNativeTransport;
@@ -159,6 +160,7 @@ public class DefaultAsyncHttpClientConfig implements AsyncHttpClientConfig {
159160
private final Duration connectTimeout;
160161
private final Duration requestTimeout;
161162
private final Duration readTimeout;
163+
private final boolean useEventLoopTimeouts;
162164
private final Duration shutdownQuietPeriod;
163165
private final Duration shutdownTimeout;
164166

@@ -261,6 +263,7 @@ private DefaultAsyncHttpClientConfig(// http
261263
Duration connectTimeout,
262264
Duration requestTimeout,
263265
Duration readTimeout,
266+
boolean useEventLoopTimeouts,
264267
Duration shutdownQuietPeriod,
265268
Duration shutdownTimeout,
266269

@@ -371,6 +374,7 @@ private DefaultAsyncHttpClientConfig(// http
371374
this.connectTimeout = connectTimeout;
372375
this.requestTimeout = requestTimeout;
373376
this.readTimeout = readTimeout;
377+
this.useEventLoopTimeouts = useEventLoopTimeouts;
374378
this.shutdownQuietPeriod = shutdownQuietPeriod;
375379
this.shutdownTimeout = shutdownTimeout;
376380

@@ -594,6 +598,11 @@ public Duration getReadTimeout() {
594598
return readTimeout;
595599
}
596600

601+
@Override
602+
public boolean isUseEventLoopTimeouts() {
603+
return useEventLoopTimeouts;
604+
}
605+
597606
@Override
598607
public Duration getShutdownQuietPeriod() {
599608
return shutdownQuietPeriod;
@@ -968,6 +977,7 @@ public static class Builder {
968977
private Duration connectTimeout = defaultConnectTimeout();
969978
private Duration requestTimeout = defaultRequestTimeout();
970979
private Duration readTimeout = defaultReadTimeout();
980+
private boolean useEventLoopTimeouts = defaultUseEventLoopTimeouts();
971981
private Duration shutdownQuietPeriod = defaultShutdownQuietPeriod();
972982
private Duration shutdownTimeout = defaultShutdownTimeout();
973983

@@ -1075,6 +1085,7 @@ public Builder(AsyncHttpClientConfig config) {
10751085
connectTimeout = config.getConnectTimeout();
10761086
requestTimeout = config.getRequestTimeout();
10771087
readTimeout = config.getReadTimeout();
1088+
useEventLoopTimeouts = config.isUseEventLoopTimeouts();
10781089
shutdownQuietPeriod = config.getShutdownQuietPeriod();
10791090
shutdownTimeout = config.getShutdownTimeout();
10801091

@@ -1377,6 +1388,17 @@ public Builder setReadTimeout(Duration readTimeout) {
13771388
return this;
13781389
}
13791390

1391+
/**
1392+
* @param useEventLoopTimeouts whether to arm request and read timeouts on an event loop instead of on
1393+
* the client's timer; see {@link AsyncHttpClientConfig#isUseEventLoopTimeouts()}
1394+
* for the trade-off this makes
1395+
* @return this
1396+
*/
1397+
public Builder setUseEventLoopTimeouts(boolean useEventLoopTimeouts) {
1398+
this.useEventLoopTimeouts = useEventLoopTimeouts;
1399+
return this;
1400+
}
1401+
13801402
public Builder setShutdownQuietPeriod(Duration shutdownQuietPeriod) {
13811403
this.shutdownQuietPeriod = shutdownQuietPeriod;
13821404
return this;
@@ -1787,6 +1809,7 @@ public DefaultAsyncHttpClientConfig build() {
17871809
connectTimeout,
17881810
requestTimeout,
17891811
readTimeout,
1812+
useEventLoopTimeouts,
17901813
shutdownQuietPeriod,
17911814
shutdownTimeout,
17921815
keepAlive,

‎client/src/main/java/org/asynchttpclient/config/AsyncHttpClientConfigDefaults.java‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ public final class AsyncHttpClientConfigDefaults {
4141
public static final String CONNECTION_POOL_CLEANER_PERIOD_CONFIG = "connectionPoolCleanerPeriod";
4242
public static final String READ_TIMEOUT_CONFIG = "readTimeout";
4343
public static final String REQUEST_TIMEOUT_CONFIG = "requestTimeout";
44+
public static final String USE_EVENT_LOOP_TIMEOUTS_CONFIG = "useEventLoopTimeouts";
4445
public static final String CONNECTION_TTL_CONFIG = "connectionTtl";
4546
public static final String FOLLOW_REDIRECT_CONFIG = "followRedirect";
4647
public static final String MAX_REDIRECTS_CONFIG = "maxRedirects";
@@ -155,6 +156,10 @@ public static Duration defaultRequestTimeout() {
155156
return AsyncHttpClientConfigHelper.getAsyncHttpClientConfig().getDuration(ASYNC_CLIENT_CONFIG_ROOT + REQUEST_TIMEOUT_CONFIG);
156157
}
157158

159+
public static boolean defaultUseEventLoopTimeouts() {
160+
return AsyncHttpClientConfigHelper.getAsyncHttpClientConfig().getBoolean(ASYNC_CLIENT_CONFIG_ROOT + USE_EVENT_LOOP_TIMEOUTS_CONFIG);
161+
}
162+
158163
public static Duration defaultConnectionTtl() {
159164
return AsyncHttpClientConfigHelper.getAsyncHttpClientConfig().getDuration(ASYNC_CLIENT_CONFIG_ROOT + CONNECTION_TTL_CONFIG);
160165
}

‎client/src/main/java/org/asynchttpclient/netty/NettyResponseFuture.java‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -469,6 +469,11 @@ public void setTimeoutsHolder(TimeoutsHolder timeoutsHolder) {
469469
if (ref != null) {
470470
ref.cancel();
471471
}
472+
if (timeoutsHolder != null) {
473+
// Armed here rather than by the caller: a holder can run its timeout the moment it is armed, so it
474+
// has to be reachable from this future first, and no caller can then install one that never arms.
475+
timeoutsHolder.start();
476+
}
472477
}
473478

474479
public boolean isInAuth() {

‎client/src/main/java/org/asynchttpclient/netty/channel/NettyConnectListener.java‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,11 @@ public void onSuccess(Channel channel, InetSocketAddress remoteAddress) {
124124
// mid-handshake could not close the socket, stranding it until handshakeTimeout (issue #2189).
125125
future.attachChannel(channel, false);
126126

127+
// The timeouts were armed before there was a channel to arm them on; hand them the one the exchange
128+
// ended up with. This listener runs on that channel's own loop, so the move needs no wakeup, and from
129+
// here on an expiry runs on the thread that would have to close the socket.
130+
timeoutsHolder.rehomeOn(channel.eventLoop());
131+
127132
Request request = future.getTargetRequest();
128133
Uri uri = request.getUri();
129134
// don't set a null resolved address - if the remoteAddress is null we keep

‎client/src/main/java/org/asynchttpclient/netty/request/NettyRequestSender.java‎

Lines changed: 33 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,7 @@
8282
import org.asynchttpclient.resolver.RequestHostnameResolver;
8383
import org.asynchttpclient.uri.Uri;
8484
import org.asynchttpclient.ws.WebSocketUpgradeHandler;
85+
import org.jetbrains.annotations.Nullable;
8586

8687
import org.slf4j.Logger;
8788
import org.slf4j.LoggerFactory;
@@ -402,15 +403,17 @@ private <T> ListenableFuture<T> sendRequestWithOpenChannel(NettyResponseFuture<T
402403
return future;
403404
}
404405

406+
future.setChannelState(ChannelState.POOLED);
407+
// Before the timeout is armed, not after: an expiry reaches the channel only through the future, and on
408+
// an event loop a short enough deadline can be delivered before the next statement would have run.
409+
future.attachChannel(channel, false);
410+
405411
SocketAddress channelRemoteAddress = channel.remoteAddress();
406412
if (channelRemoteAddress != null) {
407413
// otherwise, bad luck, the channel was closed, see bellow
408-
scheduleRequestTimeout(future, (InetSocketAddress) channelRemoteAddress);
414+
scheduleRequestTimeout(future, (InetSocketAddress) channelRemoteAddress, channel);
409415
}
410416

411-
future.setChannelState(ChannelState.POOLED);
412-
future.attachChannel(channel, false);
413-
414417
if (LOGGER.isDebugEnabled()) {
415418
HttpRequest httpRequest = future.getNettyRequest().getHttpRequest();
416419
LOGGER.debug("Using open Channel {} for {} '{}'", channel, httpRequest.method(), httpRequest.uri());
@@ -1083,12 +1086,36 @@ private static void configureTransferAdapter(AsyncHandler<?> handler, HttpReques
10831086

10841087
private void scheduleRequestTimeout(NettyResponseFuture<?> nettyResponseFuture,
10851088
InetSocketAddress originalRemoteAddress) {
1089+
scheduleRequestTimeout(nettyResponseFuture, originalRemoteAddress, null);
1090+
}
1091+
1092+
/**
1093+
* @param channel the channel the exchange will run on when it is already known, so the timeout can be armed
1094+
* on the loop that owns it. Null on the connect path: the timeout is armed before the channel
1095+
* exists, deliberately, so that it also bounds address resolution and the connect itself, and
1096+
* {@code TimeoutsHolder#rehomeOn} moves it onto the loop once there is one.
1097+
*/
1098+
private void scheduleRequestTimeout(NettyResponseFuture<?> nettyResponseFuture,
1099+
InetSocketAddress originalRemoteAddress,
1100+
@Nullable Channel channel) {
10861101
nettyResponseFuture.touch();
1087-
TimeoutsHolder timeoutsHolder = new TimeoutsHolder(nettyTimer, nettyResponseFuture, this, config,
1088-
originalRemoteAddress);
1102+
TimeoutsHolder timeoutsHolder = new TimeoutsHolder(nettyTimer, timeoutExecutor(channel), nettyResponseFuture,
1103+
this, config, originalRemoteAddress);
1104+
// Arms the timeout as a part of installing the holder, which is why the pooled path attaches the
1105+
// channel first: an expiry that lands immediately reaches the channel only through the future.
10891106
nettyResponseFuture.setTimeoutsHolder(timeoutsHolder);
10901107
}
10911108

1109+
/**
1110+
* The loop to arm an exchange's timeouts on, or null to leave them on the client's timer. Only ever the
1111+
* exchange's own channel's loop: any other loop would be woken by an entry it has no interest in, and the
1112+
* group's chooser hands out channels from the same counter, so drawing from it here would shift which loops
1113+
* connections land on.
1114+
*/
1115+
private @Nullable EventExecutor timeoutExecutor(@Nullable Channel channel) {
1116+
return config.isUseEventLoopTimeouts() && channel != null ? channel.eventLoop() : null;
1117+
}
1118+
10921119
private static void scheduleReadTimeout(NettyResponseFuture<?> nettyResponseFuture) {
10931120
TimeoutsHolder timeoutsHolder = nettyResponseFuture.getTimeoutsHolder();
10941121
if (timeoutsHolder != null) {

‎client/src/main/java/org/asynchttpclient/netty/timeout/ReadTimeoutTimerTask.java‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@
2222

2323
import static org.asynchttpclient.util.DateUtils.unpreciseMillisTime;
2424

25-
public class ReadTimeoutTimerTask extends TimeoutTimerTask {
25+
public class ReadTimeoutTimerTask extends TimeoutTimerTask implements Runnable {
2626

2727
private final long readTimeout;
2828

@@ -31,6 +31,15 @@ public class ReadTimeoutTimerTask extends TimeoutTimerTask {
3131
this.readTimeout = readTimeout;
3232
}
3333

34+
/**
35+
* The event-loop entry point. Nothing below reads the {@link Timeout}, which is the timer's own handle on
36+
* this task and something an event loop has no equivalent of, so both entry points share one body.
37+
*/
38+
@Override
39+
public void run() {
40+
run(null);
41+
}
42+
3443
@Override
3544
public void run(Timeout timeout) {
3645
if (done.getAndSet(true) || requestSender.isClosed()) {

‎client/src/main/java/org/asynchttpclient/netty/timeout/RequestTimeoutTimerTask.java‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@
2222

2323
import static org.asynchttpclient.util.DateUtils.unpreciseMillisTime;
2424

25-
public class RequestTimeoutTimerTask extends TimeoutTimerTask {
25+
public class RequestTimeoutTimerTask extends TimeoutTimerTask implements Runnable {
2626

2727
private final long requestTimeout;
2828

@@ -34,6 +34,15 @@ public class RequestTimeoutTimerTask extends TimeoutTimerTask {
3434
this.requestTimeout = requestTimeout;
3535
}
3636

37+
/**
38+
* The event-loop entry point. Nothing below reads the {@link Timeout}, which is the timer's own handle on
39+
* this task and something an event loop has no equivalent of, so both entry points share one body.
40+
*/
41+
@Override
42+
public void run() {
43+
run(null);
44+
}
45+
3746
@Override
3847
public void run(Timeout timeout) {
3948
if (done.getAndSet(true) || requestSender.isClosed()) {

‎client/src/main/java/org/asynchttpclient/netty/timeout/TimeoutTimerTask.java‎

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,16 +15,27 @@
1515
*/
1616
package org.asynchttpclient.netty.timeout;
1717

18+
import io.netty.util.Timeout;
1819
import io.netty.util.TimerTask;
20+
import io.netty.util.concurrent.ScheduledFuture;
1921
import org.asynchttpclient.netty.NettyResponseFuture;
2022
import org.asynchttpclient.netty.request.NettyRequestSender;
23+
import org.jetbrains.annotations.Nullable;
2124
import org.slf4j.Logger;
2225
import org.slf4j.LoggerFactory;
2326

2427
import java.net.InetSocketAddress;
28+
import java.util.concurrent.RejectedExecutionException;
2529
import java.util.concurrent.TimeoutException;
2630
import java.util.concurrent.atomic.AtomicBoolean;
2731

32+
/**
33+
* A timeout that can be armed either on a {@link io.netty.util.Timer} or on an event loop; which one an
34+
* exchange uses is {@link org.asynchttpclient.AsyncHttpClientConfig#isUseEventLoopTimeouts()}. An event loop
35+
* schedules {@link Runnable}s, so each subclass implements that as a second entry point into the same body.
36+
* This class stays a {@link TimerTask} alone: {@code Runnable#run} declares no checked exception, so a
37+
* {@code run()} here would have to catch what {@link TimerTask#run(Timeout)} declares and no subclass throws.
38+
*/
2839
public abstract class TimeoutTimerTask implements TimerTask {
2940

3041
private static final Logger LOGGER = LoggerFactory.getLogger(TimeoutTimerTask.class);
@@ -33,13 +44,67 @@ public abstract class TimeoutTimerTask implements TimerTask {
3344
protected final NettyRequestSender requestSender;
3445
final TimeoutsHolder timeoutsHolder;
3546
volatile NettyResponseFuture<?> nettyResponseFuture;
47+
// The scheduled entry this task is armed on, one field per scheduler so that a scheduler changing its
48+
// return type is a compile error rather than a cancellation that silently stops working. At most one is
49+
// ever set. Held here rather than in a wrapper so arming allocates nothing beyond what the scheduler needs.
50+
private volatile @Nullable Timeout timerHandle;
51+
private volatile @Nullable ScheduledFuture<?> loopHandle;
3652

3753
TimeoutTimerTask(NettyResponseFuture<?> nettyResponseFuture, NettyRequestSender requestSender, TimeoutsHolder timeoutsHolder) {
3854
this.nettyResponseFuture = nettyResponseFuture;
3955
this.requestSender = requestSender;
4056
this.timeoutsHolder = timeoutsHolder;
4157
}
4258

59+
void armedOn(Timeout handle) {
60+
// Each clears the other, so a handle left over from a previous arming cannot mask the live one and
61+
// leave its entry sitting in a scheduler, holding the future until a deadline nobody is waiting for.
62+
loopHandle = null;
63+
timerHandle = handle;
64+
}
65+
66+
void armedOn(ScheduledFuture<?> handle) {
67+
timerHandle = null;
68+
loopHandle = handle;
69+
}
70+
71+
/**
72+
* Cancels the scheduled entry this task was armed on, if any. Never interrupts: on the event-loop path the
73+
* task may be running on the very thread this is called from, and nothing in it answers interruption.
74+
*
75+
* @return whether an entry was taken back out of its scheduler before it could run
76+
*/
77+
boolean cancelArmed() {
78+
Timeout timer = timerHandle;
79+
if (timer != null) {
80+
timerHandle = null;
81+
return timer.cancel();
82+
}
83+
ScheduledFuture<?> scheduled = loopHandle;
84+
if (scheduled != null) {
85+
loopHandle = null;
86+
try {
87+
return scheduled.cancel(false);
88+
} catch (RejectedExecutionException e) {
89+
// Cancelling from off the loop enqueues the removal, which a loop that is already shutting down
90+
// rejects. The entry dies with the loop either way, and this runs under
91+
// ListenableFuture#cancel, which has never thrown for a client that is closing.
92+
LOGGER.debug("Event loop rejected a timeout cancellation", e);
93+
return false;
94+
}
95+
}
96+
return false;
97+
}
98+
99+
/**
100+
* Whether this task has been claimed, either by firing or by {@link #clean()}. Stands in for the
101+
* scheduler's own already-expired flag, which the two schedulers spell differently, and is if anything the
102+
* more precise of the two: it flips when {@code run} is entered rather than when the entry is marked.
103+
*/
104+
boolean isClaimed() {
105+
return done.get();
106+
}
107+
43108
void expire(String message, long time) {
44109
LOGGER.debug("{} for {} after {} ms", message, nettyResponseFuture, time);
45110
requestSender.abort(nettyResponseFuture.channel(), nettyResponseFuture, new TimeoutException(message));

0 commit comments

Comments
 (0)