Adding context to existing CS/CF instances

[FroMage]

ATM we have two APIs for adding context to CompletionStage and CompletableFuture:

• ManagedExecutor: you can create contextualised CompletableFuture instances that are completed or incomplete, but you can't wrap existing instances.
• ThreadContext: you can only wrap contextualised CompletableFuture and CompletionStage instances, but those can't have an Executor defined, and will throw when calling Async methods.

While working around a bug, I found myself having to call the Async methods of a CS I had wrapped using ThreadContext, and it threw at me.

Then I found myself having to switch to using ManagedExecutor to wrap my CS stuff, but although I could replace some calls to tc.withContextPropagation(CompletableFuture.completedValue(...)) I found I could not wrap existing CS instances with that API. The bext thing I found was to use:

managedExecutor.completedFuture(null).thenCompose(v -> pool.begin())

Which is not great, but also hard to guess to people not familiar with CS and composition. I couldn't use existing ManagedExecutor methods to wrap a CS I did not create myself.

This leads me to believe there's something missing in the spec.

Now, additionally, for SmallRye-CP we had to add a method to our impl of ContextManager.Builder:

        public Builder withDefaultExecutorService(ExecutorService executorService) {
            this.defaultExecutorService = executorService;
            return this;
        }

To be able to specify a default executor service for all built ManagedExecutor (unless overridden by a similar method in our impl of ManagedExecutor.Builder).

Which leads me to think that for deployments where a (single) default executor service can be specified, it shouldn't be an issue to call Async methods of contextualised CS/CF by ThreadContext. But the current API forbids this.

Let me know how it's implemented in OpenLiberty, but if you have the same builder extensions to be able to specify a default executor service, we could add those to the spec to the three builders (ContextManager, ThreadContext and ManagedExecutor) and soften the API for wrapping CS/CF from ThreadContext wrt having a default executor?

I'd also favour add a method to be able to wrap existing CS/CF to ManagedExecutor and adding variants in ThreadContext to be able to specify an ExecutorService when wrapping CS/CF.

This way, no matter which of the two APIs you're using you should be able to wrap existing CS/CF and even override the ExecutorService if it turns out your platform has no default.

WDYT?

I can work out a PR if you agree with the general direction. Or even do a PR just to help make a decision and discuss.

[njr-11]

The possibility of an equivalent on ManagedExecutor for withContextCapture was something that came to mind while working on v1.0, but I had put the thought on hold to finish off the initial release, recognizing that thenCompose does provide a working (albeit ugly) solution as a stopgap measure.

I'd rather not see a ManagedExecutor supplied to a ThreadContext.Builder or new variants of the withContextCapture(stage, executor) methods to accept an executor parameter. In both cases, that creates confusion with conflicting context propagation settings (the managed executor's vs the thread context's).

Instead, I think the right approach here is to create a parallel interface on ManagedExecutor for the Java 11 CompletableFuture.copy method, which directly matches the behavior one would expect out of a withContextCapture equivalent (copy is what we modeled withContextCapture after). The spec already employs this approach for other methods on ManagedExecutor: managedExecutor.newIncompleteFuture(), managedExecutor.runAsync(action), managedExecutor.supplyAsync(action), and so on, so it is very natural to have,

stage2 = managedExecutor.copy(stage1);

Here's a draft of what the JavaDoc might look like, copying directly from withContextCapture,

/**
 * <p>Returns a new <code>CompletableFuture</code> that is completed by the completion of the
 * specified stage.</p>
 *
 * <p>The new completable future is backed by the ManagedExecutor upon which copy is invoked,
 * which serves as the default asynchronous execution facility
 * for the new stage and all dependent stages created from it, and so forth.</p>
 *
 * <p>When dependent stages are created from the new completable future, thread context is captured
 * and/or cleared by the ManagedExecutor. This guarantees that the action
 * performed by each stage always runs under the thread context of the code that creates the stage,
 * unless the user explicitly overrides by supplying a pre-contextualized action.</p>
 *
 * <p>Invocation of this method does not impact thread context propagation for the supplied
 * completable future or any dependent stages created from it, other than the new dependent
 * completable future that is created by this method.</p>
 *
 * @param <T> completable future result type.
 * @param stage a completable future whose completion triggers completion of the new completable
 *        future that is created by this method.
 * @return the new completable future.
 */
<T> CompletableFuture<T> copy(CompletableFuture<T> stage);

The above addresses the first half of the issue, and since the comment is already getting a bit long, I'm going to stop here and post a second comment for the second half discussing configuring/using default executor services.

