Skip to content

Commit 18d9f38

Browse files
authored
feat: Improve virtual thread compatibility (#165)
- Replace synchronized methods with ReentrantLock in AsyncStreamObserver to avoid pinning virtual threads to carrier threads - Replace synchronized singleton in Tracer with holder-class idiom Drive-By: - Remove TemporaryBuffers ThreadLocal caching; inline char[] allocation in Tracer instead - Delete TemporaryBuffers and TemporaryBuffersTest
1 parent f01c77a commit 18d9f38

4 files changed

Lines changed: 92 additions & 128 deletions

File tree

jdbc-core/src/main/java/com/salesforce/datacloud/jdbc/protocol/async/core/AsyncStreamObserver.java

Lines changed: 85 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
import java.util.Queue;
1414
import java.util.concurrent.CompletableFuture;
1515
import java.util.concurrent.CompletionStage;
16+
import java.util.concurrent.locks.ReentrantLock;
1617
import lombok.NonNull;
1718
import org.slf4j.Logger;
1819

@@ -26,7 +27,8 @@
2627
* it would be ambiguous whether all messages have been received). Messages are buffered
2728
* in memory until consumed.</p>
2829
*
29-
* <p>Thread-safety: All state mutations are protected by synchronization on {@code this}.
30+
* <p>Thread-safety: All state mutations are protected by a {@link ReentrantLock}
31+
* (virtual-thread friendly, unlike {@code synchronized}).
3032
* The observer can be closed from any thread.</p>
3133
*
3234
* @param <ReqT> the request message type
@@ -39,6 +41,9 @@ public class AsyncStreamObserver<ReqT, RespT extends AbstractMessage> implements
3941
// for always INITIAL_REQUEST_COUNT outstanding messages.
4042
private static final int INITIAL_REQUEST_COUNT = 16;
4143

44+
// Lock used instead of synchronized to avoid pinning virtual threads to carrier threads
45+
private final ReentrantLock lock = new ReentrantLock();
46+
4247
// The logger which should be used for logging
4348
private final Logger logger;
4449
// The name to use for elapse time logging
@@ -86,52 +91,68 @@ public void beforeStart(ClientCallStreamObserver<ReqT> callStream) {
8691
}
8792

8893
@Override
89-
public synchronized void onNext(RespT value) {
90-
totalResponseSize += value.getSerializedSize();
91-
92-
// If there's a pending future waiting for data, complete it directly
93-
if (pendingFuture != null) {
94-
CompletableFuture<Optional<RespT>> future = pendingFuture;
95-
pendingFuture = null;
96-
future.complete(Optional.of(value));
97-
} else {
98-
// Otherwise buffer the message
99-
buffer.add(value);
100-
}
94+
public void onNext(RespT value) {
95+
lock.lock();
96+
try {
97+
totalResponseSize += value.getSerializedSize();
98+
99+
// If there's a pending future waiting for data, complete it directly
100+
if (pendingFuture != null) {
101+
CompletableFuture<Optional<RespT>> future = pendingFuture;
102+
pendingFuture = null;
103+
future.complete(Optional.of(value));
104+
} else {
105+
// Otherwise buffer the message
106+
buffer.add(value);
107+
}
101108

102-
// Request the next message to keep the pipeline flowing
103-
if (callStream != null) {
104-
callStream.request(1);
109+
// Request the next message to keep the pipeline flowing
110+
if (callStream != null) {
111+
callStream.request(1);
112+
}
113+
} finally {
114+
lock.unlock();
105115
}
106116
}
107117

108118
@Override
109-
public synchronized void onError(Throwable t) {
110-
long elapsed = System.nanoTime() - startNanos;
111-
ElapsedLogger.logFailure(
112-
logger, timingName + ", responseSizeMb=" + totalResponseSize / 1_000_000.0, elapsed, t);
113-
streamEnded = true;
114-
terminalError = t;
115-
116-
// Complete any pending future with the error
117-
if (pendingFuture != null) {
118-
CompletableFuture<Optional<RespT>> future = pendingFuture;
119-
pendingFuture = null;
120-
future.completeExceptionally(t);
119+
public void onError(Throwable t) {
120+
lock.lock();
121+
try {
122+
long elapsed = System.nanoTime() - startNanos;
123+
ElapsedLogger.logFailure(
124+
logger, timingName + ", responseSizeMb=" + totalResponseSize / 1_000_000.0, elapsed, t);
125+
streamEnded = true;
126+
terminalError = t;
127+
128+
// Complete any pending future with the error
129+
if (pendingFuture != null) {
130+
CompletableFuture<Optional<RespT>> future = pendingFuture;
131+
pendingFuture = null;
132+
future.completeExceptionally(t);
133+
}
134+
} finally {
135+
lock.unlock();
121136
}
122137
}
123138

124139
@Override
125-
public synchronized void onCompleted() {
126-
long elapsed = System.nanoTime() - startNanos;
127-
ElapsedLogger.logSuccess(logger, timingName + ", responseSizeMb=" + totalResponseSize / 1_000_000.0, elapsed);
128-
streamEnded = true;
129-
130-
// Complete any pending future with empty (only if buffer is also empty)
131-
if (pendingFuture != null && buffer.isEmpty()) {
132-
CompletableFuture<Optional<RespT>> future = pendingFuture;
133-
pendingFuture = null;
134-
future.complete(Optional.empty());
140+
public void onCompleted() {
141+
lock.lock();
142+
try {
143+
long elapsed = System.nanoTime() - startNanos;
144+
ElapsedLogger.logSuccess(
145+
logger, timingName + ", responseSizeMb=" + totalResponseSize / 1_000_000.0, elapsed);
146+
streamEnded = true;
147+
148+
// Complete any pending future with empty (only if buffer is also empty)
149+
if (pendingFuture != null && buffer.isEmpty()) {
150+
CompletableFuture<Optional<RespT>> future = pendingFuture;
151+
pendingFuture = null;
152+
future.complete(Optional.empty());
153+
}
154+
} finally {
155+
lock.unlock();
135156
}
136157
}
137158

@@ -148,31 +169,36 @@ public synchronized void onCompleted() {
148169
*
149170
* @return a CompletionStage for the next element
150171
*/
151-
public synchronized CompletionStage<Optional<RespT>> requestNext() {
152-
// If there are buffered messages, return one immediately
153-
if (!buffer.isEmpty()) {
154-
return CompletableFuture.completedFuture(Optional.of(buffer.poll()));
155-
}
172+
public CompletionStage<Optional<RespT>> requestNext() {
173+
lock.lock();
174+
try {
175+
// If there are buffered messages, return one immediately
176+
if (!buffer.isEmpty()) {
177+
return CompletableFuture.completedFuture(Optional.of(buffer.poll()));
178+
}
156179

157-
// If stream already ended, return immediately
158-
if (streamEnded) {
159-
if (terminalError != null) {
160-
CompletableFuture<Optional<RespT>> future = new CompletableFuture<>();
161-
future.completeExceptionally(terminalError);
162-
return future;
180+
// If stream already ended, return immediately
181+
if (streamEnded) {
182+
if (terminalError != null) {
183+
CompletableFuture<Optional<RespT>> future = new CompletableFuture<>();
184+
future.completeExceptionally(terminalError);
185+
return future;
186+
}
187+
// Empty signals success
188+
return CompletableFuture.completedFuture(Optional.empty());
163189
}
164-
// Empty signals success
165-
return CompletableFuture.completedFuture(Optional.empty());
166-
}
167190

168-
// Create a new future that will be completed when data arrives
169-
// Fail if there is an unconsumed future (would indicate concurrent requestNext calls
170-
// which are not supported)
171-
if (pendingFuture != null) {
172-
throw new IllegalStateException("Unfulfilled previous future when next is requested");
191+
// Create a new future that will be completed when data arrives
192+
// Fail if there is an unconsumed future (would indicate concurrent requestNext calls
193+
// which are not supported)
194+
if (pendingFuture != null) {
195+
throw new IllegalStateException("Unfulfilled previous future when next is requested");
196+
}
197+
pendingFuture = new CompletableFuture<>();
198+
return pendingFuture;
199+
} finally {
200+
lock.unlock();
173201
}
174-
pendingFuture = new CompletableFuture<>();
175-
return pendingFuture;
176202
}
177203

178204
/**

jdbc-util/src/main/java/com/salesforce/datacloud/jdbc/tracing/TemporaryBuffers.java

Lines changed: 0 additions & 33 deletions
This file was deleted.

jdbc-util/src/main/java/com/salesforce/datacloud/jdbc/tracing/Tracer.java

Lines changed: 7 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -13,17 +13,12 @@ public class Tracer {
1313

1414
private static final String INVALID = "0000000000000000";
1515

16-
private static volatile Tracer instance;
16+
private static final class Holder {
17+
static final Tracer INSTANCE = new Tracer();
18+
}
1719

18-
public static synchronized Tracer get() {
19-
if (instance == null) {
20-
synchronized (Tracer.class) {
21-
if (instance == null) {
22-
instance = new Tracer();
23-
}
24-
}
25-
}
26-
return instance;
20+
public static Tracer get() {
21+
return Holder.INSTANCE;
2722
}
2823

2924
public String nextSpanId() {
@@ -45,7 +40,7 @@ private String fromLong(long id) {
4540
if (id == 0) {
4641
return INVALID;
4742
}
48-
char[] result = TemporaryBuffers.chars(SPAN_ID_HEX_LENGTH);
43+
char[] result = new char[SPAN_ID_HEX_LENGTH];
4944
EncodingUtils.longToBase16String(id, result, 0);
5045
return new String(result, 0, SPAN_ID_HEX_LENGTH);
5146
}
@@ -74,7 +69,7 @@ private String fromLongs(long traceIdLongHighPart, long traceIdLongLowPart) {
7469
if (traceIdLongHighPart == 0 && traceIdLongLowPart == 0) {
7570
return INVALID;
7671
}
77-
char[] chars = TemporaryBuffers.chars(TRACE_ID_HEX_LENGTH);
72+
char[] chars = new char[TRACE_ID_HEX_LENGTH];
7873
EncodingUtils.longToBase16String(traceIdLongHighPart, chars, 0);
7974
EncodingUtils.longToBase16String(traceIdLongLowPart, chars, 16);
8075
return new String(chars, 0, TRACE_ID_HEX_LENGTH);

jdbc-util/src/test/java/com/salesforce/datacloud/jdbc/tracing/TemporaryBuffersTest.java

Lines changed: 0 additions & 24 deletions
This file was deleted.

0 commit comments

Comments
 (0)