Context propagation and Mutiny infinite Multi

[mkouba]

While working on #23300 we've identified a problem with CP and a deferred infinite Multi. In short, a request context map leaks from a StartupEvent observer notification, due to use of a deferred Multi that handles infinite stream. In other words, the context map is captured and then used forever. As a consequence, whenever a request scoped bean is used in a Multi callback a bean instance is created and never destroyed.

@cescoffier is working on a fix for Reactive Messaging but I believe that it's a general problem that deserves a more robust solution.

First, I'd propose to add an isValid() method to the io.quarkus.arc.InjectableContext.ContextState and skip the context activation in the ArcContextProvider wherever needed. Possibly also log a warning or something like that.

We could also introduce a new annotation to mark a business method to be executed with no request context active, e.g. @DeactivateRequestContext (in contrast with @javax.enterprise.context.control.ActivateRequestContext). Functionally similar to @Transactional(TxType.NOT_SUPPORTED). That would allow users to effectively ignore context propagation inside a bussiness method invocation.

Any other ideas, concerns?

[mkouba]

CC @FroMage @manovotn

[FroMage]

Those two solutions look good. I agree that we should not reactivate terminated contexts. The question will be whether to throw when this happens or not. We could throw when this happens.

If we don't throw, and:

• something requires the request context, it will throw, so we'll still get an error, but will it be too late? It's not clear, perhaps it's enough.
• nothing requires the request context, so everything will be fine

So perhaps it's best to not throw.

A third solution is to not capture the request context in that method, using https://quarkus.io/guides/context-propagation#overriding-the-propagated-contexts-using-annotations

[mkouba]

A third solution is to not capture the request context in that method, using https://quarkus.io/guides/context-propagation#overriding-the-propagated-contexts-using-annotations

+1 didn't know about this one.

[mkouba]

• something requires the request context, it will throw, so we'll still get an error, but will it be too late? It's not clear, perhaps it's enough.

In theory, we could activate a new context just for the particular invocation.

[mkouba]

We could also introduce a new annotation to mark a business method to be executed with no request context active,

In fact, we could introduce a more general annotation that would allow to specify various requirements, e.g. something like @Requestional(NEW), @Requestional(DEACTIVATE) and @Requestional(ACTIVE) (default value), where the last one is functionally equivalent to @ActivateRequestContext. Of course, the annotation name would be a subject of a long debate ;-)

[manovotn]

@mkouba I am not sure I understand who'd be responsible for invocation of isValid() on the CDI context. It also leads to weird behavior because whether valid or not, to user the context is just running and populated with beans. However, at some points where they attempt propagation, it just won't work even though the context seems perfectly OK.

I think the main issue here is how we stop Mutiny from reusing the same context endlessly and whether we want the context to be by default propagated for these infinite scenarios (I don't think so) or whether we want users to explicitly declare it.

[mkouba]

I am not sure I understand who'd be responsible for invocation of isValid() on the CDI context.

First, it's not on the context but its state. The ArcContextProvider should check the validity whenever it attempts to use it.

It also leads to weird behavior ...

IMO the result would be better than the current weird and incorrect behavior.

I think the main issue here is how we stop Mutiny from reusing the same context endlessly and whether we want the context to be by default propagated for these infinite scenarios (I don't think so) or whether we want users to explicitly declare it.

That's a good point. However, I don't think we can detect infinite streams easily... Also it would be a breaking change (which might be ok but still ;-).

[cescoffier]

Only the user / middleware can tell us if the stream is intended to be infinite. A Multi is potentially infinite. However, "infinite" might not be the right wording.

Indeed, I just found another case. I would consider odd but not impossible. It would apply to both Uni and Multi: things that never sends anything. They don't complete or fail, they don't emit items. We often use such kind of objects in tests, but it's not limited to tests. In this case, we would have the same behavior. The context would be captured. The only difference is that it will never be restored (because we don't get any events).

[manovotn]

First, it's not on the context but its state. The ArcContextProvider should check the validity whenever it attempts to use it.

Sorry, my bad wording.
The point I was trying to make is that the provider on its own lacks the information to be able to tell when not to propagate anymore. It only gets notified for when propagation is already attempted and works with given state (which may or may not be valid), but who controls the validity in your proposal? User defined annotation?

IMO the result would be better than the current weird and incorrect behavior.

I'd say both variant are equally wrong and incorrect :)

Only the user / middleware can tell us if the stream is intended to be infinite...

Thanks for clarification.

I think we need to first decide whether the goal is to have request context active or propagate in these situations.
And if we need to have it propagated, then we should perhaps allow users to declare an annotation that stops this propagation for scenarios that @cescoffier described (assuming users are the ones who can tell anyway)?

[mkouba]

The point I was trying to make is that the provider on its own lacks the information to be able to tell when not to propagate anymore.

In theory, yes. In practice, a context state should not be valid (and propagated) when the request context is destroyed. And we do destroy the request context e.g. when an HTPP request is processed or an observer notification finished.

[manovotn]

The point I was trying to make is that the provider on its own lacks the information to be able to tell when not to propagate anymore.

In theory, yes. In practice, a context state should not be valid (and propagated) when the request context is destroyed. And we do destroy the request context e.g. when an HTPP request is processed or an observer notification finished.

So we would basically invalidate the state when we invoke destroy() on req. context. Ok, now I got what you mean.

As for how ArcContextProvider handles invalid state - I suggest we just create a fresh state and go from there.
I could try to prepare a PR if you want me to @mkouba.

[Sanne]

Not meaning to derail this too much, but would it be possible to restrict / validate the possible scopes of some components?

So to guide the end users into catching mistakes in scope and lifecycle expectations.

For example:

• mark a Kafka stream consumer as "expected long lived". e.g. only an @ApplicationScoped bean can inject things like a KafkaStreams and similar.
• we mark things like JPA's EntityManager as the opposite: not allowed to be injected in long-lived objects.

I haven't thought it through, scopes are really not my area of expertise, but I hope we could validate at least some special cases to bail out at build time?

[manovotn]

Theoretically, you can do such inspection even via some extension (as in, check what is every bean injecting) but in practice I don't think it makes much sense because dynamic resolution can always bypass this. Not to mention you'd only check few components and users can create their own beans with wrong scopes that they can then inject.

IMO of the the causes of this is our overzealous activation of req. context in many cases. Combine that with context propagation and this is what we get :)
Apart from req. context, there isn't much else doing harm because both app context and singleton context are basically "global" anyway.

[Sanne]

Ok, thank you :)

I'll put this aside for now; but maybe one day we'll want at least to check in what kind of scope an EntityManager ends up in - I totally understand it would be impossible to catch all user mistakes, but I wonder if catching some "essential" mistakes could help at least educating.