raise IllegalArgumentException on attempts to contexualize an already-contextualized action

[njr-11]

pull fixes #115

Signed-off-by: Nathan Rauh nathan.rauh@us.ibm.com

[njr-11]

@FroMage or @manovotn could you review this? It updates the JavaDoc to state that IllegalArgumentException is raised if already contextualized. As we discussed on the call, this does not require that we detect contextualRunnable( () -> alreadyContextualizedRunnable.run() ) because that is not an attempt to contextualize an already-contextualized Runnable, but is instead an attempt to contextualize a new Runnable that happens to use another contextualized Runnable within its implementation.

[FroMage]

What about already-contextualised CompletionStage and CompletableFuture?

[FroMage]

Also, what about the behaviour of contextual CF and CS when given pre-contextualised lambdas? And what about the behaviour of TC.currentContextExecutor()'s returned Executor when passed pre-contextualised runnables?

I should note that I think lambdas are much cheaper to use in the JVM than the classes this forces us to write to mark pre-contextualised actions, so we may end up paying a perf cost to detect this.

[FroMage]

I see you already decided to not recontextualise actions passed to managed executors and CF/CS. You even have tests for this, except they're not testing what they say they test, since they pass for our implementation which does the opposite. Indeed both the TC and ME have the exact same settings:

    /**
     * When an already-contextualized Consumer or BiFunction is specified as the action/task,
     * the action/task runs with its already-captured context rather than
     * capturing and applying context per the configuration of the managed executor.
     */
    @Test
    public void contextOfContextualConsumerAndBiFunctionOverrideContextOfManagedExecutor()
            throws ExecutionException, InterruptedException, TimeoutException {
        ThreadContext labelContext = ThreadContext.builder()
                .propagated(Label.CONTEXT_NAME)
                .cleared(ThreadContext.ALL_REMAINING)
                .build();

        ManagedExecutor executor = ManagedExecutor.builder()
                .propagated(Buffer.CONTEXT_NAME)
                .cleared(ThreadContext.ALL_REMAINING)
                .build();

[FroMage]

they pass for our implementation which does the opposite

Actually no, our implementation does not do the opposite at all, sorry. It does the same thing, by virtue of not being able to recontextualise pre-contextualised actions. Sorry for the confusion: the test is right.

[njr-11]

What about already-contextualised CompletionStage and CompletableFuture?

I believe the withContextCapture documentation already addresses this. The context propagation settings of the ThreadContext instance apply when to all dependent stages created from the new stage. Note that the new stage itself does not actually have context, in that it is simply completed with the result of the stage upon which withContextCapture is invoked.

When dependent stages are created from the new completable future, thread context is captured from the thread that creates the dependent stage and is applied to the thread that runs the and/or cleared as described in the documentation of the {@link ManagedExecutor} class, except that action, being removed afterward. When dependent stages are created from these dependent stages, this ThreadContext instance takes the place of the default asynchronous execution facility in and likewise from any dependent stages created from those, and so on, thread context is captured supplying the configuration of cleared/propagated context types. This guarantees that the action from the respective thread that creates each dependent stage. This guarantees that the action performed by each stage always runs under the thread context of the code that creates the stage, 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.

I could certainly see adding a TCK test or two for a scenario like this, which can be done under the TCK issue.

Also, what about the behaviour of contextual CF and CS when given pre-contextualised lambdas?

This is a good question. We covered it under issue #110 / pull #111 with the language, If the supplied action is already contextual (for example, threadContext.contextualFunction(function1)), then the action runs with the already-captured context. and tested it in the tck under pull #118 which includes the test case that you later found as an example of this.

And what about the behaviour of TC.currentContextExecutor()'s returned Executor when passed pre-contextualised runnables?

Good catch, I missed that in my updates. I'll add a commit updating this scenario to raise IllegalArgumentException here as well.

[njr-11]

@FroMage / @manovotn I've fixed the code review issue that you found with contextual executor executing a precontextualized Runnable under this additional commit. Please review again, https://github.com/eclipse/microprofile-concurrency/pull/119/commits/e88c2dedb94578d8a7f72bc75a870915d86da50b

[FroMage]

OK thanks, LGTM.