Arc - Decide whether req. context is active based on validity of its ContextState #38107
Conversation
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
I agree this fixes the first reproducer, but does not fix the second one (the one with gRPC security event observer, but I don't think it's related to events only).
I can see the request context state invalidated same amount of times as activated when arriving to io.quarkus.grpc.runtime.supports.context.GrpcRequestContextGrpcInterceptor#interceptCall I can see it active. You have better chance to figure why it works this way, could you look please? Separately, this change is good in my eyes, but @mkouba should decide that.
|
Sure, I'll take a look. |
|
CC @franz1981 as this fix means one more |
| @@ -112,7 +112,8 @@ public <T> T get(Contextual<T> contextual) { | |||
|
|
|||
| @Override | |||
| public boolean isActive() { | |||
| return currentContext.get() != null; | |||
| RequestContextState requestContextState = currentContext.get(); | |||
| return requestContextState == null ? false : requestContextState.isValid(); | |||
There was a problem hiding this comment.
It's fine, the only downside is the dependent load, but having a volatile load is pretty cheap on most architecture, and regardless... there is not much to do here, given that we requires correctness first!
|
I've added a test for this change as well and I've reached out to Michal over Zulip to discuss this further. |
|
✔️ The latest workflow run for the pull request has completed successfully. It should be safe to merge provided you have a look at the other checks in the summary. You can consult the Develocity build scans. |
- this is a follow-up of quarkusio#38107
- this is a follow-up of quarkusio#38107
- this is a follow-up of quarkusio#38107 (cherry picked from commit 7db82da)
Fixes #37741
@michalvavrik Can you give this a spin please? I did test it with your first reproducer (the
code-with-quarkuswith added observer) but not much else yet.I am still trying to understand why we never clear the context state from duplicated context but I didn't get any closer to an explanation yet. It could just be an optimization 🤷
I'll try to some simple test as well but I first wanted to see what CI thinks about it as I only ran tests from few modules locally.