quarkus icon indicating copy to clipboard operation
quarkus copied to clipboard

Use the new Vertx local context storage

Open franz1981 opened this issue 1 year ago • 5 comments

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

  1. 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...)
  2. 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
  3. 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)

franz1981 avatar May 20 '24 16:05 franz1981

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?

franz1981 avatar May 22 '24 12:05 franz1981

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 avatar May 23 '24 12:05 franz1981

This PR will remain in draft till https://github.com/eclipse-vertx/vert.x/pull/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 avatar Jun 04 '24 09:06 franz1981

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

franz1981 avatar Jun 18 '24 07:06 franz1981

@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

franz1981 avatar Jun 27 '24 12:06 franz1981

@franz1981 should we go forward with this, or should it wait Vertx 5?

cescoffier avatar Dec 20 '24 06:12 cescoffier

Hi @cescoffier I would love to move this forward but there is a missing "feature" in vertx 4.x i.e. moving local storage values from duplicated ctx with a filter - or just been able to query them. In an hour I will attach the relevant issue which explain this, because the same applies to vertx 5: if we want to use the new API we need it on vertx

franz1981 avatar Dec 20 '24 07:12 franz1981

@franz1981 do you have the relevant issues?

cescoffier avatar Jan 08 '25 07:01 cescoffier

@cescoffier I cannot see an issue but https://github.com/eclipse-vertx/vert.x/pull/5215 on vertx side which is the in progress work on it. IDK if Julien is aware of exactly we need

franz1981 avatar Jan 08 '25 14:01 franz1981