Fix Mutiny schedulers context propagation bug #26242

jponge · 2022-06-20T15:40:02Z

This is part of a coordinated fix across Quarkus and Mutiny where scheduler wrapping would cause Vert.x context propagation not to be done.

See smallrye/smallrye-mutiny#955

Fixes #25818

quarkus-bot · 2022-06-20T15:40:25Z

Thanks for your pull request!

The title of your pull request does not follow our editorial rules. Could you have a look?

title should not contain an issue number (use Fix #1234 in the description instead)

This message is automatically generated by a bot.

luneo7

What about the schedule methods? Shouldn't they also propagate Vert.x context?

luneo7 · 2022-06-20T16:33:32Z

extensions/mutiny/runtime/src/main/java/io/quarkus/mutiny/runtime/MutinyInfrastructure.java

+
+            @Override
+            public ScheduledFuture<?> schedule(Runnable command, long delay, TimeUnit unit) {
+                return mutinyScheduler.schedule(command, delay, unit);


If we schedule something, inside a route handler the Vert.x context won't be propagated, just when execute is called, that would mean schedule it to run right away... If someone use the Infrastructure.getDefaultWorkerPool and call any of the schedule methods it won't propagate the vert.x context.

I'm trying a variation of your code (we won't add another parameter to Infrastructure in Mutiny) but I get an error testing the Mutiny extension:

Errors: [ERROR] MutinyTest » Runtime java.lang.RuntimeException: io.quarkus.builder.ChainBuildException: No producers for required item class io.quarkus.deployment.builditem.ContextHandlerBuildItem

Other than that I have added test cases calling submit and schedule, context does propagate with your changes to the Vert.x extension.

Wrapping in an Optional<...>

Yup, in the mutiny processor the runtimeInit needs to take an Optional<ContextHandlerBuildItem> contextHandlerBuildItem and need to update the code when there is no context handler

Making it runtimeInit accept optional:

Creating the right MutinyScheduler if context handler is not null:

Pushed it to my branch, if you are basing on something there: main...luneo7:mutiny-executors-simple

I already have something like this locally 👍

I just let a null handler being forwarded, and just call Runnable::run() when null in CRSF

@luneo7

Adapted from the draft code from @luneo7 in the discussions of - quarkusio#26242 - quarkusio#25818

jponge · 2022-06-20T20:17:26Z

a06b438 is an adaptation of @luneo7 code, with added tests.

With these changes, all executor methods (scheduleXYZ, submit, execute) should propagate the Vert.x context, so it indeed goes beyond the initial fix. Thanks @luneo7 for insisting on these points.

@geoand @cescoffier I'm keeping this PR as a draft right now so we can discuss, I'm not a specialist of the Vert.x extension and there's also a slight change here. I'll eventually squash commits, etc.

extensions/mutiny/runtime/src/main/java/io/quarkus/mutiny/runtime/MutinyInfrastructure.java

...utiny/runtime/src/main/java/io/quarkus/mutiny/runtime/ContextualRunnableScheduledFuture.java

geoand · 2022-06-21T05:55:19Z

I'll leave this one to you and @cescoffier :)

...utiny/runtime/src/main/java/io/quarkus/mutiny/runtime/ContextualRunnableScheduledFuture.java

extensions/mutiny/runtime/src/main/java/io/quarkus/mutiny/runtime/MutinyInfrastructure.java

cescoffier · 2022-06-21T05:56:49Z

extensions/vertx/runtime/src/main/java/io/quarkus/vertx/core/runtime/VertxCoreRecorder.java

@@ -546,14 +546,15 @@ public Object captureContext() {

            @Override
            public void runWith(Runnable task, Object context) {
-                if (context != null) {
+                ContextInternal currentContext = (ContextInternal) Vertx.currentContext();
+                if (context != null && context != currentContext) {


And now the philosophical question: should we distinguish root context and duplicated context?

I do not believe so, but I think we should consider the question.

Duplicated contexts do not share the context-local data, so I would say we are running with a different context.

Those context are mostly duplicated contexts. When you duplicate a root context, their local data will also by propagated to the duplicate context, the only thing is that changing this local data won't change the root local data.
When I added this check ’if not null and if different from current’ was just a safe guard if trying to run the context handler twice in the same context, so we don't dispatch the vertx context multiple times. Since the executor will span a thread that is not a vertx thread, when you try to get the current vertx context it will always be null, unless the context handler was run multiple times in that thread.
Don't think that we need to distinguish root and duplicated contexts.

@luneo7

…hedulers Fixes quarkusio#25818 This is part of a coordinated fix across Quarkus and Mutiny where scheduler wrapping would cause Vert.x context propagation not to be done. Some changes have been adapted from the draft code from @luneo7 in the discussions of: - quarkusio#26242 - quarkusio#25818 See the matching changes in Mutiny: smallrye/smallrye-mutiny#955

@luneo7

…hedulers Fixes quarkusio#25818 This is part of a coordinated fix across Quarkus and Mutiny where scheduler wrapping would cause Vert.x context propagation not to be done. Some changes have been adapted from the draft code from @luneo7 in the discussions of: - quarkusio#26242 - quarkusio#25818 See the matching changes in Mutiny: smallrye/smallrye-mutiny#955

jponge · 2022-06-21T14:07:52Z

Reworked as a single commit, ready for another review @cescoffier

gsmet · 2022-06-21T15:11:43Z

Wondering if we should backport it to 2.10.1.Final as it looks rather annoying? Please add the triage/backport? label if I'm not mistaken.

jponge · 2022-06-21T15:59:17Z

This is for rather advanced use cases, but yes it's worth a potential backport.

@luneo7

…hedulers Fixes quarkusio#25818 This is part of a coordinated fix across Quarkus and Mutiny where scheduler wrapping would cause Vert.x context propagation not to be done. Some changes have been adapted from the draft code from @luneo7 in the discussions of: - quarkusio#26242 - quarkusio#25818 See the matching changes in Mutiny: smallrye/smallrye-mutiny#955

jponge · 2022-06-22T07:28:47Z

(rebased)

jponge · 2022-06-22T16:04:36Z

CI is green, waiting for @cescoffier to do another review

jponge · 2022-06-22T16:05:00Z

Ah... conflicts, great

@luneo7

…hedulers Fixes quarkusio#25818 This is part of a coordinated fix across Quarkus and Mutiny where scheduler wrapping would cause Vert.x context propagation not to be done. Some changes have been adapted from the draft code from @luneo7 in the discussions of: - quarkusio#26242 - quarkusio#25818 See the matching changes in Mutiny: smallrye/smallrye-mutiny#955

jponge · 2022-06-22T16:22:55Z

Fixed

@luneo7

…hedulers Fixes quarkusio#25818 This is part of a coordinated fix across Quarkus and Mutiny where scheduler wrapping would cause Vert.x context propagation not to be done. Some changes have been adapted from the draft code from @luneo7 in the discussions of: - quarkusio#26242 - quarkusio#25818 See the matching changes in Mutiny: smallrye/smallrye-mutiny#955 (cherry picked from commit 059b7b7)

quarkus-bot bot added area/dependencies Pull requests that update a dependency file area/mutiny area/smallrye labels Jun 20, 2022

jponge changed the title ~~Fix for #25818 and upgrade to Mutiny 1.6.0~~ Fix #25818 and upgrade to Mutiny 1.6.0 Jun 20, 2022

jponge force-pushed the bug/25818 branch from b9c4b8a to ad6bfc9 Compare June 20, 2022 15:46

jponge changed the title ~~Fix #25818 and upgrade to Mutiny 1.6.0~~ Fix Mutiny schedulers context propagation bug and upgrade to Mutiny 1.6.0 Jun 20, 2022

jponge requested a review from cescoffier June 20, 2022 15:48

jponge added the area/vertx label Jun 20, 2022

jponge requested a review from geoand June 20, 2022 16:22

luneo7 reviewed Jun 20, 2022

View reviewed changes

jponge marked this pull request as draft June 20, 2022 17:25

This comment has been minimized.

Sign in to view

jponge added a commit to jponge/quarkus that referenced this pull request Jun 20, 2022

Capture context across all scheduler methods

a06b438

Adapted from the draft code from @luneo7 in the discussions of - quarkusio#26242 - quarkusio#25818

jponge force-pushed the bug/25818 branch from ad6bfc9 to a06b438 Compare June 20, 2022 20:13

luneo7 reviewed Jun 20, 2022

View reviewed changes

extensions/mutiny/runtime/src/main/java/io/quarkus/mutiny/runtime/MutinyInfrastructure.java Outdated Show resolved Hide resolved

luneo7 approved these changes Jun 20, 2022

View reviewed changes

geoand reviewed Jun 21, 2022

View reviewed changes

...utiny/runtime/src/main/java/io/quarkus/mutiny/runtime/ContextualRunnableScheduledFuture.java Outdated Show resolved Hide resolved

cescoffier requested changes Jun 21, 2022

View reviewed changes

jponge force-pushed the bug/25818 branch from 1afe6b5 to 07a3893 Compare June 21, 2022 07:35

jponge requested a review from cescoffier June 21, 2022 07:44

jponge added the kind/bugfix label Jun 21, 2022

jponge force-pushed the bug/25818 branch from 07a3893 to 014122a Compare June 21, 2022 13:48

jponge marked this pull request as ready for review June 21, 2022 13:48

jponge force-pushed the bug/25818 branch from 014122a to e70072f Compare June 21, 2022 13:49

jponge added the triage/backport? label Jun 21, 2022

jponge changed the title ~~Fix Mutiny schedulers context propagation bug and upgrade to Mutiny 1.6.0~~ Fix Mutiny schedulers context propagation bug Jun 21, 2022

This comment has been minimized.

Sign in to view

jponge force-pushed the bug/25818 branch from e70072f to a3e04ed Compare June 22, 2022 07:25

jponge force-pushed the bug/25818 branch from a3e04ed to 059b7b7 Compare June 22, 2022 16:22

cescoffier approved these changes Jun 23, 2022

View reviewed changes

cescoffier merged commit d28eb6e into quarkusio:main Jun 23, 2022

quarkus-bot bot added this to the 2.11 - main milestone Jun 23, 2022

gsmet modified the milestones: 2.11 - main, 2.10.1.Final Jun 28, 2022

gsmet removed the triage/backport? label Jun 28, 2022

luneo7 mentioned this pull request Jul 6, 2022

Mutiny is not compatible with quarkus opentelemetry #25453

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Mutiny schedulers context propagation bug #26242

Fix Mutiny schedulers context propagation bug #26242

jponge commented Jun 20, 2022

quarkus-bot bot commented Jun 20, 2022

luneo7 left a comment

luneo7 Jun 20, 2022

jponge Jun 20, 2022

jponge Jun 20, 2022

luneo7 Jun 20, 2022

luneo7 Jun 20, 2022

luneo7 Jun 20, 2022

jponge Jun 20, 2022

jponge Jun 20, 2022

This comment has been minimized.

jponge commented Jun 20, 2022 •

edited

Loading

geoand commented Jun 21, 2022

cescoffier Jun 21, 2022

jponge Jun 21, 2022

luneo7 Jun 21, 2022

jponge commented Jun 21, 2022

gsmet commented Jun 21, 2022

jponge commented Jun 21, 2022

This comment has been minimized.

jponge commented Jun 22, 2022

jponge commented Jun 22, 2022

jponge commented Jun 22, 2022

jponge commented Jun 22, 2022

Fix Mutiny schedulers context propagation bug #26242

Fix Mutiny schedulers context propagation bug #26242

Conversation

jponge commented Jun 20, 2022

quarkus-bot bot commented Jun 20, 2022

luneo7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

jponge commented Jun 20, 2022 • edited Loading

geoand commented Jun 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jponge commented Jun 21, 2022

gsmet commented Jun 21, 2022

jponge commented Jun 21, 2022

This comment has been minimized.

jponge commented Jun 22, 2022

jponge commented Jun 22, 2022

jponge commented Jun 22, 2022

jponge commented Jun 22, 2022

jponge commented Jun 20, 2022 •

edited

Loading