Skip to content

Commit 83888d1

Browse files
committed
fix: use separate retryScheduler in DispatchingTransport to prevent deadlock
BulkIngester submits afterBulk callbacks to bulkScheduler via scheduler.submit(). Its single scheduler thread can block inside FnCondition.awaitUninterruptibly() while waiting for a concurrency slot (requestsInFlightCount < maxConcurrentRequests). When DispatchingTransport shared bulkScheduler for retry tasks (introduced in the previous commit), that created a deadlock: 1. bulkScheduler thread blocks in sendRequestCondition.awaitUninterruptibly() waiting for an in-flight slot to free up. 2. Freeing a slot requires result.completeExceptionally() to fire, which requires the retry task to run. 3. The retry task is queued in bulkScheduler, but bulkScheduler is blocked. 4. Neither side can proceed — the connector task stays RUNNING indefinitely. Fix: give DispatchingTransport its own dedicated single-threaded retryScheduler so retry tasks are never blocked by BulkIngester's flush tasks.
1 parent 24c1341 commit 83888d1

1 file changed

Lines changed: 19 additions & 4 deletions

File tree

src/main/java/io/confluent/connect/elasticsearch/ElasticsearchClient.java

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -110,6 +110,7 @@ public class ElasticsearchClient {
110110
private final RestClient restClient;
111111
private final ExecutorService callbackDispatcher;
112112
private final ScheduledExecutorService bulkScheduler;
113+
private final ScheduledExecutorService retryScheduler;
113114
private final ElasticsearchSinkConnectorConfig config;
114115
private final ErrantRecordReporter reporter;
115116
private final Time clock;
@@ -159,18 +160,30 @@ public ElasticsearchClient(
159160
}
160161
);
161162

163+
// retryScheduler is intentionally separate from bulkScheduler. BulkIngester submits
164+
// afterBulk callbacks to bulkScheduler and its single thread can block inside
165+
// FnCondition.awaitUninterruptibly() waiting for a concurrency slot. If retry tasks
166+
// shared that same thread they would never run, permanently deadlocking the flush.
167+
String retryPrefix = connectorName + "-" + taskId + "-es-retry-";
168+
AtomicInteger retryCounter = new AtomicInteger(0);
169+
this.retryScheduler = Executors.newScheduledThreadPool(
170+
1,
171+
r -> {
172+
Thread t = new Thread(r, retryPrefix + retryCounter.getAndIncrement());
173+
t.setDaemon(true);
174+
return t;
175+
}
176+
);
177+
162178
// DispatchingTransport breaks a lock-ordering deadlock between BulkIngester's FnCondition lock
163179
// and the HLRC NIO pool lock: by dispatching completion callbacks to a separate executor,
164180
// the IO thread never directly acquires the FnCondition lock.
165181
// It also retries transport-level failures (e.g. SocketTimeoutException), equivalent to what
166182
// the original callWithRetries() provided in buildConsumer().
167-
// bulkScheduler is shared with BulkIngester's flush scheduling. schedule() merely enqueues
168-
// the retry task without occupying the scheduler thread, so sharing is safe and does not
169-
// warrant a separate pool.
170183
RestClientTransport rawTransport = new RestClientTransport(
171184
restClient, new JacksonJsonpMapper());
172185
DispatchingTransport transport = new DispatchingTransport(
173-
rawTransport, callbackDispatcher, bulkScheduler,
186+
rawTransport, callbackDispatcher, retryScheduler,
174187
config.maxRetries(), config.retryBackoffMs());
175188
this.client = new co.elastic.clients.elasticsearch.ElasticsearchClient(transport);
176189

@@ -211,6 +224,7 @@ public ElasticsearchClient(
211224
this.restClient = restClient;
212225
this.callbackDispatcher = Executors.newCachedThreadPool();
213226
this.bulkScheduler = Executors.newScheduledThreadPool(1);
227+
this.retryScheduler = Executors.newScheduledThreadPool(1);
214228
this.bulkIngester = bulkIngester;
215229
this.client = null;
216230
this.esVersion = UNKNOWN_VERSION_TAG;
@@ -527,6 +541,7 @@ private void closeResources() {
527541
// shutdown() not shutdownNow(): bulkIngester.close() already drained all pending flushes,
528542
// so only future periodic triggers remain — no need to interrupt any running task.
529543
bulkScheduler.shutdown();
544+
retryScheduler.shutdown();
530545
callbackDispatcher.shutdown();
531546
try {
532547
restClient.close();

0 commit comments

Comments
 (0)