[njr-11]

Regarding the second set of questions, OpenLiberty doesn't implement a ContextManager.Builder (it's optional and we didn't need one), however, we do have the concept of a default executor service. This default executor service/thread pool isn't something that a user ever directly interfaces with and so they wouldn't be able to supply it to a spec method. Instead, this default executor service/thread pool is what backs all managed executors and many other async operations as well. For environments like this (with a built-in default executor/thread pool, or for an executor that is specified with the new ContextManager.Builder.withDefaultExecutorService method), it seems natural to lift the withContextCapture restriction against async stages and be able to utilize that executor for async operations.

Specifically regarding the proposal, that means I'm fine with the addition of:
ContextManager.Builder.withDefaultExecutorService(executor)

With respect to having withDefaultExecutorService on ThreadContext.Builder, it seems out of place. ThreadContext ought to only concern thread context and only be used for thread context, so I don't think we should add withDefaultExecutorService there, but hopefully my proposal in the previous comment to add managedExecutor.copy will cover all of the scenarios where that would have been needed.

With regard to ManagedExecutor.Builder.withDefaultExecutorService(executor), I'm undecided. I can see where this might provide additional flexibility in some cases, but I can also foresee ambiguity and potential collisions between the supplied executor's behavior and other builder config like the maxQueued/maxAsync settings. With that in mind, a different option could be threadContext.createManagedExecutor(executor)? Or, maybe threadContext.withContextCapture(stage, executor) (mentioned earlier) isn't so bad if clearly documented that the thread context comes from the invoking threadContext instance, not the supplied executor. I'm not sure what is best here, and will need to give this some more thought.

[FroMage]

I'd rather not see a ManagedExecutor supplied to a ThreadContext.Builder or new variants of the withContextCapture(stage, executor) methods to accept an executor parameter. In both cases, that creates confusion with conflicting context propagation settings (the managed executor's vs the thread context's).

I was suggesting an ExecutorService. It would be clearly indicated that even if you pass a ManagedExecutor, its propagation settings would be ignored in favour of the ThreadContext.

ManagedExecutor.copy works for me, except that it should have a variant for CompletionStage because most of our APIs don't deal with CompletableFuture.

[FroMage]

Specifically regarding the proposal, that means I'm fine with the addition of:
ContextManager.Builder.withDefaultExecutorService(executor)

Cool.

With respect to having withDefaultExecutorService on ThreadContext.Builder, it seems out of place. ThreadContext ought to only concern thread context and only be used for thread context, so I don't think we should add withDefaultExecutorService there, but hopefully my proposal in the previous comment to add managedExecutor.copy will cover all of the scenarios where that would have been needed.

They would indeed, but I thought it would make it more regular, and allow people to specify an ExecutorService that is not the default one, which can be handy because the CS/CF API doesn't let you specify one otherwise. I don't have a use-case for this, so I'm not going to fight over it and we can always add it later if a use-case appears. But I don't really see where it would hurt.

With regard to ManagedExecutor.Builder.withDefaultExecutorService(executor), I'm undecided. I can see where this might provide additional flexibility in some cases, but I can also foresee ambiguity and potential collisions between the supplied executor's behavior and other builder config like the maxQueued/maxAsync settings

Well, there's always going to be a bit of a question when it comes to that, especially because we can't query the ExecutorService's maxQueued/maxAsync settings to check if they are coherent.

But if you view the ManagedExecutor as a "view" over another ExecutorService you can easily defend that having a lower maxQueued/maxAsync setting in the view than in the backing ExecutorService restricts the view's window (which is fine), and having a larger setting in the view means we will never reach the max, but that's also acceptable: if you wanted to attain the max you could have given a different backing executor, or none at all and it would create one of the right size.

With that in mind, a different option could be threadContext.createManagedExecutor(executor)?

That would be easy to implement for us. But then I feel we could solve a bunch of issues by adding ManagedExecutor.getThreadContext() as well, to get additional operations :)

maybe threadContext.withContextCapture(stage, executor) (mentioned earlier) isn't so bad if clearly documented that the thread context comes from the invoking threadContext instance, not the supplied executor. I'm not sure what is best here, and will need to give this some more thought.

That is indeed the behaviour I expect. If that seems acceptable, then ThreadContext.Builder.withDefaultExecutorService(ExecutorService) can't be so unacceptable, then, because it provides a default for that parameter.

[njr-11]

ManagedExecutor.copy works for me, except that it should have a variant for CompletionStage because most of our APIs don't deal with CompletableFuture.

Agreed. I would expect to have signatures for both CompletionStage and CompletableFuture, just like withContextCapture has.

With that in mind, a different option could be threadContext.createManagedExecutor(executor)?

That would be easy to implement for us. But then I feel we could solve a bunch of issues by adding ManagedExecutor.getThreadContext() as well, to get additional operations :)

If you want to have a ManagedExecutor.getThreadContext(), I would be fine with it. This seems consistent because managed executor configuration clearly includes thread context settings and propagates thread context as well, so it makes sense that you would be able to obtain the ThreadContext instance which it uses (I think we basically implemented it this way as well, so there is no cost to us here to do this).

It seems like we have a lot of agreement already on many of the proposed interfaces.
I'm going to try to summarize those here. Do these cover all of the scenarios you had in mind?

new ContextManager.Builder methods:
Builder withDefaultExecutorService(ExecutorService executorService)

new ThreadContext methods:
ManagedExecutor createManagedExecutor(Executor executor);

updated ThreadContext method JavaDoc (to allow async operations using built-in or configured executor)
<T> CompletableFuture<T> withContextCapture(CompletableFuture<T> stage)
<T> CompletionStage<T> withContextCapture(CompletionStage<T> stage)

new ManagedExecutor methods:
<T> CompletableFuture<T> copy(CompletableFuture<T> stage);
<T> CompletionStage<T> copy(CompletionStage<T> stage);
ThreadContext getThreadContext();

Having just written the above right next to each other, I can see that we need to decide between java.util.concurrent.Executor versus java.util.concurrent.ExecutorService as the parameter type for the first two methods. It doesn't make any difference to me - just make them consistent. If you don't have any other way of deciding, we could just go with Executor because it looks like that is what the CompletableFuture API uses here.

[FroMage]

Ah, good question. Thing is, ManagedExecutor really forwards calls to an ExecutorService so providing it with an Executor is a lot less useful.

But you're right that CS/CF use Executor, which is easier to implement. However, if both our implementations eventually delegate to an ExecutorService I'd say let's stick with this type at least for ContextManager.Builder.withDefaultExecutorService and ThreadContext.createManagedExecutor.

Given that this is on the builder for ContextManager which serves both for ManagedExecutor and ThreadContext I think it makes sense to make it ExecutorService because this is a platform API and platforms are more likely to delegate to that type than any hand-made Executor.

If we add ThreadContext.withContextCapture(CF, Executor) we can use Executor for CS/CF coherence. I've checked and our implementation only needs an Executor, so it's possible yours too.

[njr-11]

Our global thread pool is an ExecutorService, but our implementation of ManagedExecutor is written such that Executor could alternatively be used if one was supplied, so we are compatible with either way.
I thought of one other reason why a user might be interested in supplying Executor rather ExecutorService, which is to prevent anyone with access to completion stages from attempting the shutdown operation. The following would not be possible with just Executor,

((ExecutorService) completableFuture.defaultExecutor()).shutdown();

This was never a concern when the default executor was a ManagedExecutor owned by the application, because the application is then just shutting down its own instance. However, when backed by our global thread pool as the default executor, we will certainly need to include a layer in between here, because shutting that down would be catastrophic and not something that applications ought to be able to do.

[FroMage]

In our case, we pass an ExecutorService which throws on shutdown methods. I think that's enough to avoid the issue.

As for ManagedExecutor being able to work when backed by a simple Executor, that must complexify the implementation, unless there's a simple implementation in the JDK that I'm unaware of…

[FroMage]

So, in smallrye/smallrye-context-propagation#183 I've made an implementation of this, with the methods you mentioned, minus one:

• new ContextManager.Builder methods:

  • Builder withDefaultExecutorService(ExecutorService executorService)

• updated ThreadContext method JavaDoc (to allow async operations using built-in or configured executor)

  • CompletableFuture withContextCapture(CompletableFuture stage)
  • CompletionStage withContextCapture(CompletionStage stage)

• new ManagedExecutor methods:

  • CompletableFuture copy(CompletableFuture stage);
  • CompletionStage copy(CompletionStage stage);
  • ThreadContext getThreadContext();

I did not add this one because I'm not sure it brings value, I think we can wait to see if it's useful:

• new ThreadContext methods:
  • ManagedExecutor createManagedExecutor(Executor executor);

I had to add the notion that ThreadContext has a "default executor" which comes from either the ContextManager (as specified in its builder), or from the ManagedExecutor on which ManagedExecutor.getThreadContextwas called.

