Determine which contexts should propagate by default

[njr-11]

At first glance, propagating all available context seems like a nice, consistent default to have that will be intuitive for users. However, certain context types, namely transaction context, can have some very undesirable consequences that will be inconvenient for users to code around, and, if unaware of it, could lead to data corruption (due to operating under an unexpected transaction) and deadlocks.

It is my own opinion that we should continue the default behavior established by the EE Concurrency spec in by default, establishing a cleared/empty transaction context for dependent stages, within which the user is free to begin and commit/roll back their own transactions if they wish to. This seems to me by far the most likely and desirable pattern in which users will wish to use transactions in combination with CompletableFuture and/or other async tasks. Propagating a transaction across threads seems to me like an advanced pattern that, when the user so desires, it will be natural to explicitly request this behavior (propagate=ThreadContext.TRANSACTION).

The flip side of the argument, if I am representing it correctly, is that because this capability isn't currently possible, we don't know how popular it will become. Users might come to expect the consistence of having all thread context types propagated, including transaction context. And of course, if the user expected it to be propagated and it wasn't, the very same arguments around preventing data corruption apply equally for this case as well.

[njr-11]

@FroMage Here is my idea solve this issue, hopefully addressing both concerns:

Continue to have a ThreadContext.ALL option that propagates all available context. Ensure that it is clearly documented to remind users of the behavior where bringing in a new or updated context provider will change what context gets propagated to threads.

Do not default the propagate config to ALL.

Instead, introduce a default value for propagate of ThreadContext.DEFAULTS. When this is specified, we will ask each available ThreadContextProvider whether it should propagate by default. Add propagatesWhenConfigAbsent method to ThreadContextProvider for this. We can have this be a default method that returns true so that it only needs to be implemented by the few providers (transaction context) that need to change it. The JavaDoc for this method would elaborate that all context providers must aim to provide propagation by default except in the rare cases where there are serious concerns around causing unexpected behavior to users, with transaction context as an example.

This approach will hopefully be a good compromise because it preserves the ability to easily force the ALL available behavior, while enabling a default behavior that will be most expected for users.

This will also solve the issue #24 where we want to reject unavailable context because the DEFAULTS constant will be defined as only including available context types, so users will not automatically see failures, for example, just because a product's CDI or Security feature isn't enabled and therefore has no providers enabled of those types.

Unrelated to this, we can also have the existing ThreadContextProvider.getPrerequisites be a default method so that the majority of providers can avoid implementing it.

[manovotn]

+1 for having ALL as an option and not default.

However, the approach you suggest means that user reading the spec won't really know what's going to be propagated and what won't be since it will basically be up to the implementation. User would need to inspect every implementation to be sure what their default is.
From my perspective it's clearer to have a DEFAULT setup defined in spec and say that it propagates security, CDI,... and does not propagate transaction because....

[njr-11]

@manovotn I'm fine with leaving out the ability for ThreadContextProviders to specify whether they get included in the defaults and just including everything available beside transactions. I coded up the changes under pull #28. Please review. Also copying @FroMage for review because this issue was opened in response to a previous discussion

[FroMage]

Add propagatesWhenConfigAbsent method to ThreadContextProvider for this

Your PR does not have this change, is it normal?

This will also solve the issue #24 where we want to reject unavailable context because the DEFAULTS constant will be defined as only including available context types, so users will not automatically see failures, for example, just because a product's CDI or Security feature isn't enabled and therefore has no providers enabled of those types.

It doesn't exactly solve the issue, which was more general and not just for context types defined in ALL, but it helps solve that particular case. It would however require us to explain which minimum set of context types are required by the spec. My understanding was that before, we had to implement a minimum of three context types. I guess this would remove any minimum context types in implementations, which sounds OK to me.

[FroMage]

I think I like the idea of having providers be able to declare if that context type should be in the default, but if transactions are not in the default (however which way they are not in), I wonder how I can request it explicitly, especially when using the context propagator SPI in #7 to propagate contexts automatically for RxJava, where I don't see any evident way to override the default behaviour. I'll have to think about that.

[manovotn]

This might be a worthy case to discuss on the meeting, not sure I can clearly understand that use case with SPI

