Fix for Context Propagation on Reactive Messaging #27802
Conversation
quarkus-bot
bot
added
area/arc
...n/java/io/quarkus/smallrye/reactivemessaging/runtime/SmallRyeReactiveMessagingLifecycle.java
Outdated
Show resolved
Hide resolved
| try { | ||
| mediatorManager.start(); | ||
| } catch (Exception e) { | ||
| if (e instanceof DeploymentException || e instanceof DefinitionException) { | ||
| throw e; | ||
| } | ||
| throw new DeploymentException(e); | ||
| } finally { | ||
| if (isRequestScopeActive) { | ||
| Arc.container().requestContext().activate(); |
There was a problem hiding this comment.
I really don't know, but I'm wondering if the initial request context shouldn't be restored rather than a new one being activated? Are we ok to throw a way the context state?
There was a problem hiding this comment.
When you say it, it makes sense. I've done as you suggest, but I didn't resolve the conversation to double check
There was a problem hiding this comment.
I think this makes sense.
EDIT: as it is now, I mean. Restoring the original request context.
|
Thank you so much for digging! I'll let others review though, my knowledge of CDI semantic expectations is too limited. |
ActivateRequestContextInterceptor : Destroy request scope property when reactive method returns on different thread
ozangunalp
force-pushed
the
fix_context_propagation_reactive_messaging_request_context
branch
from
0bd1f6c to
0f32c84
Compare
| .thenCompose(state -> proceedWithStage(ctx).whenComplete((r, t) -> { | ||
| requestContext.destroy(state); | ||
| requestContext.deactivate(); | ||
| })); |
There was a problem hiding this comment.
I was wondering what's the difference here, as terminate() == destroy() + deactivate(). It's that you explicitly pass state to destroy, right?
There was a problem hiding this comment.
Ah all changes in this file do this same thing. I wonder if it's just a precaution, or if destroy() without parameters finds (and destroys) a different context than it should...
In any case, being explicit can't hurt.
There was a problem hiding this comment.
Exactly, otherwise it'll destroy the state from the current context.
|
There seems to be a problem in my reproducer still; although I'm not sure if my test is valid or perhaps I have some invalid assumptions. W/o this PR, I was having But with this patch appied, while the Kafka one seems fixed, the other one starts failing - I need to look better into it but it seems to indicate a new leak. |
|
Nevermind, I had a problem in my test. |
|
@Sanne I forked your test and added some more here: https://github.com/ozangunalp/quarkus-scope-tests, with this branch they all pass for me. |
|
Right, seems all good. I'm verifying now if it also fixes #27166 |
|
@gsmet N.B. I've marked this both as candidate for backporting and as breaking change (!).
|
|
It's not backportable, removing the label. |
The issue revealed by @Sanne in https://github.com/Sanne/quarkus-scope-tests
Reactive Messaging doesn't handle CDI request scope, it only ensures that consumer methods are dispatched on a duplicated vert.x context per message.
When a request scoped bean is used in consumer methods, the request context should not be active by default and can be activated through the
@ActivateRequestContextannotation, properly creating request scoped beans and destroying them on completion.The fix involves:
ActivateRequestContextInterceptorterminates request context properly on returning Uni.ActivateRequestContextInterceptordestroys request context state properly when reactive method completes on a different thread (or a different Vert.x context)A separate PR will follow on smallrye-reactive-messaging for returning message (n)ack's on the message context.
Fixes #27166