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

Caching the ThreadContext breaks in multi-deployment environments #335

Closed
wants to merge 1 commit into from

Conversation

kabir
Copy link
Contributor

@kabir kabir commented Oct 20, 2020

My use-case is that I first deploy application A in WildFly, then undeploy application A, and then deploy application B.

Before this fix application B will still use the ThreadContext and underlying ContextManager from application A. This breaks the optimisation @stuartwdouglas added to cache the TransactionManager in the JtaThreadContextProvider as it tries to read data from the bean manager from deployment A which has been cleared. Also, I believe we cannot cache in case deployments supply their own ThreadContext implementations.

Creating the ThreadContext on demand as I am doing here fixes the problem.

@codecov
Copy link

codecov bot commented Oct 20, 2020

Codecov Report

Merging #335 into master will increase coverage by 0.17%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #335      +/-   ##
============================================
+ Coverage     89.62%   89.80%   +0.17%     
- Complexity     2636     2642       +6     
============================================
  Files           354      354              
  Lines         10150    10161      +11     
  Branches       1250     1252       +2     
============================================
+ Hits           9097     9125      +28     
+ Misses          545      537       -8     
+ Partials        508      499       -9     
Impacted Files Coverage Δ Complexity Δ
...ny/context/ContextPropagationMultiInterceptor.java 100.00% <100.00%> (ø) 4.00 <3.00> (ø)
...tiny/context/ContextPropagationUniInterceptor.java 86.66% <100.00%> (ø) 4.00 <3.00> (ø)
.../io/smallrye/mutiny/helpers/queues/DrainUtils.java 82.22% <0.00%> (-4.45%) 13.00% <0.00%> (-1.00%)
...io/smallrye/mutiny/converters/uni/ToPublisher.java 87.09% <0.00%> (-3.23%) 3.00% <0.00%> (ø%)
...subscription/SwitchableSubscriptionSubscriber.java 95.08% <0.00%> (-2.46%) 45.00% <0.00%> (-1.00%)
...a/io/smallrye/mutiny/converters/multi/ToMaybe.java 100.00% <0.00%> (ø) 4.00% <0.00%> (ø%)
...java/io/smallrye/mutiny/helpers/Subscriptions.java 79.55% <0.00%> (ø) 48.00% <0.00%> (ø%)
...rye/mutiny/operators/multi/MultiRepeatUntilOp.java 100.00% <0.00%> (ø) 5.00% <0.00%> (ø%)
...e/mutiny/streams/stages/FindFirstStageFactory.java 100.00% <0.00%> (ø) 5.00% <0.00%> (ø%)
...mutiny/operators/multi/builders/ResourceMulti.java 87.00% <0.00%> (+0.13%) 5.00% <0.00%> (ø%)
... and 14 more

@cescoffier
Copy link
Contributor

I discussed with @kabir offline, and I'm worried about the cost of such a change. We are discussing alternatives:

  • specific interceptors for EAP
  • adding a way to clear the static on termination

@cescoffier
Copy link
Contributor

Closing this PR for now, we may reopen it if the alternatives do not work.

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.

2 participants