search

smallrye / smallrye-context-propagation

SmallRye implementation of MicroProfile Context Propagation
Apache License 2.0
30 stars 24 forks source link

Generalize ThreadLocal to ThreadScope #442

Closed mariofusco closed 6 months ago

mariofusco commented 10 months ago

This pull request is a proposal to solve this issue and also a generalization of the ThreadLocals' usage made inside smallrye allowing to plug different implementations including something more virtual threads friendly.

I understand that replacing ThreadLocal with the new ThreadScope interface that I introduced with this pull request is quite invasive and could have a (relevant?) impact on other projects downstream. Nevertheless I believe that exposing ThreadLocal as we did so far is a leaky abstraction obliging us to use a very specific implementation for a quite general purpose.

In essence this solution has a twofold advantage:

  1. Exposing only the new ThreadScope interface limits the use of a ThreadLocal-like data structure to the only 3 methods that we really need.
  2. Even more important its introduction allows to use different and more efficient implementations that we couldn't use otherwise. For instance netty's FastThreadLocal which doesn't extend ThreadLocal or some other future implementations also playing well with virtual threads.

/cc @FroMage @Sanne @geoand @franz1981

mariofusco commented 10 months ago

Overall, I'm not sure I understand the value of replacing ThreadLocal as a general interface.

You can declare subclasses of ThreadLocal by overriding the three interesting methods, and in practice forward to Netty's TL and any other implementation, without any performance penalty.

Or am I missing something? This appears to be declaring a new interface in place of ThreadLocal, but it's already useful as an interface-like.

This is also doable, but a bit awkward to be honest: if I understand correctly what you're suggesting is subclassing ThreadLocal, but only for type compatibility, without using any data structure of the parent class (also a waste of space I guess) and delegating all the actual implementation to an internal netty FastThreadLocal. And you will have to do this not only for those 3 methods, but for all methods, otherwise if anybody inadvertently uses one of the other method you will end up with an inconsistent behavior.

Personally I don't like this design, but I agree that at the moment it will be less invasive and as I said I don't have a clear idea of how impactful the change that I'm proposing could be.

geoand commented 10 months ago

Also cc @manovotn

FroMage commented 10 months ago

This is also doable, but a bit awkward to be honest: if I understand correctly what you're suggesting is subclassing ThreadLocal, but only for type compatibility, without using any data structure of the parent class (also a waste of space I guess) and delegating all the actual implementation to an internal netty FastThreadLocal. And you will have to do this not only for those 3 methods, but for all methods, otherwise if anybody inadvertently uses one of the other method you will end up with an inconsistent behavior.

Personally I don't like this design, but I agree that at the moment it will be less invasive and as I said I don't have a clear idea of how impactful the change that I'm proposing could be.

This is how it currently works ATM. I'm pretty sure you only have to override the three methods, since there are only three methods. I don't think it wastes any space either because there are no fields in this class, the default methods all store stuff in the thread.