-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use the new Vertx local context storage #40725
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
package io.quarkus.vertx.core.runtime; | ||
|
||
import java.util.concurrent.atomic.AtomicReferenceArray; | ||
import java.util.function.Supplier; | ||
|
||
import io.vertx.core.Context; | ||
import io.vertx.core.spi.context.storage.AccessMode; | ||
|
||
final class QuarkusAccessModes { | ||
|
||
/** | ||
* Beware this {@link AccessMode#getOrCreate(AtomicReferenceArray, int, Supplier)} because, differently from | ||
* {@link io.vertx.core.spi.context.storage.ContextLocal#get(Context, Supplier)}, | ||
* is not suitable to be used with {@link io.vertx.core.spi.context.storage.ContextLocal#get(Context, AccessMode, Supplier)} | ||
* with the same guarantees of atomicity i.e. the supplier can get called more than once by different racing threads! | ||
*/ | ||
public static final AccessMode ACQUIRE_RELEASE = new AccessMode() { | ||
@Override | ||
public Object get(AtomicReferenceArray<Object> locals, int idx) { | ||
return locals.getAcquire(idx); | ||
} | ||
|
||
@Override | ||
public void put(AtomicReferenceArray<Object> locals, int idx, Object value) { | ||
// This is still ensuring visibility across threads and happens-before, | ||
// but won't impose setVolatile total ordering i.e. StoreLoad barriers after write | ||
// to make it faster | ||
locals.setRelease(idx, value); | ||
} | ||
|
||
@Override | ||
public Object getOrCreate(AtomicReferenceArray<Object> locals, int idx, Supplier<Object> initialValueSupplier) { | ||
Object value = locals.getAcquire(idx); | ||
if (value == null) { | ||
value = initialValueSupplier.get(); | ||
locals.setRelease(idx, value); | ||
} | ||
return value; | ||
} | ||
}; | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,11 @@ | ||
package io.quarkus.vertx.core.runtime.context; | ||
|
||
import static io.quarkus.vertx.runtime.storage.QuarkusLocalStorageKeyVertxServiceProvider.ACCESS_TOGGLE_KEY; | ||
|
||
import io.smallrye.common.vertx.VertxContext; | ||
import io.vertx.core.Context; | ||
import io.vertx.core.Vertx; | ||
import io.vertx.core.spi.context.storage.ContextLocal; | ||
|
||
/** | ||
* This is meant for other extensions to integrate with, to help | ||
|
@@ -39,7 +42,6 @@ | |
*/ | ||
public final class VertxContextSafetyToggle { | ||
|
||
private static final Object ACCESS_TOGGLE_KEY = new Object(); | ||
public static final String UNRESTRICTED_BY_DEFAULT_PROPERTY = "io.quarkus.vertx.core.runtime.context.VertxContextSafetyToggle.UNRESTRICTED_BY_DEFAULT"; | ||
|
||
/** | ||
|
@@ -50,6 +52,12 @@ public final class VertxContextSafetyToggle { | |
private static final boolean UNRESTRICTED_BY_DEFAULT = Boolean.getBoolean(UNRESTRICTED_BY_DEFAULT_PROPERTY); | ||
private static final boolean FULLY_DISABLED = Boolean.getBoolean(FULLY_DISABLE_PROPERTY); | ||
|
||
public static ContextLocal<Boolean> registerAccessToggleKey() { | ||
if (FULLY_DISABLED) | ||
return null; | ||
return ContextLocal.registerLocal(Boolean.class); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @vietj is this ok? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is not how it should be used imho There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what do you mean? |
||
} | ||
|
||
/** | ||
* Verifies if the current Vert.x context was flagged as safe | ||
* to be accessed by components which expect non-concurrent | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
package io.quarkus.vertx.runtime.storage; | ||
|
||
import io.quarkus.arc.InjectableContext; | ||
import io.quarkus.vertx.core.runtime.context.VertxContextSafetyToggle; | ||
import io.vertx.core.impl.VertxBuilder; | ||
import io.vertx.core.spi.VertxServiceProvider; | ||
import io.vertx.core.spi.context.storage.ContextLocal; | ||
|
||
/** | ||
* 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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @geoand @cescoffier I don't like to make spi loading to happen just for this TBH and I could, maybe, hack the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can skip the service loading in this case and just use this class, no? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I don't believe we should allow any other implementation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds fine to me |
||
|
||
public static final ContextLocal<Boolean> ACCESS_TOGGLE_KEY = VertxContextSafetyToggle.registerAccessToggleKey(); | ||
public static final ContextLocal<InjectableContext.ContextState> REQUEST_SCOPED_LOCAL_KEY = ContextLocal | ||
.registerLocal(InjectableContext.ContextState.class); | ||
|
||
@Override | ||
public void init(VertxBuilder builder) { | ||
|
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
io.quarkus.vertx.runtime.storage.QuarkusLocalStorageKeyVertxServiceProvider | ||
franz1981 marked this conversation as resolved.
Show resolved
Hide resolved
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?