Skip to content

Introduce necessary executors and implement sleepAsync - #3

Draft
stIncMale wants to merge 10 commits into
introduceRetryPolicyfrom
sleepAsync
Draft

Introduce necessary executors and implement sleepAsync#3
stIncMale wants to merge 10 commits into
introduceRetryPolicyfrom
sleepAsync

Conversation

@stIncMale

@stIncMale stIncMale commented Jun 30, 2026

Copy link
Copy Markdown
Owner

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 stIncMale self-assigned this Jun 30, 2026
@stIncMale
stIncMale force-pushed the sleepAsync branch 5 times, most recently from 634c8dd to 85b1c3d Compare July 3, 2026 07:39
Comment thread driver-core/src/main/com/mongodb/internal/async/AsyncRunnable.java Outdated
commonExecutor().uncaughtError((Error) exception);
}
if (exception != null) {
logger.error("A task completed abruptly", exception);

@stIncMale stIncMale Jul 20, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@stIncMale stIncMale Jul 21, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 6479ad9.

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

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vbabanin, I created mongodb#2028. Can't create that in any branch that is based on the backpressure branch, as the required master changes are missing. Once I rebase backpressure on top of the current master, I'll change that PR to target backpressure.

For now, please review the requested change there.

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

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what getException(r, t) is, but the explanation still holds.

Resolved.

@stIncMale stIncMale Aug 7, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jyemin This comment was about the code that is currently in MongoThreadPoolExecutor.unwrapThrowable (former getException, which was accepting both of the parameters that the method propagateTaskFailureToUncaughtExceptionHandler accepts currently).

If the runnable r is a Future, we unwrap the exception from it by calling Future.get. The thread that does this is the worker thread, which could have been interrupted by the failed task. Some future implementations in some JDKs, like ForkJoinTask (in OpenJDK JDK 17, for example, but not in OpenJDK JDK 23), throw InterruptedException in this case despite the future having been completed (isDone).

To unwrap the exception from such a Future implementation despite the thread's interrupted status, we agreed to call unwrapThrowable (former getException) recursively, as when a Future implementation throws InterruptedException, it also clears the interrupted status.

However, it turned out that the Future implementation used by ScheduledThreadPoolExecutor/ThreadPoolExecutor does not have the aforementioned behavior of throwing InterruptedException if isDone. So I could not test the code change, and decided not to do it at all.

Alternatively, since we do the isDone check, we could have called Future.resultNow instead of get, which implements the same idea (calls get multiple times, though in a loop instead of recursively), but we can't because that method is from Java SE 19.

P.S. I can still implement the above, though without testing the code for the reason described, if you think we should do that.

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

@stIncMale stIncMale Jul 21, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is about updating Jira, so not blocking merge.

Comment thread driver-core/src/main/com/mongodb/internal/thread/MongoThreadPoolExecutor.java Outdated
*
* @see #uncaughtError(Error)
*/
private final ExecutorService uncaughtExceptionHandlerExecutor;

@stIncMale stIncMale Jul 21, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@stIncMale stIncMale Jul 29, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 stIncMale Jul 30, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Furthermore, AI made it abundantly clear to me that calling UncaughtExceptionHandler is never a problem in terms of blocking a thread (it also made me feel quite dumb): if it is called, then the thread has been or is going to be terminated (the Java SE API documentation says "when the given thread terminates", which is not specific enough). Thus, it does not matter whether UncaughtExceptionHandler is blocking or not. And since CommonExecutor.singleThreadScheduler replaces the single worker thread (we verify that in CommonExecutorTest.singleSchedulingThreadIsReplacedIfTerminated), thread termination is also not a problem.

Comment thread driver-core/src/main/com/mongodb/internal/thread/MongoThreadPoolExecutor.java Outdated
Comment thread driver-core/src/main/com/mongodb/internal/thread/CommonExecutor.java Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants