#680

Sync batch does not honour BatchPolicy.totalTimeout when waiting for sub-commands; the calling thread can block far beyond it

Openaltaiezioropened 1mo ago

Summary

On the sync batch path, the calling thread waits for batch sub-commands with no time limit. BatchPolicy.totalTimeout is not consulted for that wait. If any sub-command outlives its timeout, the caller stays blocked for as long as the sub-command runs — in a server application that is a request thread, and the exposure repeats per request.

The async batch path already bounds this correctly. This report is about the sync path only.

Version

com.aerospike:aerospike-client-jdk21 10.4.0, and current stage (7577294e). Reproduced on Temurin JDK 21.0.1 and 25.0.3.

What happens

BatchExecutor.execute waits by leaving a try-with-resources block:

try (ExecutorService es = Executors.newThreadPerTaskExecutor(cluster.threadFactory);) {
    for (IBatchCommand command : commands) {
        es.execute(command);
    }
}

Leaving that block calls the default ExecutorService.close(), which in both JDK 21 and JDK 25 is:

while (!terminated) {
    try {
        terminated = awaitTermination(1L, TimeUnit.DAYS);
    } catch (InterruptedException e) { ... }
}

The one-day wait is not a ceiling — when it expires with tasks still running the loop arms another. There is no timeout parameter and nothing to configure.

Why a sub-command can outlive totalTimeout

SyncCommand compares against its deadline only between retries. Within a single attempt, Connection.readFully loops in.read(...) bounded per read by socketTimeout, never in total, so a server that keeps feeding bytes slowly holds one attempt open past totalTimeout. I will file that separately as its own issue, since it is an independent defect and the root cause of this one.

Expected

totalTimeout bounds the batch as a whole, as its javadoc describes. totalTimeout = 0 continues to mean no client-side limit.

Reproducer

No server required. Drives BatchExecutor directly with two sub-commands that stall, so the wait is the only thing under test:

BatchPolicy policy = new BatchPolicy();
policy.totalTimeout = 500;
IBatchCommand[] commands = { stallingCommand(), stallingCommand() };  // each sleeps 10s
BatchExecutor.execute(cluster, policy, commands, new BatchStatus(false));

Observed: returns after 10,035 ms and throws nothing. Expected: an AerospikeException.Timeout at roughly 500 ms.

The async path already does this

Each async sub-command carries its own HashedWheelTimeout, armed against min(totalDeadline, socketDeadline) and re-armed across REGISTERED, DELAY_QUEUE and BATCH_RETRY, with connectTimeout handled separately. The listener is always notified on time. Async also has a live concurrency bound in EventPolicy.maxCommandsInProcess. The sync path has neither.

Other call sites with the same pattern

  • command/Batch.java:743 — batch split retry
  • command/Executor.java:54 — sync scanAll and multi-node commands
  • query/QueryPartitionExecutor.java:99 — explicit es.close()

The scan and query policies default totalTimeout to 0, so those two are unaffected at default settings and only bite a user who sets it.

Suggested fix

Wait explicitly — shutdown(), awaitTermination(remaining), then shutdownNow() and report a client timeout. BatchStatus already keeps the first exception, so a real sub-command error still takes precedence over the synthetic timeout.

StructuredTaskScope.joinUntil(Instant) is the natural fit, but it is still @PreviewFeature in both JDK 21 and 25 and the API changed shape between them, so it is not usable by this library yet.

I have a patch and the test above ready; happy to open a PR if the approach looks right. It covers BatchExecutor and Batch.retryBatch; Executor takes no policy so bounding it needs a signature change across ~10 call sites in AerospikeClient, which I left out pending your preference.

Java 8 line is unaffected — it has no virtual threads on this path.

This report was prepared with AI assistance.

Activity

1
altaiezior1mo ago

The root cause referred to above is now filed separately as #682: socketTimeout bounds each socket read rather than the attempt, so a single attempt can outlive totalTimeout. That issue carries a server-free reproducer.

The two are independent — #682 is why a sub-command overruns, this one is why the overrun reaches the caller — so bounding the wait here is still worthwhile on its own.