So I rephrased the javadoc for ThreadContext.withContextCapture(CompletableFuture) and ThreadContext.withContextCapture(CompletionStage) to mention that this default executor would be used for the Async methods which aren't passed an explicit Executor, and that they will still throw if there is no default executor.

I also mention in ManagedExecutor.getThreadContext that the resulting thread context uses this executor as default executor. Actually in the implementation they use the managed executor's forwarding ExecutorService because our ManagedExecutor is shallow.

In practice, ThreadContext.withContextCapture and ManagedExecutor.copy do the same thing now, when there is a default executor.

I did notice a weird detail wrt the unchanged setting in that only ThreadContext has such a setting, while ManagedExecutor does not. Which means that the ThreadContext returned by ManagedExecutor.getThreadContext is less configurable than those that don't belong to managed executors.

I remember a discussion about this discrepency a long time ago, but I don't remember why they differed.

[FroMage]

If we add ThreadContext.withContextCapture(CF, Executor)

Hah, what's funny is that we already have this method as public in SmallRye for a long time.

[njr-11]

That's great to hear you have it prototyped!

I did not add this one because I'm not sure it brings value, I think we can wait to see if it's useful:

* new ThreadContext methods:
  
  * ManagedExecutor createManagedExecutor(Executor executor);

That's fine with me. I'm not aware of any request for it either, so it's best not to add it.

I did notice a weird detail wrt the unchanged setting in that only ThreadContext has such a setting, while ManagedExecutor does not. Which means that the ThreadContext returned by ManagedExecutor.getThreadContext is less configurable than those that don't belong to managed executors.

I remember a discussion about this discrepency a long time ago, but I don't remember why they differed.

ManagedExecutor lacks configuration for unchanged contexts because until now there weren't any scenarios with it where running inline was guaranteed, so as to produce a deterministic result when leaving a context type unchanged. ThreadContext, however, offers the ability to contextualize various operations that will always run inline, where it might be a useful behavior to in certain cases run with context that is already on the thread. For example, if you have a pre-contextualized Runnable that left transaction context unchanged, then you could write code that runs it inline under an execution transaction.

tran.begin();
... do transactional stuff
contextualRunnable.run(); // runs under the transaction because transaction context was configured to unchanged.
tran.commit();

Now that you can create a ThreadContext from a ManagedExecutor, I think it would make sense for us to now add the unchanged configuration to ManagedExecutor as well.

Is there a pull for these spec updates anywhere? For some reason I was thinking we had created one, but maybe that was just me thinking of the JavaDoc/method signature examples that we've been posting into this issue.

[FroMage]

I don't think there's a PR yet.

[njr-11]

I don't think there's a PR yet.

Are you planning to create a PR (or already working on one)? Or would you rather have me create it? I'm fine either way, just looking to avoid duplication. It should be easy enough given that we have a bunch of draft method signatures and even some draft JavaDoc for it in the discussion above.

[FroMage]

I can make a PR.

[FroMage]

It's at #196.

[FroMage]

And TCK at #197

[gorshkov-leonid]

@FroMage I can't find ManagedExecutor.allOf . May be it is unnecessary or it is a gap. Could you please give some comments?

[njr-11]

@FroMage I can't find ManagedExecutor.allOf . May be it is unnecessary or it is a gap. Could you please give some comments?

We didn't add managedExecutor.allOf because it seemed unnecessary, just a shortcut for doing:

stage = managedExecutor.copy(CompletableFuture.allOf(...));

[gorshkov-leonid]

@njr-11 thank you. But actually I use quarkus and it seems it does not provide microprofile-context-propagation-api 1.1 by some reason. But as I understand that managedExecutor.copy is the same thing as threadContext.withContextCapture. That is enough.
links:
quarkusio/quarkus#16059
quarkusio/quarkus#17486

[gorshkov-leonid]

@njr-11 I counted how often allOf is used in my case - 25 times. I think it is not few. What do you think of adding allOf to make API more symmetric?

[njr-11]

@njr-11 I counted how often allOf is used in my case - 25 times. I think it is not few. What do you think of adding allOf to make API more symmetric?

It would be fine with me to add it if it makes the programming model easier to use for applications. I expect the ManagedExecutor variant of allOf wouldn't be static like CompletableFuture.allOf is, because the resulting completion stage ought to be backed by a specific ManagedExecutor for the sake of any dependent stages that are added on to it. Go ahead and open an issue for it (this current issue where we are discussing this is already closed out) and we can discuss it on the MicroProfile Context Propagation call on June 10 as a possible enhancement for the next release of the spec.