ArC - introduce the CurrentContextFactory abstraction

[mkouba]

• this API allows quarkus to supply an optimized backend for any
  non-shared context of a normal scope
• currently it's only used by the request context
• an extension can provide a custom factory via
  ContextReferenceFactoryBuildItem

[mkouba]

The workflow is simple: the request context will use a ContextReference created via the ContextReferenceFactory. The default impl is using TheadLocal variables. A quarkus extension, e.g. vertx, can provide an optimized implementation of ContextReference via ContextReferenceFactoryBuildItem.

[mkouba]

Hm, maybe ContextStateReference or StateReference would be a better name...

[stuartwdouglas]

I think this looks good. I will look at building on top of this.

[geoand]

👍

[manovotn]

+1, this looks great.

Hm, maybe ContextStateReference or StateReference would be a better name...

I'd prefer ContextStateReference as well just to make it super clear but it's nitpicking.

One more note - the ThreadLocalContextReference and the factory for it could IMO be directly in Arc. Especially if this SPI can be used by any custom normal scope whatsoever.

[mkouba]

One more note - the ThreadLocalContextReference and the factory for it could IMO be directly in Arc.

It is in ArC... Do you mean the io.quarkus.arc API package? I don't think it's necessary because a custom context can just obtain the factory via Arc.container().getContextReferenceFactory() and the default impl is threadlocal-based.

[manovotn]

Yes, I meant the API package but failed to mention it, sorry.

don't think it's necessary because a custom context can just obtain the factory via Arc.container().getContextReferenceFactory()

Ok, fair enough, I didn't realize that. Consider my complaint obsolete :-)

[FroMage]

SR-CP had this already created in a new Maven project with zero dependency:

The idea was that you declared your storage need with https://github.com/smallrye/smallrye-context-propagation/blob/main/storage/src/main/java/io/smallrye/context/storage/spi/StorageDeclaration.java which allowed our extension to pick up all storage slots: FroMage@b90e4e5

Here's how ArC could then declare its storage requirements and use the new storage: FroMage@372c229

Note that we went with ThreadLocal because that minimises other libraries changes to replacing new ThreadLocal<>() with StorageManager.threadLocal(RequestContextStorageDeclaration.class);

[FroMage]

On non-Quarkus, the default storage manager hands out real ThreadLocal instances: https://github.com/smallrye/smallrye-context-propagation/blob/main/storage/src/main/java/io/smallrye/context/storage/impl/DefaultStorageManager.java

But for Quarkus, the ThreadLocals will store stuff in custom thread fields: FroMage@b90e4e5#diff-f36606de9fadce013fb441b209205c1ff4b55eaae93d2a1d5c3fa7549e6003ecR100 . We only have to change that to store them in the Vert.x context.

[mkouba]

This abstraction is an ArC-specific bit that is only used by ArC contexts. I don't want to add the smallrye-context-propagation-storage dependency in the ArC core, i.e. io.quarkus.arc:arc. Ofc, we can implement this in quarkus-arc.

[FroMage]

Well, how do you get other projects to use the ArC storage then if there's no module they can import to get it?

[mkouba]

Well, how do you get other projects to use the ArC storage then if there's no module they can import to get it?

I'm not sure I understand. Other projects can simply use the beans stored in the CDI contexts. We don't want them to use the ArC internals...

[stuartwdouglas]

Here is something (a bit hacked up) built on top of this that should be suitable for testing:
https://github.com/quarkusio/quarkus/compare/main...stuartwdouglas:arc-context-store?expand=1

The interesting bit here is that to get the result we want we need to effectively deliberately 'leak' the context by ignoring the 'remove' method. As we know the context is tied to the request this should be safe, as the data will now have the same lifecycle as the context data.

@Sanne any chance you could benchmark this?

[FroMage]

I'm not sure I understand. Other projects can simply use the beans stored in the CDI contexts. We don't want them to use the ArC internals...

That's precisely my point: we have an independent module already with zero dependencies, that abstracts ThreadLocals, that JTA and RESTEasy and any lib can use without depending on CDI or ArC, and even ArC can use, and then we can store all these in the Vert.x context and call it a day.

[mkouba]

That's precisely my point: we have an independent module already with zero dependencies, that abstracts ThreadLocals, that JTA and RESTEasy and any lib can use without depending on CDI or ArC, and even ArC can use, and then we can store all these in the Vert.x context and call it a day.

Well, this PR does not prevent this solution except that I want to have the ArC core more pluggable and not tied to the API of the smallrye-context-propagation-storage. We could add the smallrye-context-propagation-storage dependency to the quarkus-arc and implement StorageDeclaration there...

[FroMage]

But ContextReference and smallrye-context-propagation-storage appear to do the exact same thing. Perhaps you're thinking that because of its name that module depends on CP, but it doesn't.

[mkouba]

But ContextReference and smallrye-context-propagation-storage appear to do the exact same thing.

Almost. A ContextReference impl does not need to use threadlocals at all...

[manovotn]

@mkouba @Sanne @stuartwdouglas I have rebased this branch and resolved conflicts with the recent changes to req. context propagation. I think we should restart the discussion here as this is still a valuable addition.

Some time ago, Stuart created a testable PoC for this PR, see #22319 (comment)

We can still take that commit and slap it on top of this rebased branch just fine.
@Sanne would you be able to benchmark it to see the actual numbers this would give us?

[mkouba]

I've rebased this PR one more time and also renamed the ContextReference abstraction to CurrentContext (which is IMO closer to the spec wording).

Unfortunately, this means that Stuart's PoC would have to be rebased as well.

In any case, I believe that this abstraction makes a lot of sense and I'd like to have it merged no matter whether we'll have some benchmarks available or not (and given the current workload it seems that we won't have them any time soon ;-).

[Ladicek]

Just a few suggestions to unify naming, otherwise LGTM!

[mkouba]

Just a few suggestions to unify naming, otherwise LGTM!

Thanks. Comments are addressed.

[Sanne]

awesome, thanks!
I still plan on running benchmarks, but having it merged will help me a lot :)