Alternative to #12395.

[sbordet]

Alternative to #12395 The main difference is assuming that the Runnable passed to Content.Source.demand(Runnable) is non-blocking unless explicitly declared so. This leaves most of the application code using lambdas and method references unchanged.

Deprecated usages of CompletableFuture APIs in Content.Source.

[sbordet]

@gregw I think we're on similar conclusions, that some of the utility classes using CompletableFuture exposed the problem.

As for the demand Runnable to be blocking by default, I think that is fine for these reasons:

• It aligns with how we treat Handler.handle() -- blocking. To be explicit, a request may arrive with all content already available, we call handle() which reads asynchronously, never hits a null chunk, and upon the last chunk it performs a blocking JDBC call; similarly, a request may arrive, have no content yet, so handle() would read a null chunk, and register a demand Runnable which when invoked would read all the chunks and upon the last perform the JDBC call. So Handler.handle() and the demand Runnable.run() should be equivalent with respect to their InvocationType.
• Defaulting to non-blocking has the problem that SerializedInvoker.Link's invocation type could not be hard-coded to blocking like it is now. This PR changes it to look up the Runnable invocation type, but it's not right if a subsequent task is blocking.

On the other hand, perhaps most of the demand Runnable implementations are non-blocking, so it would be nice to avoid the extra ceremony to specify that, so assuming non-blocking would be a better default. However, I have no real-world evidence of how many demand Runnable implementations are non-blocking.

In summary, I think there is unrelated goodness in this PR (e.g. fixing some of the missing executeImmediately()), both PRs must pay attention to the fact that the demand Runnable may specify the InvocationType, and propagate it if the demand Runnable is wrapped.

As for use of CompletableFuture, I am for deprecating its usages where not appropriate, but I would avoid introducing custom CompletableFuture to try to overcome the problem: it was a mistake, we deprecate it. Had we used CompletionStage instead of CompletableFuture, we would not have had the problem.

[gregw]

@sbordet we are in agreement with almost everything.... except when you say:

I would avoid introducing custom CompletableFuture to try to overcome the problem: it was a mistake, we deprecate it.

I think we have to fix CF usage in the next releases. At best, we can only deprecate existing usage, so the API will remain for sometime. It is entirely fixable (if somewhat fragile to do so), so we should fix it. If we can deprecate and replace it as well, then great, but we have to fix it.

Currently my extended CF is only used by ContentSourceCompletableFuture, which is then used by FormFields, MultiPartFormData and MultiPartByteRanges. So a single moderately simple fix is addressing 3 of our most key use-cases. We could just do the fix in ContentSourceCompletableFuture rather than in an explicit InvocableCompletableFuture, but I think that exacerbates the fragility of the fix. The issue is, that with a fat interface on CF then the fixed methods will be "lost" if they are done in another class. By making an explicit CF class, we contain the work around we need into one place... even if deprecated and scheduled for ultimate removal. I don't particularly care if the fix adapts dynamically from the andThen methods or if they throw ISE or IAE (see my last comment on the other issue).

OK, I also don't understand when you say:

Had we used CompletionStage instead of CompletableFuture, we would not have had the problem

Our problem is when we have a callback that is doing the adaption from async to blocking and is actually a non-blocking wakeup for an already blocked thread. That is the CF.get() API which we use extensively to map our non-blocking parsers of several content-type to the blocking servlet APIs. So we need the CF.get() methods, which are not provided by CompletionStage. Adding our own get API doesn't help as we will still have the duality of usage of these utility classes: sometimes used asynchronously with andThen and sometimes used blocking with get() - and we need to adjust the invocation type accordingly. Remember, that there is nothing wrong with a blocking demand callback, so long as it is not just a wakeup of a blocked thread.

So how would we fix this is we didn't have the CF interface? We have fully async implementations of forms, and multiparts that can be used either async or blocking. If we gave them only an async Promise based API, then we'd still need to check the InvocationType of that promise, and utility classes that converted the Promise into a blocking API would still need to ensure they were non-blocking. Thus the "fix" would look exactly the same as what the other PR has done for CF, except it would not be on a fat API and thus less fragile. I guess we could also use Invocable.Task in the new API, to make it clear we are checking the invocation type, although that is a bit of a pain for the valid use-cases of passing a real blocking callback. In fact, thinking about it, I'm not 100% convinced that deprecating the CF is worth it (see below).

So, to move forward, can you:

• merge the goodness from this PR into the other one, which can be focused on fixing the APIs that we have.
• review the fix on ContentSourceCompletableFuture (using the extended CF) in the other PR. Comment on which of the variants is your preference (or none).
• open another draft PR in which we can experiment with alternatives to the CF based APIs. Only once we have found something better should we deprecate the CF APIs in that PR.

[gregw]

Actually, now I'm not entirely convinced with the CF fix either. See discussion coming soon in the other PR....

[sbordet]

Closing, as the good changes from this PR have been ported to #12395 in 43232f588f154ab205ae0d74c820a1975de73e8d, and there is agreement that defaulting to blocking behavior is better.