Introduce necessary executors and implement sleepAsync#3
Conversation
634c8dd to
85b1c3d
Compare
JAVA-6240
JAVA-6240
| return asyncFunctionSuccessfulResult.get().getNullable(); | ||
| } | ||
|
|
||
| private void sleep(final Duration duration) { |
| 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 -> { |
There was a problem hiding this comment.
VAKOTODO thenRunWhileLoop ends if the body fails. Rewrite taking this into account.
| return thenRun(callback -> { | ||
| new RetryingAsyncCallbackSupplier<Void>( | ||
| // `AsyncClientExecutor` is not needed, given the contract of `SimpleRetryPolicy`, `RetryingAsyncCallbackSupplier` | ||
| AsyncClientExecutor.unimplemented(), |
There was a problem hiding this comment.
VAKOTODO Replace AsyncClientExecutor.unimplemented with AsyncClientExecutor.NO_OP.
| @Override | ||
| protected void afterExecute(final Runnable r, @Nullable final Throwable t) { | ||
| super.afterExecute(r, t); | ||
| assertTrue(r instanceof Future<?>); |
There was a problem hiding this comment.
VAKOTODO Leave a comment explaining this assertion.
| 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.
| 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.
| * 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.
| } | ||
|
|
||
| @Nullable | ||
| private static Throwable getException(final Runnable r, @Nullable final Throwable t) { |
There was a problem hiding this comment.
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.
| * | ||
| * @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().
| 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); |
There was a problem hiding this comment.
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.
1 participant
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