Introduce necessary executors and implement sleepAsync - #3
Conversation
634c8dd to
85b1c3d
Compare
JAVA-6240
JAVA-6240
| commonExecutor().uncaughtError((Error) exception); | ||
| } | ||
| if (exception != null) { | ||
| logger.error("A task completed abruptly", exception); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
@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(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I'm not sure what getException(r, t) is, but the explanation still holds.
Resolved.
There was a problem hiding this comment.
@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 { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This is about updating Jira, so not blocking merge.
| * | ||
| * @see #uncaughtError(Error) | ||
| */ | ||
| private final ExecutorService uncaughtExceptionHandlerExecutor; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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
CommonExecutoroffloading scheduled tasks to anotherExecutor, 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 inDefaultAsyncClientExecutor.JAVA-6240