-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Reuse the per-request partition key instead of recomputing it #2231
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -136,6 +136,9 @@ public final class NettyResponseFuture<V> implements ListenableFuture<V> { | |
| private volatile List<InetSocketAddress> roundRobinAddresses; | ||
| private volatile Uri roundRobinBaseUri; | ||
| private volatile ScramContext scramContext; | ||
| // Memoized base (host/scheme/port) partition key; see basePartitionKey(). proxyServer is final and | ||
| // targetRequest is its only other input, so this is invalidated only by setTargetRequest. | ||
| private volatile Object basePartitionKeyCache; | ||
|
|
||
| public NettyResponseFuture(Request originalRequest, | ||
| AsyncHandler<V> asyncHandler, | ||
|
|
@@ -370,6 +373,9 @@ public Request getTargetRequest() { | |
|
|
||
| public void setTargetRequest(Request targetRequest) { | ||
| this.targetRequest = targetRequest; | ||
| // Invalidate the memoized base partition key: a redirect/retry may target a different | ||
| // host/scheme/port, which changes the key. | ||
| basePartitionKeyCache = null; | ||
| } | ||
|
|
||
| public Request getCurrentRequest() { | ||
|
|
@@ -552,8 +558,16 @@ public Object getPartitionKey() { | |
| * initially selected. | ||
| */ | ||
| public Object basePartitionKey() { | ||
| return connectionPoolPartitioning.getPartitionKey(targetRequest.getUri(), targetRequest.getVirtualHost(), | ||
| proxyServer); | ||
| // Memoized: the same key is needed at several sites per request attempt (semaphore acquire, pool | ||
| // poll/offer, HTTP/2 registration). It depends only on targetRequest (host/scheme/port + virtualHost) | ||
| // and the final proxyServer, so it is recomputed only when setTargetRequest changes the target. | ||
| Object key = basePartitionKeyCache; | ||
| if (key == null) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not a bug today, since all reads and writes to this cache are serialized by the one-at-a-time request lifecycle. However, the lazy check-then-store pattern only remains safe as long as that invariant holds. Computing the key eagerly in the constructor and in |
||
| key = connectionPoolPartitioning.getPartitionKey(targetRequest.getUri(), targetRequest.getVirtualHost(), | ||
| proxyServer); | ||
| basePartitionKeyCache = key; | ||
| } | ||
| return key; | ||
| } | ||
|
|
||
| /** | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment lists "pool poll/offer" as sites that use this memoized key, but
pollPooledChannelnever callsbasePartitionKey(). It computes its own key from the request parameter, and that is intentional. On the filter replay path (replayRequest→sendNextRequest), a filter can replace the request withoutsetTargetRequest()ever being called, so the future's memoized key may still correspond to the original target.If someone later "fixes"
pollPooledChannelto usebasePartitionKey()based on this comment, a replayed request could poll a pooled connection using the wrong host's key. Could you reword the comment to mention only the actual consumers (semaphoreacquisition, pool offer, and HTTP/2 registration)? It would also be worth adding a short note inpollPooledChannelexplaining that it intentionally derives the key from the current request.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
client/src/main/java/org/asynchttpclient/netty/channel/ChannelManager.java:451After this change,
poll(Uri, String, ProxyServer, ChannelPoolPartitioning)has no remaining internal callers. SinceChannelManageris a public class, we shouldn't remove it, but it would be good to mark it@Deprecatedand add Javadoc pointing callers topoll(Object)instead. That helps prevent it from being reintroduced internally and sets it up for removal in the next major release.The same could be considered for
pollHttp2(Uri, ...)at line 446 once its remaining internal caller (NettyRequestSenderaround line 1162, in thewaitForHttp2Connectionpath) has been migrated topollHttp2Connection(Object).