Introduce necessary executors and implement sleepAsync by stIncMale · Pull Request #3 · stIncMale/mongo-java-driver

stIncMale · 2026-06-30T14:20:25Z

AI usage

AI was used only to review and to suggest ways to deal with the serious bug it discovered (see below).

AI identified a serious bug with CommonExecutor offloading scheduled tasks to another Executor, which I failed to think about on my own. AI also expressed ideas on how one may deal with that problem. One of them I manually implemented in DefaultAsyncClientExecutor.

JAVA-6240

stIncMale · 2026-07-20T21:29:35Z

+        return asyncFunctionSuccessfulResult.get().getNullable();
+    }
+
+    private void sleep(final Duration duration) {


VAKOTODO Make static.

Done in 12602ec.

@vbabanin Please review and resolve the thread if you consider the issue resolved.

stIncMale · 2026-07-20T21:51:25Z

-        public void onResult(@Nullable final R attemptSuccessfulResult, @Nullable final Throwable attemptFailedResult) {
-            if (attemptFailedResult != null) {
+        MutableValue<MutableValue<R>> asyncFunctionSuccessfulResult = new MutableValue<>();
+        beginAsync().thenRunWhileLoop(() -> asyncFunctionSuccessfulResult.getNullable() == null, iterationCallback -> {


VAKOTODO thenRunWhileLoop ends if the body fails. Rewrite taking this into account.

@vbabanin, I don't think this code can be rewritten in a way that gets rid of asyncFunctionSuccessfulResult (unless, of course, AsyncCallbackLoop is used directly, as we will be able to call LoopControl.breakAndCompleteIf). The problem is not to break the loop on exception, but to break the loop when asyncFunction.get fails, but to break it when the function succeeds. The only way to achieve that with the AsyncRunnable.thenRunWhileLoop method is via its whileCheck parameter, which has to be implemented using something like asyncFunctionSuccessfulResult.

stIncMale · 2026-07-20T22:00:14Z

        return thenRun(callback -> {
            new RetryingAsyncCallbackSupplier<Void>(
+                    // `AsyncClientExecutor` is not needed, given the contract of `SimpleRetryPolicy`, `RetryingAsyncCallbackSupplier`
                    AsyncClientExecutor.unimplemented(),


VAKOTODO Replace AsyncClientExecutor.unimplemented with AsyncClientExecutor.NO_OP.

Done in 2546e07.

@vbabanin Please review and resolve the thread if you consider the issue resolved.

stIncMale · 2026-07-20T22:49:25Z

+    @Override
+    protected void afterExecute(final Runnable r, @Nullable final Throwable t) {
+        super.afterExecute(r, t);
+        assertTrue(r instanceof Future<?>);


VAKOTODO Leave a comment explaining this assertion.

This assertions was removed because it is no longer needed. It was needed to help understand the assertNull(t) assertion that followed, but that assertion no longer holds, because our afterExecute can now throw, which sometimes causes ThreadPoolExecutor to call afterExecute again.

@vbabanin Please review and resolve the thread if you consider the issue resolved.

stIncMale · 2026-07-20T23:21:16Z

+            commonExecutor().uncaughtError((Error) exception);
+        }
+        if (exception != null) {
+            logger.error("A task completed abruptly", exception);


VAKOTODO If t != null (and so is exception), then conceptually do commonExecutor().uncaughtError(new AssertionError(exception)), but make sure not to double-wrap AssertionError. This way Logger is not needed at all.

Leave a comment about uncaughtError logging to stdout in there is no better action, as documented by ThreadGroup.uncaughtException.

Discuss this with @vbabanin again. Given that one of the motivations behind overriding afterExecute was to do it "Instead of modifying every Runnable/Callable we schedule", maybe we should not treat uncaught Exceptions as bugs.

Discussed, we will treat Exceptions in afterExecute as AssertionError, because this gives an application a standard programmatic way to react to a driver bug when asynchronous driver API is used with MongoThreadPoolExecutor being used as the IO executor.

Update the root AGENTS.md with the new Code correctness rules section that explains the rule for all tasks that run in MongoThreadPoolExecutor/MongoScheduledThreadPoolExecutor (make sure the change is in line with mongodb@dd814f8).

Done in 6479ad9.

@vbabanin Please review. I will modify AGENTS.md later.

stIncMale · 2026-07-21T02:26:24Z

+                return assertNotNull(e.getCause());
+            } catch (InterruptedException e) {
+                // not else to do but to reinstate the interrupted status
+                Thread.currentThread().interrupt();


VAKOTODO
Do

try { return getException(r, t); } finally { // not else to do but to reinstate the interrupted status Thread.currentThread().interrupt(); }

This way we get the exception even if the thread was interrupted. There is no harm, only benefits.

I made the change, then wanted to test it, and did not find a situation in which here a completed Future (FutureTask) would have thrown InterruptedException. So I rolled back the change, leaving the code mostly similar to the example in the Java SE API documentation for afterExecute.

@vbabanin Please review and resolve the thread if you consider the issue resolved.

stIncMale · 2026-07-21T02:45:35Z

+ *     All {@link Throwable}s are logged.</li>
+ * </ul>
+ */
+public final class MongoThreadPoolExecutor extends ThreadPoolExecutor {


VAKOTODO Update the description of https://jira.mongodb.org/browse/JAVA-6109: mention that all other executor implementations / single threads should be replaces either with virtual threads (https://jira.mongodb.org/browse/JAVA-4930 - distant future), or MongoThreadPoolExecutor/MongoScheduledThreadPoolExecutor (this includes the executors created in AsynchronousTlsChannelGroup, NettyStreamFactoryFactory (NioEventLoopGroup)), unless it's an IO executor supplied by an application.

Also mention in that ticket to document that the executors supplied by applications should themselves make sure uncaught Throwables are not swallowed, potentially the same way MongoThreadPoolExecutor/MongoScheduledThreadPoolExecutor do it.

stIncMale · 2026-07-21T22:44:04Z

+    }
+
+    @Nullable
+    private static Throwable getException(final Runnable r, @Nullable final Throwable t) {


VAKOTODO Call commonExecutor().uncaughtError only if the Throwable is in a Future. Otherwise, the Throwable will be delivered to the uncaught exception handler anyway, and our call will cause an extra handler invocation.

As of 6479ad9, commonExecutor().uncaughtError no longer exists, and afterExecute passes all task failures to the UncaughtExceptionHandler by throwing them (unless they have already been thrown).

@vbabanin Please review and resolve the thread if you consider the issue resolved.

stIncMale · 2026-07-21T23:55:22Z

+     *
+     * @see #uncaughtError(Error)
+     */
+    private final ExecutorService uncaughtExceptionHandlerExecutor;


VAKOTODO We don't need this. It is always OK to call Thread.currentThread().getUncaughtExceptionHandler().

Leave a code comment explaining how this may be a problem. But that is very unlikely, and we are OK with taking that risk for now, instead of complicating the solution.

See #3 (comment).

Also, I failed to see again how executing UncaughtExceptionHandler may be a problem introduced by one MongoClient but affecting another MongoClient. I know we discussed this, and I saw how this can be, but I can't see that anymore.

@vbabanin Please review and resolve the thread if you consider the issue resolved.

stIncMale · 2026-07-22T00:03:42Z

+        Throwable exception = getException(r, t);
+        if (exception instanceof Error && t == null) {
+            // the `Error` is held in `r`, and would have not been thrown to be handled by the uncaught exception handler
+            commonExecutor().uncaughtError((Error) exception);


VAKOTODO Instead of calling the uncaught exception handler explicitly, throw from afterExecute, and let the handler be called the way it is always called. Make sure this works out OK even for CommonExecutor.scheduler.

Done in 6479ad9.

@vbabanin Please review and resolve the thread if you consider the issue resolved.

stIncMale · 2026-07-23T21:57:41Z

+     */
+    ScheduledFuture<?> schedule(final Runnable command, final Duration delay, final Executor executor) {
+        try {
+            return scheduler.schedule(() -> executor.execute(command), delay.toNanos(), NANOSECONDS);


VAKOTODO Depending on the RejectedExecutionHandler on executor, which may come from an application, the scheduler thread may end up executing application code (the command or anything else, really). We are OK taking this risk for now, an need to leave a code comment about it.

The alternative is not to have the single common scheduler thread, and instead, create one in DefaultAsyncClientExecutor if backingExecutor is not ScheduledExecutorService.

Done in 6479ad9.

@vbabanin Please review and resolve the thread if you consider the issue resolved.

stIncMale self-assigned this Jun 30, 2026

stIncMale force-pushed the sleepAsync branch 5 times, most recently from 634c8dd to 85b1c3d Compare July 3, 2026 07:39

stIncMale added 3 commits July 3, 2026 02:12

Introduce sleepAsync

b9e04fd

JAVA-6240

Thread AsyncClientExecutor through to make it accessible

7680946

JAVA-6240

Add backoff to RetryAttemptInfo

8951205

JAVA-6240

stIncMale force-pushed the sleepAsync branch from 85b1c3d to 8951205 Compare July 3, 2026 08:13

stIncMale commented Jul 20, 2026

View reviewed changes

stIncMale commented Jul 21, 2026

View reviewed changes

stIncMale commented Jul 22, 2026

View reviewed changes

stIncMale commented Jul 23, 2026

View reviewed changes

stIncMale added 5 commits July 27, 2026 20:41

Merge branch 'introduceRetryPolicy' into sleepAsync

1f7f170

Make RetryingSyncSupplier.sleep static

12602ec

Replace AsyncClientExecutor.unimplemented -> NO_OP

2546e07

Remove dead code

33f93bb

Do the design changes as discussed

6479ad9

Conversation

stIncMale commented Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

AI usage

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stIncMale Jul 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stIncMale Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stIncMale Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stIncMale Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stIncMale Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stIncMale Jul 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

stIncMale commented Jun 30, 2026 •

edited

Loading

stIncMale Jul 20, 2026 •

edited

Loading

stIncMale Jul 21, 2026 •

edited

Loading

stIncMale Jul 21, 2026 •

edited

Loading

stIncMale Jul 21, 2026 •

edited

Loading

stIncMale Jul 29, 2026 •

edited

Loading

stIncMale Jul 22, 2026 •

edited

Loading