Skip to content

Commit 482d068

Browse files
committed
Skip defensive copy for completed prepare cache futures
When the cached CompletableFuture is already done, return it directly instead of creating a defensive copy via thenApply(x -> x). Completed futures are immutable (cancel/complete are no-ops), so the copy only served to release the caller's strong reference to the cached value, causing premature weak-value eviction under GC pressure. This led to repeated PREPARE requests being sent to all nodes on every execution, as the cache entry would be garbage-collected between calls. With prepare-on-all-nodes=true (default), each eviction multiplied the re-prepare cost by the cluster node count. The defensive copy is still used for in-flight futures to protect the shared cache entry from cancellation by concurrent waiters. Ref: CUSTOMER-372
1 parent 9149592 commit 482d068

2 files changed

Lines changed: 98 additions & 2 deletions

File tree

core/src/main/java/com/datastax/oss/driver/internal/core/cql/CqlPrepareAsyncProcessor.java

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -162,8 +162,16 @@ public CompletionStage<PreparedStatement> process(
162162
});
163163
}
164164
}
165-
// Return a defensive copy. So if a client cancels its request, the cache won't be impacted
166-
// nor a potential concurrent request.
165+
if (result.isDone()) {
166+
// Once a CompletableFuture is completed, cancel(), complete(), and
167+
// completeExceptionally() are no-ops (return false), so returning the cached
168+
// instance directly is safe from concurrent callers. This also keeps the cache
169+
// entry alive via the caller's strong reference, preventing premature weak-value
170+
// eviction under GC pressure.
171+
return result;
172+
}
173+
// Defensive copy for in-flight preparations only: protects the shared cached future
174+
// from cancellation by one of multiple concurrent waiters.
167175
return result.thenApply(x -> x); // copy() is available only since Java 9
168176
} catch (ExecutionException e) {
169177
return CompletableFutures.failedFuture(e.getCause());
Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,88 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one
3+
* or more contributor license agreements. See the NOTICE file
4+
* distributed with this work for additional information
5+
* regarding copyright ownership. The ASF licenses this file
6+
* to you under the Apache License, Version 2.0 (the
7+
* "License"); you may not use this file except in compliance
8+
* with the License. You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing, software
13+
* distributed under the License is distributed on an "AS IS" BASIS,
14+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
15+
* See the License for the specific language governing permissions and
16+
* limitations under the License.
17+
*/
18+
package com.datastax.oss.driver.internal.core.cql;
19+
20+
import static org.assertj.core.api.Assertions.assertThat;
21+
22+
import com.datastax.oss.driver.api.core.cql.PrepareRequest;
23+
import com.datastax.oss.driver.api.core.cql.PreparedStatement;
24+
import com.datastax.oss.driver.shaded.guava.common.cache.Cache;
25+
import java.util.Optional;
26+
import java.util.concurrent.CompletableFuture;
27+
import java.util.concurrent.CompletionStage;
28+
import org.junit.Before;
29+
import org.junit.Test;
30+
import org.mockito.Mockito;
31+
32+
/**
33+
* Unit tests for {@link CqlPrepareAsyncProcessor} focusing on the caching behavior of {@link
34+
* CqlPrepareAsyncProcessor#process} with respect to defensive copies and weak-value retention.
35+
*/
36+
public class CqlPrepareAsyncProcessorTest {
37+
38+
private CqlPrepareAsyncProcessor processor;
39+
private Cache<PrepareRequest, CompletableFuture<PreparedStatement>> cache;
40+
41+
@Before
42+
public void setup() {
43+
processor = new CqlPrepareAsyncProcessor(Optional.empty());
44+
cache = processor.getCache();
45+
}
46+
47+
/**
48+
* When the cached future is already completed, process() should return the exact same instance
49+
* (identity). This ensures callers hold a strong reference to the cached CF, preventing
50+
* weak-value eviction under GC pressure.
51+
*/
52+
@Test
53+
public void should_return_cached_future_directly_when_already_completed() throws Exception {
54+
PrepareRequest request = new DefaultPrepareRequest("SELECT 1");
55+
PreparedStatement ps = Mockito.mock(PreparedStatement.class);
56+
57+
// Pre-populate cache with a completed future
58+
CompletableFuture<PreparedStatement> completed = CompletableFuture.completedFuture(ps);
59+
cache.put(request, completed);
60+
61+
// process() should return the exact same object
62+
CompletionStage<PreparedStatement> returned = processor.process(request, null, null, "test");
63+
64+
assertThat(returned).isSameAs(completed);
65+
}
66+
67+
/**
68+
* When the cached future is still in-flight (not yet done), process() should return a defensive
69+
* copy to protect the cache from cancellation by the caller.
70+
*/
71+
@Test
72+
public void should_return_defensive_copy_when_future_is_in_flight() throws Exception {
73+
PrepareRequest request = new DefaultPrepareRequest("SELECT 1");
74+
75+
// Pre-populate cache with an incomplete future
76+
CompletableFuture<PreparedStatement> inFlight = new CompletableFuture<>();
77+
cache.put(request, inFlight);
78+
79+
CompletionStage<PreparedStatement> returned = processor.process(request, null, null, "test");
80+
81+
// Should NOT be the same instance
82+
assertThat(returned).isNotSameAs(inFlight);
83+
84+
// Cancelling the returned copy should NOT affect the cached future
85+
returned.toCompletableFuture().cancel(false);
86+
assertThat(inFlight.isCancelled()).isFalse();
87+
}
88+
}

0 commit comments

Comments
 (0)