Use the new Vertx local context storage #40725

franz1981 · 2024-05-20T16:31:39Z

This is to keep up with the Vertx API changes for eclipse-vertx/vert.x@88cf77b, although I'm not very happy with the current state (that's why is in draft):

VertxLocalsHelper. is "stealthy" using the new API by intercepting when users are passing ContextLocal-like keys, which means performing 2 instanceof checks (one being an interface check too, which is not yet safe with JDK till 21...)
As written in the TODO parts, we have some ordering issue: we need the keys statically defined as ACCESS_TOGGLE_KEY to be initialized before instantiating the Vertx instance has to use them, due to https://github.com/eclipse-vertx/vert.x/blob/4.5.7/src/main/java/io/vertx/core/impl/VertxImpl.java#L194 which is forcing every new context creation to dedicate the fixed amount of "optimized" keys in the context's local storage
The new AccessMode I've defined is good enough that having a new method in AccessMode::putRelease, would have been better IMO (@vietj )

For @mkouba and @Ladicek : the new "optimized" API for local storage is double edge sword because it needs some care on init ordering, need some care in dev mode (I think, @geoand ? @Sanne ?) and requires to create local "permanent" keys only for the ones we're 100% are going to be stored on each context (and I need some help from @Ladicek and @mkouba for such, I have no IDEA!)

Additionally there are some weird things in the original API (but is ok, the purpose of dog fooding is indeed to try things and found how to use them!): @vietj see https://github.com/quarkusio/quarkus/compare/main...franz1981:quarkus:local_ctx?expand=1#diff-af571e338cf42e877b9b99b70840f0842b293365f18c24d5fb24c930e5b0b064R64: this one is just wrong, because we cannot query all the local storage ContextLocal's values stored in the source context...we cannot do it unless we have a new method as ContextInternal::addLocals(ContextInternal from, AccessMode accessMode)

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

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

extensions/vertx/runtime/src/main/java/io/quarkus/vertx/runtime/VertxCurrentContextFactory.java

franz1981 · 2024-05-22T12:41:42Z

PTAL @mariofusco @mkouba

Note: I'm still concerned about the ordering by which these statically defined "keys" are generated, in order to happen before Vertx is created: any suggestion on how to do it the quarkus-way?

extensions/vertx/runtime/src/main/java/io/quarkus/vertx/runtime/VertxCurrentContextFactory.java

franz1981 · 2024-05-23T12:47:20Z

In order to enforce the ordering of key creation I should make a vertx provider which is look-up before Vertx instance is allocated, see doc at https://github.com/eclipse-vertx/vert.x/blob/4.5.7/src/main/java/io/vertx/core/spi/context/storage/ContextLocal.java#L25C100-L25C120

franz1981 · 2024-05-24T09:35:42Z

...c/main/java/io/quarkus/vertx/runtime/storage/QuarkusLocalStorageKeyVertxServiceProvider.java