[FroMage]

I think we discussed it two meetings back. I can try to re-explain it in #7.

[njr-11]

Add propagatesWhenConfigAbsent method to ThreadContextProvider for this

Your PR does not have this change, is it normal?

@FroMage I left out propagatesWhenConfigAbsent from the pull due to the comment from @manovotn that he would prefer we not include this. Could the two of you sort this out? I'm fine with it either way. Alternately we can discuss on the Friday call if both of you are there.

I think I like the idea of having providers be able to declare if that context type should be in the default, but if transactions are not in the default (however which way they are not in), I wonder how I can request it explicitly...

I think the natural way to indicate that you want all of the defaults plus transaction context to be propagated would be as follows,
executor = new ManagedExecutorBuilder().propagated(ThreadContext.DEFAULTS, ThreadContext.TRANSACTIONS).build();

I'd be happy to add an example like the above to the JavaDoc if we end up going with the approach that includes the ability to let providers indicate whether they are in the default.

[FroMage]

Well yes, but that's only when contexts are explicitly propagated, which is the less ideal usage. I'll refer to the example I just added to #7:

@GET
public Single<Hello> hello(){
 return Single.just("Hello")
  .delay(2, TimeUnit.SECONDS)
  .map(string -> string); // you can use your context here
}

Here, since it's the RxJava hooks (via the propagator SPI) which define that context is captured by RxJava pipelines, how can I possibly override it? The last thing I want is to require users to do manual context propagation to get a behaviour they may expect to be the default.

[njr-11]

Well yes, but that's only when contexts are explicitly propagated, which is the less ideal usage. I'll refer to the example I just added to #7:

@GET
public Single<Hello> hello(){
 return Single.just("Hello")
  .delay(2, TimeUnit.SECONDS)
  .map(string -> string); // you can use your context here
}

Here, since it's the RxJava hooks (via the propagator SPI) which define that context is captured by RxJava pipelines, how can I possibly override it? The last thing I want is to require users to do manual context propagation to get a behaviour they may expect to be the default.

I think this may be getting confused with issue #7. The proposal here is not a replacement for the propagator mechanism. But, the propagator would be able to take advantage of the defaults. For example,

public ContextCapturerSingle(Single.OnSubscribe<T> source) {
    this.source = source;
    threadContext = concurrencyProvider.newThreadContextBuilder()
                    .propagated(ThreadContext.DEFAULTS, ThreadContext.TRANSACTION)
                    .build();
    contextSnapshot = threadContext.withCurrentContext();
}

@Override
public void call(SingleSubscriber<? superT> t) {
    contextSnapshot.execute(() -> source.call(newOnAssemblySingleSubscriber<T>(t, contextSnapshot)));
}
...

[FroMage]

Yes, we can make RxJava propagate ALL as well, but there's also a chance that some users will want to override that at the method level to turn one context on/off:

@WithContextCapture(ThreadContext.ALL) // assuming the default does not include transactions
@GET
public Single<Hello> hello(){
 return Single.just("Hello")
  .delay(2, TimeUnit.SECONDS)
  .map(string -> string); // you can use your context here
}

Or:

@WithoutContextCapture(ThreadContext.TRANSACTION) // assuming the default includes ALL
@GET
public Single<Hello> hello(){
 return Single.just("Hello")
  .delay(2, TimeUnit.SECONDS)
  .map(string -> string); // you can use your context here
}

Or something programmatic:

@GET
public Single<Hello> hello(@Inject ThreadContext context){
 return context.removeContext(Single.just("Hello"), ThreadContext.TRANSACTION)
  .delay(2, TimeUnit.SECONDS)
  .map(string -> string); // you can use your context here
}

This is veering very off-topic, but the point I wanted to make was that whatever default we pick, I strongly suspect users will expect the same default to also be used by context propagators, which means we have even more of an incentive to figure out how to override the defaults for context propagators too. I'll open another issue tomorrow.

[njr-11]

Just in case anyone missed my comment on the pull request, I've added a commit for the rename of ALL_OTHER to ALL_REMAINING as discussed on last week's MP Concurrency call.