Skip to content

Commit 2ea7c06

Browse files
slandelleslandelle
committed
Improve remote address handling in TimeoutHolder
Motivation: d86d481 reduced GC with only generating String from InetSocketAddress. There’s still room for improvement: * Less String concatenations when generating message * Stop reporting « not-connected » when timeout happens before channel gets connected Modifications:  * Use a pooled StringBuilder to concatenate timeout message part (still, IP String is not optimized as algorithm is very complex) * Set an unresolved InetSocketAddress when creating TimeoutHolder Result: More informative timeout message and less allocations
1 parent 1b35dbe commit 2ea7c06

File tree

7 files changed

+74
-49
lines changed

7 files changed

+74
-49
lines changed

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,7 @@ public void operationComplete(Future<? super Void> future) throws Exception {
114114
Request request = future.getTargetRequest();
115115
Uri uri = request.getUri();
116116

117-
timeoutsHolder.initRemoteAddress(remoteAddress);
117+
timeoutsHolder.setResolvedRemoteAddress(remoteAddress);
118118

119119
// in case of proxy tunneling, we'll add the SslHandler later, after the CONNECT request
120120
if (future.getProxyServer() == null && uri.isSecured()) {

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

Lines changed: 39 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
package org.asynchttpclient.netty.request;
1515

1616
import static io.netty.handler.codec.http.HttpHeaderNames.EXPECT;
17+
import static java.util.Collections.singletonList;
1718
import static org.asynchttpclient.handler.AsyncHandlerExtensionsUtils.toAsyncHandlerExtensions;
1819
import static org.asynchttpclient.util.Assertions.assertNotNull;
1920
import static org.asynchttpclient.util.AuthenticatorUtils.*;
@@ -31,6 +32,9 @@
3132
import io.netty.handler.codec.http.HttpMethod;
3233
import io.netty.handler.codec.http.HttpRequest;
3334
import io.netty.util.Timer;
35+
import io.netty.util.concurrent.Future;
36+
import io.netty.util.concurrent.ImmediateEventExecutor;
37+
import io.netty.util.concurrent.Promise;
3438

3539
import java.io.IOException;
3640
import java.net.InetSocketAddress;
@@ -230,8 +234,7 @@ private <T> ListenableFuture<T> sendRequestWithOpenChannel(Request request, Prox
230234
}
231235
}
232236

233-
TimeoutsHolder timeoutsHolder = scheduleRequestTimeout(future);
234-
timeoutsHolder.initRemoteAddress((InetSocketAddress) channel.remoteAddress());
237+
scheduleRequestTimeout(future, (InetSocketAddress) channel.remoteAddress());
235238
future.setChannelState(ChannelState.POOLED);
236239
future.attachChannel(channel, false);
237240

@@ -293,9 +296,7 @@ private <T> ListenableFuture<T> sendRequestWithNewChannel(//
293296
return future;
294297
}
295298

296-
scheduleRequestTimeout(future);
297-
298-
RequestHostnameResolver.INSTANCE.resolve(request, proxy, asyncHandler)//
299+
resolveAddresses(request, proxy, future, asyncHandler)//
299300
.addListener(new SimpleFutureListener<List<InetSocketAddress>>() {
300301

301302
@Override
@@ -315,6 +316,37 @@ protected void onFailure(Throwable cause) {
315316

316317
return future;
317318
}
319+
320+
private <T> Future<List<InetSocketAddress>> resolveAddresses(
321+
Request request,//
322+
ProxyServer proxy,//
323+
NettyResponseFuture<T> future,//
324+
AsyncHandler<T> asyncHandler) {
325+
326+
Uri uri = request.getUri();
327+
final Promise<List<InetSocketAddress>> promise = ImmediateEventExecutor.INSTANCE.newPromise();
328+
329+
if (proxy != null && !proxy.isIgnoredForHost(uri.getHost())) {
330+
int port = uri.isSecured() ? proxy.getSecuredPort() : proxy.getPort();
331+
InetSocketAddress unresolvedRemoteAddress = InetSocketAddress.createUnresolved(proxy.getHost(), port);
332+
scheduleRequestTimeout(future, unresolvedRemoteAddress);
333+
return RequestHostnameResolver.INSTANCE.resolve(request.getNameResolver(), unresolvedRemoteAddress, toAsyncHandlerExtensions(asyncHandler));
334+
335+
} else {
336+
int port = uri.getExplicitPort();
337+
338+
if (request.getAddress() != null) {
339+
// bypass resolution
340+
InetSocketAddress inetSocketAddress = new InetSocketAddress(request.getAddress(), port);
341+
return promise.setSuccess(singletonList(inetSocketAddress));
342+
343+
} else {
344+
InetSocketAddress unresolvedRemoteAddress = InetSocketAddress.createUnresolved(uri.getHost(), port);
345+
scheduleRequestTimeout(future, unresolvedRemoteAddress);
346+
return RequestHostnameResolver.INSTANCE.resolve(request.getNameResolver(), unresolvedRemoteAddress, toAsyncHandlerExtensions(asyncHandler));
347+
}
348+
}
349+
}
318350

319351
private <T> NettyResponseFuture<T> newNettyResponseFuture(Request request, AsyncHandler<T> asyncHandler, NettyRequest nettyRequest, ProxyServer proxyServer) {
320352

@@ -396,11 +428,10 @@ private void configureTransferAdapter(AsyncHandler<?> handler, HttpRequest httpR
396428
TransferCompletionHandler.class.cast(handler).headers(h);
397429
}
398430

399-
private TimeoutsHolder scheduleRequestTimeout(NettyResponseFuture<?> nettyResponseFuture) {
431+
private void scheduleRequestTimeout(NettyResponseFuture<?> nettyResponseFuture, InetSocketAddress originalRemoteAddress) {
400432
nettyResponseFuture.touch();
401-
TimeoutsHolder timeoutsHolder = new TimeoutsHolder(nettyTimer, nettyResponseFuture, this, config);
433+
TimeoutsHolder timeoutsHolder = new TimeoutsHolder(nettyTimer, nettyResponseFuture, this, config, originalRemoteAddress);
402434
nettyResponseFuture.setTimeoutsHolder(timeoutsHolder);
403-
return timeoutsHolder;
404435
}
405436

406437
private void scheduleReadTimeout(NettyResponseFuture<?> nettyResponseFuture) {

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

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818

1919
import org.asynchttpclient.netty.NettyResponseFuture;
2020
import org.asynchttpclient.netty.request.NettyRequestSender;
21+
import org.asynchttpclient.util.StringBuilderPool;
2122

2223
public class ReadTimeoutTimerTask extends TimeoutTimerTask {
2324

@@ -49,7 +50,9 @@ public void run(Timeout timeout) throws Exception {
4950

5051
if (durationBeforeCurrentReadTimeout <= 0L) {
5152
// idleConnectTimeout reached
52-
String message = "Read timeout to " + timeoutsHolder.remoteAddress() + " after " + readTimeout + " ms";
53+
StringBuilder sb = StringBuilderPool.DEFAULT.stringBuilder().append("Read timeout to ");
54+
appendRemoteAddress(sb);
55+
String message = sb.append(" after ").append(readTimeout).append(" ms").toString();
5356
long durationSinceLastTouch = now - nettyResponseFuture.getLastTouch();
5457
expire(message, durationSinceLastTouch);
5558
// cancel request timeout sibling

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

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818

1919
import org.asynchttpclient.netty.NettyResponseFuture;
2020
import org.asynchttpclient.netty.request.NettyRequestSender;
21+
import org.asynchttpclient.util.StringBuilderPool;
2122

2223
public class RequestTimeoutTimerTask extends TimeoutTimerTask {
2324

@@ -43,7 +44,9 @@ public void run(Timeout timeout) throws Exception {
4344
if (nettyResponseFuture.isDone())
4445
return;
4546

46-
String message = "Request timeout to " + timeoutsHolder.remoteAddress() + " after " + requestTimeout + " ms";
47+
StringBuilder sb = StringBuilderPool.DEFAULT.stringBuilder().append("Request timeout to ");
48+
appendRemoteAddress(sb);
49+
String message = sb.append(" after ").append(requestTimeout).append(" ms").toString();
4750
long age = unpreciseMillisTime() - nettyResponseFuture.getStart();
4851
expire(message, age);
4952
}

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

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515

1616
import io.netty.util.TimerTask;
1717

18+
import java.net.InetSocketAddress;
1819
import java.util.concurrent.TimeoutException;
1920
import java.util.concurrent.atomic.AtomicBoolean;
2021

@@ -44,12 +45,21 @@ protected void expire(String message, long time) {
4445
}
4546

4647
/**
47-
* When the timeout is cancelled, it could still be referenced for quite some time in the Timer.
48-
* Holding a reference to the future might mean holding a reference to the channel, and heavy objects such as SslEngines
48+
* When the timeout is cancelled, it could still be referenced for quite some time in the Timer. Holding a reference to the future might mean holding a reference to the
49+
* channel, and heavy objects such as SslEngines
4950
*/
5051
public void clean() {
5152
if (done.compareAndSet(false, true)) {
5253
nettyResponseFuture = null;
5354
}
5455
}
56+
57+
protected void appendRemoteAddress(StringBuilder sb) {
58+
InetSocketAddress remoteAddress = timeoutsHolder.remoteAddress();
59+
sb.append(remoteAddress.getHostName());
60+
if (!remoteAddress.isUnresolved()) {
61+
sb.append('/').append(remoteAddress.getAddress().getHostAddress());
62+
}
63+
sb.append(':').append(remoteAddress.getPort());
64+
}
5565
}

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

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ public class TimeoutsHolder {
4141
public volatile Timeout readTimeout;
4242
private volatile InetSocketAddress remoteAddress;
4343

44-
public TimeoutsHolder(Timer nettyTimer, NettyResponseFuture<?> nettyResponseFuture, NettyRequestSender requestSender, AsyncHttpClientConfig config) {
44+
public TimeoutsHolder(Timer nettyTimer, NettyResponseFuture<?> nettyResponseFuture, NettyRequestSender requestSender, AsyncHttpClientConfig config, InetSocketAddress originalRemoteAddress) {
4545
this.nettyTimer = nettyTimer;
4646
this.nettyResponseFuture = nettyResponseFuture;
4747
this.requestSender = requestSender;
@@ -65,10 +65,14 @@ public TimeoutsHolder(Timer nettyTimer, NettyResponseFuture<?> nettyResponseFutu
6565
}
6666
}
6767

68-
public void initRemoteAddress(InetSocketAddress address) {
68+
public void setResolvedRemoteAddress(InetSocketAddress address) {
6969
remoteAddress = address;
7070
}
7171

72+
InetSocketAddress remoteAddress() {
73+
return remoteAddress;
74+
}
75+
7276
public void startReadTimeout() {
7377
if (readTimeoutValue != -1) {
7478
startReadTimeout(null);
@@ -106,8 +110,4 @@ public void cancel() {
106110
private Timeout newTimeout(TimerTask task, long delay) {
107111
return requestSender.isClosed() ? null : nettyTimer.newTimeout(task, delay, TimeUnit.MILLISECONDS);
108112
}
109-
110-
String remoteAddress() {
111-
return remoteAddress == null ? "not-connected" : remoteAddress.toString();
112-
}
113113
}

‎client/src/main/java/org/asynchttpclient/resolver/RequestHostnameResolver.java

Lines changed: 8 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -13,64 +13,42 @@
1313
*/
1414
package org.asynchttpclient.resolver;
1515

16-
import static org.asynchttpclient.handler.AsyncHandlerExtensionsUtils.toAsyncHandlerExtensions;
16+
import io.netty.resolver.NameResolver;
1717
import io.netty.util.concurrent.Future;
1818
import io.netty.util.concurrent.ImmediateEventExecutor;
1919
import io.netty.util.concurrent.Promise;
2020

2121
import java.net.InetAddress;
2222
import java.net.InetSocketAddress;
2323
import java.util.ArrayList;
24-
import java.util.Collections;
2524
import java.util.List;
2625

27-
import org.asynchttpclient.AsyncHandler;
28-
import org.asynchttpclient.Request;
2926
import org.asynchttpclient.handler.AsyncHandlerExtensions;
3027
import org.asynchttpclient.netty.SimpleFutureListener;
31-
import org.asynchttpclient.proxy.ProxyServer;
32-
import org.asynchttpclient.uri.Uri;
3328
import org.slf4j.Logger;
3429
import org.slf4j.LoggerFactory;
3530

3631
public enum RequestHostnameResolver {
3732

3833
INSTANCE;
3934

40-
public Future<List<InetSocketAddress>> resolve(Request request, ProxyServer proxy, AsyncHandler<?> asyncHandler) {
35+
public Future<List<InetSocketAddress>> resolve(NameResolver<InetAddress> nameResolver, InetSocketAddress unresolvedAddress, AsyncHandlerExtensions asyncHandlerExtensions) {
4136

42-
Uri uri = request.getUri();
37+
final String hostname = unresolvedAddress.getHostName();
38+
final int port = unresolvedAddress.getPort();
4339
final Promise<List<InetSocketAddress>> promise = ImmediateEventExecutor.INSTANCE.newPromise();
4440

45-
if (request.getAddress() != null) {
46-
List<InetSocketAddress> resolved = Collections.singletonList(new InetSocketAddress(request.getAddress(), uri.getExplicitPort()));
47-
return promise.setSuccess(resolved);
48-
}
49-
50-
// don't notify on explicit address
51-
final AsyncHandlerExtensions asyncHandlerExtensions = request.getAddress() == null ? toAsyncHandlerExtensions(asyncHandler) : null;
52-
final String name;
53-
final int port;
54-
55-
if (proxy != null && !proxy.isIgnoredForHost(uri.getHost())) {
56-
name = proxy.getHost();
57-
port = uri.isSecured() ? proxy.getSecuredPort() : proxy.getPort();
58-
} else {
59-
name = uri.getHost();
60-
port = uri.getExplicitPort();
61-
}
62-
6341
if (asyncHandlerExtensions != null) {
6442
try {
65-
asyncHandlerExtensions.onHostnameResolutionAttempt(name);
43+
asyncHandlerExtensions.onHostnameResolutionAttempt(hostname);
6644
} catch (Exception e) {
6745
LOGGER.error("onHostnameResolutionAttempt crashed", e);
6846
promise.tryFailure(e);
6947
return promise;
7048
}
7149
}
7250

73-
final Future<List<InetAddress>> whenResolved = request.getNameResolver().resolveAll(name);
51+
final Future<List<InetAddress>> whenResolved = nameResolver.resolveAll(hostname);
7452

7553
whenResolved.addListener(new SimpleFutureListener<List<InetAddress>>() {
7654

@@ -82,7 +60,7 @@ protected void onSuccess(List<InetAddress> value) throws Exception {
8260
}
8361
if (asyncHandlerExtensions != null) {
8462
try {
85-
asyncHandlerExtensions.onHostnameResolutionSuccess(name, socketAddresses);
63+
asyncHandlerExtensions.onHostnameResolutionSuccess(hostname, socketAddresses);
8664
} catch (Exception e) {
8765
LOGGER.error("onHostnameResolutionSuccess crashed", e);
8866
promise.tryFailure(e);
@@ -96,7 +74,7 @@ protected void onSuccess(List<InetAddress> value) throws Exception {
9674
protected void onFailure(Throwable t) throws Exception {
9775
if (asyncHandlerExtensions != null) {
9876
try {
99-
asyncHandlerExtensions.onHostnameResolutionFailure(name, t);
77+
asyncHandlerExtensions.onHostnameResolutionFailure(hostname, t);
10078
} catch (Exception e) {
10179
LOGGER.error("onHostnameResolutionFailure crashed", e);
10280
promise.tryFailure(e);

0 commit comments

Comments
 (0)