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

Add sys property to control how to cleanup context's ThreadLocal (Fixes #424) #425

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

franz1981
Copy link

@franz1981 franz1981 commented Jun 20, 2023

As explained in #424 this is added to control how to cleanup the context, but:

  • I have no idea if the initial ThreadLocal have some initialization lambdas: if yes, then set(null) and remove would behave differently because a subsequent get in the former case would return null while in the latter will cause the init lambda to be called again
  • what about clearedValue? I should set to that value instead of null?

@franz1981 franz1981 marked this pull request as draft June 20, 2023 09:43
@franz1981
Copy link
Author

Sanne/quarkus@75e2d7e is a PoC branch that show how that can be used

@geoand

@geoand
Copy link
Contributor

geoand commented Jun 20, 2023

I think it makes sense.

@FroMage
Copy link
Contributor

FroMage commented Jun 20, 2023

This is only for the TL we use to override settings locally. I don't think most people use this API. If you want to clear the TL for libs (CDI, Security, Transaction) you need to do it in the respective ThreadLocalProvider implementations. Of which there are many, honestly I've lost track.

@geoand
Copy link
Contributor

geoand commented Jun 20, 2023

Yeah, we'll have to do the same in the providers we use in Quarkus

@geoand
Copy link
Contributor

geoand commented Jun 20, 2023

I am also wondering whether we should have a way to have Quarkus pass this to CP when it bootstraps it

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