Implement OIDC auth for async

[katcharov]

JAVA-4981

This is based on #1107, so it is subject to rebase once that is merged. The commit that implements this is "Implement OIDC auth for async" (and any ensuing).

[stIncMale]

Partial review: I looked only at the smaller part of the changes.

[stIncMale]

This piece of review results contains two sets of suggestions for the names of the new methods. One is intended to use the same naming scheme used by CompletionStage in the Java SE API, the other one tries to make the original PR naming scheme more internally consistent and at least not conflicting with the Java SE API. Below is the summary of both, as well as links to commits implementing them, to facilitate looking at the code.

This one tries to use the same scheme used in CompletionStage

stIncMale@b54639f

PR API	similar CompletionStage API	suggested PR API
AsyncRunnable.complete(SingleResultCallback)	N/A	AsyncRunnable.finish
AsyncSupplier.complete(SingleResultCallback)	N/A	AsyncSupplier.finish
AsyncRunnable.complete(Runnable, SingleResultCallback)	thenRun + N/A	AsyncRunnable.thenRunAndFinish
AsyncRunnable.completeAlways	whenComplete + N/A	AsyncRunnable.whenCompleteRunAndFinish
AsyncRunnable.run	thenCompose	AsyncRunnable.thenComposeRunnable
AsyncRunnable.supply	thenCompose	AsyncRunnable.thenComposeSupplier
AsyncRunnable.onErrorIf	exceptionallyCompose + N/A condition	AsyncRunnable.exceptionallyComposeRunnableIf
AsyncSupplier.onErrorIf	exceptionallyCompose + N/A condition	AsyncSupplier.exceptionallyComposeSupplierIf
AsyncRunnable.runRetryingWhen	thenCompose + N/A retrying logic	AsyncRunnable.thenComposeRunnableRetryingWhile

This one tries to make the original PR scheme more internally consistent and at least not conflicting with the Java SE API

stIncMale@6a12aed

PR API	suggested PR API
AsyncRunnable.complete(SingleResultCallback)	same
AsyncSupplier.complete(SingleResultCallback)	same
AsyncRunnable.complete(Runnable, SingleResultCallback)	AsyncRunnable.thenRunAndComplete
AsyncRunnable.completeAlways	AsyncRunnable.thenRunAlwaysAndComplete
AsyncRunnable.run	AsyncRunnable.thenRun
AsyncRunnable.supply	AsyncRunnable.thenSupply
AsyncRunnable.onErrorIf	AsyncRunnable.onErrorRunIf
AsyncSupplier.onErrorIf	AsyncSupplier.onErrorSupplyIf
AsyncRunnable.runRetryingWhen	AsyncRunnable.thenRunRetryingWhile

[stIncMale]

I have completed the review barring javadocs, which I will review when the API settles.

[stIncMale]

I have completed the review barring javadocs, which I will review when the API settles.

[stIncMale]

All concerns are optional.

[stIncMale]

[optional]

1. If t is Error, it makes sense to propagate it as is, but if it's an Exception, then we have a bug in the driver¹ and it would be better to wrap t in AssertionError to make that clear.
2. If the invocation of onResult on line 70 completes abruptly with an Exception, then, similarly to the first item, there is a bug in the driver and wrapping it in AssertionError makes that clear.

I thought we were in agreement about this. But in case we are not, this is an optional remark.

---

¹ Currently, the driver does not have a tendency to propagate exceptions about invalid user-provided arguments via Publishers, which is surprising, but users probably don't care because they don't usually pass invalid arguments in production. Anyway, there is no user-facing callback-based code, which means that we don't expect there to be checks of user-provided arguments in callback-based methods.

[katcharov]

I comment above, where I think you refer to your comment here. I had said:

Unaffected async code SHOULD be preserved, including for example null checks, which throw exceptions, so chained lambdas MAY throw exceptions, which are expected to be handled in exactly one place (the finish method, when it is invoked at the end of the chain).

To put it another way, if we do (1), "behavesSame" tests in AsyncFunctionsTest will start to fail, and if we do (2), testInvalid fails.

It seems to me that throwing exceptions is a "bug" in async code only because those exceptions can be lost. But, if the API handles them, there is no issue, and no bug. The API can either handle and allow them, or treat them as bugs (and throw assertion errors). Both are acceptable approaches. However, treating them as bugs would mean that the user will need to create boilerplate or special constructs to asynchronously handle exceptions, which is inconvenient and error-prone, so the first alternative should be preferred.

[stIncMale]

Unaffected async code SHOULD be preserved, including for example null checks, which throw exceptions, so chained lambdas MAY throw exceptions

I can't find a justification for this statement on my own: why should the behavior of incorrect code be preserved? Internal null-checks should produce AssertionError rather than NullPointerException (the latter being an Exception is an utter nonsense: NPE is always a software bug, not a recoverable exception), but even if they produce NullPointerException, that still means nothing but "the driver code is broken".

It seems to me that throwing exceptions is a "bug" in async code only because those exceptions can be lost.

If this were true, then we could just always throw all exceptions normally in async code, and make sure we log them at some point, e.g., by setting a logging UncaughtExceptionHandler for threads.

The reason the contracts of both AsyncFunction.unsafeFinish and SingleResultCallback.onResult must not allow these methods to complete abruptly with an Exception is that it violates the very basic code behavior, making programming virtually impossible: AsyncFunction.unsafeFinish/SingleResultCallback.onResult completing abruptly is equivalent to stopping at some point executing any instructions in synchronous code that follow that point in program order, including final blocks and returns from methods (whether normal or abrupt ones)¹.

To put it another way, if we do (1), "behavesSame" tests in AsyncFunctionsTest will start to fail, and if we do (2), testInvalid fails.

Executions where AsyncFunction.unsafeFinish/SingleResultCallback.onResult comple abruptly can't generally be compared with synchronous code because the behavior I described above is simply impossible in synchronous Java code. I.e., the tests comparing behavior of sync and async code are not appropriate here.

if the API handles them, there is no issue, and no bug

The way it is currently done is not a panacea:

1. If AsyncSupplier.finish can't pass t to callback.onResult because callback.onResult has already been called, then t is neither handled nor can be.
2. If AsyncSupplier.finish passes t to callback.onResult, then it is very much possible that this t was actually meant for a different callback, i.e., this may be equivalent to an exception being thrown in synchronous code out of the blue.

Neither of the above situations is fine, they are both software bugs. #1 is obvious, #2 may be less obvious, so I am providing an example:

1. Consider DefaultConnectionPool.getAsync being called as

beginAsync()
    .thenSupply(c -> pool.getAsync(operationContext, c))
    .finish(callbackForGetAsync)

2. Imagine that here

   mongo-java-driver/driver-core/src/main/com/mongodb/internal/connection/DefaultConnectionPool.java

   Lines 220 to 225 in 4ab6048

   	try {
   	stateAndGeneration.throwIfClosedOrPaused();
   	} catch (Exception e) {
   	eventSendingCallback.onResult(null, e);
   	return;
   	}

   the code incorrectly throws e instead of calling eventSendingCallback.onResult(null, e).
3. finish catches e and passes it to callbackForGetAsync, but e was meant to be handled by eventSendingCallback (which has never been called), not callbackForGetAsync.
4. Since eventSendingCallback.onResult is not called, the checkOutFailed method is not called, the driver fails to emit the corresponding event and violates the CMAP spec.
5. As you can see, finish did not make things right despite passing e to callbackForGetAsync.

---

¹ It's also possible for AsyncFunction.unsafeFinish/SingleResultCallback.onResult to complete abruptly after calling the callback (a wrapped callback in case of onResult if there is one), in which case coming up with a synchronous analogy appears to be impossible.

[katcharov]

Discussed a bit, leaving as is.

The two cases you mention are programming errors, present independently of the API. There is an opportunity to wrap exceptions in assertion errors, to better-highlight driver bugs. This would imply that any "expected" ("good") exceptions must be accurately discovered and explicitly pushed into callbacks - that is, async code would need to be "marked up" with the locations of good exceptions, and all others treated as "bad". This would complicate the async code, and complication increases the likelihood of bugs (introducing such boilerplate makes it harder to compare sync and async, which is the primary goal). It is also imperfect: arguably, indiscriminately turning what would otherwise be an "expected" exception into an assertion error is worse than indiscriminately propagating without wrapping.

The alternative taken here is to effectively wrap async code sections in a "try catch", and propagate all exceptions into the callback "as they are". This implies that it is no longer a coding error to throw "expected" exceptions from async code that is used in this API.

[stIncMale]

This would imply that any "expected" ("good") exceptions must be accurately discovered and explicitly pushed into callbacks

Any exceptions thrown by a callback-based method or SingleResultCallback.onResult is not expected and signifies a bug.

This implies that it is no longer a coding error to throw "expected" exceptions from async code that is used in this API.

It still is, and as far as I can see, nothing can be done to change that.

[stIncMale]

[optional]

All instance methods in Async* interfaces introduced in this PR have a verb in the name, including AsyncRunnable.thenRunIf, but onErrorSupplyIf was renamed to onErrorIf. Consider returning a verb into this method's name.

[katcharov]

Because we are removing code duplication using inheritance, async runnables are void suppliers (or, void-void functions). However, our naming uses run/supply, so for consistency, this method should be called onErrorRunOrSupplyIf, but this is confusing.

I think that the verb here is something like "handle" X, and "onX" is a conventional way to name such methods, which is at least less confusing than the above.

[stIncMale]

async runnables are void suppliers

This is irrelevant, as the onErrorIf method takes AsyncSupplier, i.e., it's onErrorSupplyIf.

P.S. This thread stays optional.

[katcharov]

Leaving as-is. onErrorSupplyIf is a confusing name when technically something is being supplied, yet the thing being supplied is "nothing" (void).