Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generalize ThreadLocal to ThreadScope #442

Closed
wants to merge 3 commits into from

Conversation

mariofusco
Copy link

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

Copy link
Contributor

@FroMage FroMage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

mariofusco and others added 2 commits November 8, 2023 18:34
@mariofusco
Copy link
Author

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
Copy link
Contributor

geoand commented Nov 9, 2023

Also cc @manovotn

@FroMage
Copy link
Contributor

FroMage commented Nov 9, 2023

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.

@mariofusco mariofusco closed this Feb 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants