Reuse the per-request partition key instead of recomputing it#2231
Reuse the per-request partition key instead of recomputing it#2231pavel-ptashyts wants to merge 3 commits into
Conversation
The base (host/scheme/port) partition key was recomputed — and a fresh key object allocated — at every site that needs it within one request: the connection-semaphore acquire, each pool poll, the completion offer, and the HTTP/2 registration. In pollPooledChannel it was computed twice on a single H2-miss (once for the HTTP/2 registry poll, once for the HTTP/1.1 pool poll). The key is immutable for a given target, so all but the first computation were pure young-gen churn. NettyResponseFuture.basePartitionKey() now memoizes the key. It depends only on targetRequest (host/scheme/port + virtualHost) and the final proxyServer, so it is invalidated in exactly one place — setTargetRequest, the sole post-construction writer of targetRequest (e.g. a redirect/retry to a different host). Behaviour is unchanged: the memoized key equals a fresh computation and is refreshed whenever the target changes, so a redirect can never reuse a connection keyed to the previous host. pollPooledChannel computes the base key once and passes it to the pollHttp2Connection(key)/poll(key) overloads instead of recomputing it inside each channelManager call. Adds a NettyResponseFuture unit test that the base key is memoized (same instance on repeat calls), equals a fresh computation, and is invalidated to the new host's key on setTargetRequest. Existing connection-pool, redirect-connection-usage and round-robin integration tests pass unchanged. No public API change. Fixes finding AsyncHttpClient#7.
| 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 |
There was a problem hiding this comment.
The comment lists "pool poll/offer" as sites that use this memoized key, but pollPooledChannel never calls basePartitionKey(). 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 without setTargetRequest() ever being called, so the future's memoized key may still correspond to the original target.
If someone later "fixes" pollPooledChannel to use basePartitionKey() 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 (semaphore acquisition, pool offer, and HTTP/2 registration)? It would also be worth adding a short note in pollPooledChannel explaining that it intentionally derives the key from the current request.
There was a problem hiding this comment.
client/src/main/java/org/asynchttpclient/netty/channel/ChannelManager.java:451
After this change, poll(Uri, String, ProxyServer, ChannelPoolPartitioning) has no remaining internal callers. Since ChannelManager is a public class, we shouldn't remove it, but it would be good to mark it @Deprecated and add Javadoc pointing callers to poll(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 (NettyRequestSender around line 1162, in the waitForHttp2Connection path) has been migrated to pollHttp2Connection(Object).
There was a problem hiding this comment.
Done. Reworded the basePartitionKey() javadoc to list only the real consumers (semaphore acquire, pool offer, HTTP/2 registration) and added a note that the pool/H2 poll paths intentionally derive the key from the live request (not the future's memoized key) because a filter replay can reuse a future without updating its targetRequest. Also added an explanatory comment in pollPooledChannel.
Marked both poll(Uri, …) and pollHttp2(Uri, …) @deprecated (kept for binary compatibility) pointing at poll(Object) / pollHttp2Connection(Object). Migrated waitForHttp2Connection to compute the key once before the retry loop and call pollHttp2Connection(Object), and removed the private pollHttp2 helper — the last caller of the 4-arg overload.
| if (addresses.size() > 1) { | ||
| ordered = rrSelector.rotate(host, addresses); | ||
| InetAddress chosen = ordered.get(0).getAddress(); | ||
| Object baseKey = request.getChannelPoolPartitioning().getPartitionKey(uri, request.getVirtualHost(), proxyServer); |
There was a problem hiding this comment.
Small follow-up in the same spirit as this PR: the round-robin path still recomputes the base key inline here, even though newFuture is already in scope. newFuture.basePartitionKey() returns an equivalent key (targetRequest, partitioning, and proxyServer are all fixed at construction) and would also warm the memo for the later per-host semaphore acquisition.
This is a minor optimization since it only applies with LoadBalance.ROUND_ROBIN and multiple resolved IPs, but it removes the same redundant allocation that this PR eliminates elsewhere.
There was a problem hiding this comment.
Good catch to raise it, but I believe this is unsafe as written, so I kept the inline computation and documented why (comment at line 217). basePartitionKey() reads the future's targetRequest, which is only kept in sync on the redirect path (Redirect30xInterceptor). The filter-replay path (replayRequest → sendNextRequest) reuses the future via setCurrentRequest without calling setTargetRequest. So on a host-rewriting replay in ROUND_ROBIN + multi-IP mode, newFuture.basePartitionKey() would return the previous host's base key wrapped around the new host's IP — the exact cross-host mis-keying you flagged for pollPooledChannel.
The principled alternative is to make replayRequest call future.setTargetRequest(newRequest) (which would also fix other getTargetRequest() consumers like TimeoutTimerTask/KeepAliveStrategy), but that's a broader correctness change I'd prefer to do in a separate PR. Happy to file a follow-up issue.
| // 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) { |
There was a problem hiding this comment.
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 setTargetRequest(), and making basePartitionKey() a simple read, would provide the same allocation savings without introducing a reader-side write. It also removes any concurrency considerations if the threading model changes in the future.
There was a problem hiding this comment.
Done. The base key is now computed eagerly in the constructor and recomputed in setTargetRequest via a computeBasePartitionKey() helper (null-guarded for the unit tests that construct a future with no request/partitioning), and basePartitionKey() is a plain read of the volatile field — no more reader-side write, so no concurrency considerations if the threading model changes.
…oll-key intent - NettyResponseFuture: compute the base partition key eagerly in the constructor and recompute it in setTargetRequest (via computeBasePartitionKey); basePartitionKey() is now a plain read of the volatile field, removing the reader-side write of the lazy memoize. The helper tolerates the null request/partitioning used by some unit tests. Reword the doc to name all consumers (semaphore acquire, pool offer, HTTP/2 registration) and note that the poll paths intentionally derive their key from the live request. - ChannelManager: mark the now-unused poll(Uri,...) and pollHttp2(Uri,...) overloads @deprecated pointing at the Object overloads (kept for binary compatibility). - NettyRequestSender.waitForHttp2Connection: compute the HTTP/2 poll key once before the retry loop and call pollHttp2Connection(Object); drop the private pollHttp2 helper (last caller of the deprecated overload). - Document why the round-robin base key and the pool/H2 poll keys are derived from the live request rather than future.basePartitionKey(): a filter replay can reuse a future without updating targetRequest, so the memoized key can lag a host-rewriting replay. (Declining the review suggestion to use newFuture.basePartitionKey() at the round-robin site for this reason.)
2 participants
The base (host/scheme/port) partition key was recomputed — and a fresh key object allocated — at every site that needs it within one request: the connection-semaphore acquire, each pool poll, the completion offer, and the HTTP/2 registration. In pollPooledChannel it was computed twice on a single H2-miss (once for the HTTP/2 registry poll, once for the HTTP/1.1 pool poll). The key is immutable for a given target, so all but the first computation were pure young-gen churn.
NettyResponseFuture.basePartitionKey() now memoizes the key. It depends only on targetRequest (host/scheme/port + virtualHost) and the final proxyServer, so it is invalidated in exactly one place — setTargetRequest, the sole post-construction writer of targetRequest (e.g. a redirect/retry to a different host). Behaviour is unchanged: the memoized key equals a fresh computation and is refreshed whenever the target changes, so a redirect can never reuse a connection keyed to the previous host. pollPooledChannel computes the base key once and passes it to the pollHttp2Connection(key)/poll(key) overloads instead of recomputing it inside each channelManager call.
Adds a NettyResponseFuture unit test that the base key is memoized (same instance on repeat calls), equals a fresh computation, and is invalidated to the new host's key on setTargetRequest. Existing connection-pool, redirect-connection-usage and round-robin integration tests pass unchanged. No public API change.