+ * This provider exists with the sole purpose of reliably get the optimized local keys created before
+ * the Vertx instance is created.
+ */
+public class QuarkusLocalStorageKeyVertxServiceProvider implements VertxServiceProvider {


@geoand @cescoffier I don't like to make spi loading to happen just for this TBH and I could, maybe, hack the Vertx recorder to just perform the init at the right time, but IDK what's the cost of this, usually. Any suggestion?

The question is: can we preload this at build time (static init). It would be interesting to do that and just use the result at runtime. I don't believe there is a good reason to change the SPI at runtime.

I think we can skip the service loading in this case and just use this class, no?

Yeah, I don't believe we should allow any other implementation.

thanks both, yeah, I'm not very happy about it either: so i can just have a not vertx service provider, but an holder class which is going to be initialized in the vertx recorder before vertx instances are allocated, wdyt?

Sounds fine to me

extensions/vertx/runtime/src/main/java/io/quarkus/vertx/runtime/VertxCurrentContextFactory.java

franz1981 · 2024-05-24T09:56:07Z

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

@@ -22,6 +24,11 @@ public static void throwOnRootContextAccess() {
    public static <T> T getLocal(ContextInternal context, Object key) {
        if (VertxContext.isDuplicatedContext(context)) {
            // We are on a duplicated context, allow accessing the locals
+            if (key instanceof ContextLocalImpl<?>) {


@cescoffier I will probably add the new ContextLocal-based methods to avoid this terrible hack .-.
I made it in order to have the existing code in the toggle to just use the now-deprecated methods and still get the benefit of the new API, but given that these methods are going to disappear to favour the new API, I'll probably tackle this on the current PR already, wdyt?

...ns/vertx/runtime/src/main/resources/META-INF/services/io.vertx.core.spi.VertxServiceProvider

franz1981 · 2024-06-03T13:46:57Z

...tx/runtime/src/main/java/io/quarkus/vertx/core/runtime/context/VertxContextSafetyToggle.java

+    public static ContextLocal<Boolean> registerAccessToggleKey() {
+        if (FULLY_DISABLED)
+            return null;
+        return ContextLocal.registerLocal(Boolean.class);


@vietj is this ok?

this is not how it should be used imho

what do you mean?
In our context (quarkus) we can disable specific keys, in a stable way (via the sys property), meaning that we don't have to register any local key there.
Do you have another way to provide a similar behaviour?

franz1981 · 2024-06-04T09:29:42Z

This PR will remain in draft till eclipse-vertx/vert.x#5215 is going to be into Vertx 4.x because ATM we have no way to duplicate "optimized" keys while creating a new duplicate context.

franz1981 · 2024-06-18T07:43:48Z

This PR is still blocked by the mentioned eclipse-vertx/vert.x#5215 one, which should be ported to 4.x as well.

franz1981 · 2024-06-27T12:29:32Z

@cescoffier https://github.com/eclipse-vertx/vertx-grpc/blob/main/vertx-grpc-context-storage/src/main/java/io/grpc/override/ContextStorageOverride.java#L27 probably I need to send a PR just to fix this which is should be broken in master

quarkus-bot bot added the area/vertx label May 20, 2024

franz1981 commented May 20, 2024

View reviewed changes

extensions/vertx/runtime/src/main/java/io/quarkus/vertx/core/runtime/QuarkusAccessModes.java Outdated Show resolved Hide resolved

mariofusco reviewed May 21, 2024

View reviewed changes

extensions/vertx/runtime/src/main/java/io/quarkus/vertx/core/runtime/VertxLocalsHelper.java Show resolved Hide resolved

mkouba reviewed May 21, 2024

View reviewed changes

mariofusco reviewed May 22, 2024

View reviewed changes

extensions/vertx/runtime/src/main/java/io/quarkus/vertx/runtime/VertxCurrentContextFactory.java Show resolved Hide resolved

mariofusco reviewed May 22, 2024

View reviewed changes

extensions/vertx/runtime/src/main/java/io/quarkus/vertx/runtime/VertxCurrentContextFactory.java Outdated Show resolved Hide resolved

franz1981 commented May 24, 2024

View reviewed changes

franz1981 force-pushed the local_ctx branch from cc907b4 to f6fe8f9 Compare May 24, 2024 09:43

franz1981 commented May 24, 2024

View reviewed changes

extensions/vertx/runtime/src/main/java/io/quarkus/vertx/runtime/VertxCurrentContextFactory.java Show resolved Hide resolved

franz1981 commented May 24, 2024

View reviewed changes

...ns/vertx/runtime/src/main/resources/META-INF/services/io.vertx.core.spi.VertxServiceProvider Show resolved Hide resolved

Use the new Vertx local context storage

5979b09

franz1981 force-pushed the local_ctx branch from f6fe8f9 to 5979b09 Compare May 24, 2024 10:15

franz1981 mentioned this pull request May 30, 2024

Support duplicated context duplication eclipse-vertx/vert.x#5215

Open

1 task

franz1981 commented Jun 3, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the new Vertx local context storage #40725

Use the new Vertx local context storage #40725

franz1981 commented May 20, 2024 •

edited

Loading

franz1981 commented May 22, 2024

franz1981 commented May 23, 2024

franz1981 May 24, 2024

cescoffier Jun 3, 2024

geoand Jun 3, 2024

cescoffier Jun 4, 2024

franz1981 Jun 4, 2024 •

edited

Loading

geoand Jun 4, 2024

franz1981 May 24, 2024

franz1981 Jun 3, 2024 •

edited

Loading

vietj Jun 11, 2024

franz1981 Jun 18, 2024

franz1981 commented Jun 4, 2024

franz1981 commented Jun 18, 2024

franz1981 commented Jun 27, 2024

Use the new Vertx local context storage #40725

Are you sure you want to change the base?

Use the new Vertx local context storage #40725

Conversation

franz1981 commented May 20, 2024 • edited Loading

franz1981 commented May 22, 2024

franz1981 commented May 23, 2024

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

franz1981 Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

franz1981 Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

franz1981 commented Jun 4, 2024

franz1981 commented Jun 18, 2024

franz1981 commented Jun 27, 2024

franz1981 commented May 20, 2024 •

edited

Loading

franz1981 Jun 4, 2024 •

edited

Loading

franz1981 Jun 3, 2024 •

edited

Loading