RequestContextController spec impossible to implement

[struberg]

I'm not really happy with the RequestContextController. And it appears to me that the spec is broken.

Here is the important part from the spec:

6.5.2. Activating Built In Contexts
Certain built in contexts support the ability to be activated and deactivated. This allows developers to control built-in contexts in ways that they could also manage custom built contexts.
When activating and deactivating built in contexts, it is important to realize that they can only be activated if not already active within a given thread.
6.5.2.1. Activating a Request Context
Request contexts can be managed either programmatically or via interceptor.
To programmatically manage request contexts, the container provides a built in bean that is @dependent scoped and of type RequestContextController that allows you to activate and deactivate a request context on the current thread. The object should be considered stateful, invoking the same instance on different threads may not work properly, non-portable behavior may occur.
public interface RequestContextController {
boolean activate();
void deactivate() throws ContextNotActiveException;
}
When the activate() method is called, if the request context is not already active on the current thread then it will be activated and the method returns true. Otherwise, the method returns false.
When the deactivate() method is called, if this controller started the request context then the request context is stopped. The method does nothing if this controller did not activate the context

The problem is that there are 2 ways to use that part

a.)

boolean didActivate = reqCtxCtrl.activate();
...
if (didActivate) reqCtxCrl.deactivate();

b.)

try {
  reqCtxCtrl.activate();
  ...
} finally {
  reqCtxCrl.deactivate();
}

The problematic part is nesting.
In case a) we got maybe 7 calls to activate() but only 1 to deactivate.
In case b) we got 7 calls to activate and 7 to deactivate();

There is simply no way to implement this in a clean way. A simple boolean flag does not help because of concurrency. A ThreadLocal does not help much either. If we use a ThreadLocal we potentially leak memory in case of a). If we close immediately we potentially close way too early in case b).

[manovotn]

And it appears to me that the spec is broken.

What exactly is broken with it?

I think it was deliberately designed so that both, activate() and deactivate() calls can perform no-op under certain conditions.
You actually need activate() to be lenient because you cannot be sure some other framework didn't start req. context prior to your attempt. And because of this, you then need deactivate() to not crash on you if it wasn't you activating the context.

I don't really see an issue with that, especially the approach you showed under b) looks sounds to me.

A simple boolean flag does not help because of concurrency.

From the specification part you quoted, RequestContextController is scoped to a single thread, so if by concurrency you mean multiple threads, you'd need an activation/deactivation for each thread anyway. In that case an executor (or whatever is managing thread pool) could start/stop the context for any task it executes; in other words, I'd try to get the context activating logic out of purely user bussines code. But I am just guessing here because you didn't state any actual use case.

If we close immediately we potentially close way too early in case b).

Why would finally block be too early? I suppose I am missing the use case you are referring to, could you try to lay out the scenario in which current RequestContextController doesn't work for you?

[Ladicek]

I assume the problem with case b) is that you can potentially call activate/deactivate on the same RCC multiple times. That is solved by using a flag, similarly to case a).

boolean activated = rcc.activate();
try {
    ...
} finally {
    if (activated) {
        rcc.deactivate();
    }
}

That is, IIUC, the recommended way to use RCC.

[struberg]

@Ladicek there are 2 recommended ways. What you wrote is the option a) in my original ticket. But there is also usage option b.) with the finally block. My problem is that there is no clear rule that activate/deactivate have to be called symmetrically or not. Which means either we close to early or miss a cleanup. That problem does not appear when using the interceptor, but only if using the RequestContextController manually.

I think I have an idea how to implement it different than I originally did. It will solve a few more problems, but still not all.

[manovotn]

there are 2 recommended ways.

Recommended where? The spec doc doesn't show this, nor does the javadoc of RCC.
I am not against those approaches, just stating that CDI spec actually doesn't "recommend" any way whatsoever.

[Ladicek]

I'd say that even the try/finally way of using RCC should be paired with a flag -- unless you're absolutely sure your method can't possibly be called recursively.

I believe that for the simple cases, it would be a lot better if we provided something like

// probably should have 2 variants: for Runnable and for Callable
rcc.withRequestContext(() -> {
    ...
});

but there still are the more complex cases where the request context must be activated in one method and deactivated in another one. Those should be hopefully rare, only in infrastructure code. People doing that should understand that, indeed, activation/deactivation must go in pair, otherwise lifecycle can't be guaranteed, memory leaks may happen and in general it's not a nice place to be at.

[struberg]

the original design proposed in the discussions was always with the boolean gotActivated flag. That's the reason the boolean return value was introduced in the first place. Otherwise you don't need it, isn't?

[manovotn]

I believe that for the simple cases, it would be a lot better if we provided something like

Most of these cases can likely be covered using @ActivateRequestContext interceptor.
RCC is designed to cover the cases where interceptor is not enough (e.g. where you need the context lifespan to be longer than a single intercepted method).

People doing that should understand that, indeed, activation/deactivation must go in pair, otherwise lifecycle can't be guaranteed,

+1
Also, it is hardly the only case where users make or break their CDI app (such as memory leaks with dependent beans obtained through dynamic lookup that users don't destroy).

That's the reason the boolean return value was introduced in the first place.

Yea, the spec is written so that the boolean lets you know whether you activated the context or not. Like Ladislav said, you could/should use that even in b) option.

[struberg]

Also, it is hardly the only case where users make or break their CDI app (such as memory leaks with dependent beans obtained through dynamic lookup that users don't destroy).

C'mon, that's not a serious answer, isn't? :)

Where in the spec does it say that it must get called symmetrically?
Reason I ask this is because I saw tons of usage of type a).

[manovotn]

C'mon, that's not a serious answer, isn't? :)

What I mean is that this is an API (and not the only one) that carries some potential risk of bad usage and requires user awareness. However, at the same time provides a functionality that justifies the risk. What's wrong with that?

Where in the spec does it say that it must get called symmetrically?

If this issue is all about adding "if you activate the context, you are responsible for deactivating it" into RCC, then sure, we can add that. Otherwise, I still don't see what you are getting at.

[rmannibucau]

+1 to fix this, it is just fully broken right now and there is no way to implement it due to conflicting activate/deactivate javadoc. I guess we should get a quick maintenance release (outside EE train) to enhance the controller deprecating these two method and making activate returning a Deactivation callback (a Runnable like) instead of a boolean:

Deactivation activate();

caller would then do

 final var deactivation = controller.activate();
 // ...
 deactivation.deactivate();

Not sure we can keep activate name or need another controller API but currently spec can not be implemented at the risk of thread safety or/and mem leaks (+ functional issues since predestroy are not called).

Indeed, at least fixing the javadoc is also needed by saying that calling deactiavte if activate returns false will not be a defined case (ie dont do it).

[Ladicek]

Where in the spec does it say that it must get called symmetrically?

It doesn't have to be -- but if it isn't, you get yourself into all kinds of troubles that follow from the fact that context objects for normal scopes are essentially global state. Which IMHO naturally follows from the fact that they are associated with threads, as the spec says.

I would totally be for adding some clarification to the javadoc, to improve user friendliness, but when it comes to normative text in the spec, IMHO, all the necessary provisions are already there.

enhance the controller deprecating these two method and making activate returning a Deactivation callback

If you don't call the deactivation callback, or call it too soon / too late, you have the exact same problem. So how does that improve the situation?

[rmannibucau]

@Ladicek no issue with the callback since nested calls will return a noop callback, this is how start/stop context are generally implemented in servers.

I'm not sure what you meant with "all the necessary provisions are already there" but since activate/deactivate javadoc are not consistent, the behavior is not so we'll break applications/usage by clarifying it so thought a new API can make it less ambiguous but if you think we should reuse the one we have and enforce deactivate to not be called when false is returned it can work too with some risk (and to be honest i'm not sure how low or high it is).

[Ladicek]

Your nested calls returning a noop callback directly correspond to current false return value. Calling deactivate only if activate returned true is perfectly safe. While I can see how the API you propose would be better than the current API (when it comes to safety; on the other hand, it would require more allocations), I can't see how this improvement warrants deprecating the current API and even doing a maintenance release.

Now, I'd really like to understand how the activate/deactivate javadoc are inconsistent. It may not be the best wording under the sun, but I think it's pretty clear that:

• activate will activate the request context only if it isn't already active (and returns true in such case; in all other cases, it returns false);
• deactivate will deactivate the request context only if it is active and if it was activated by the same instance of RCC (that is, if request context is active but was activated by a different RCC or through some other means, then deactivate is a noop); it will also unconditionally throw if the request context is not active.

I think that makes sense. I can't see a contradiction there, could you please point that out? Or is there a contradiction between the javadoc and the spec? I can't see that either.

(Also, I think someone mentioned thread safety in this thread. The spec explicitly says that RequestContextController shouldn't be called from multiple threads and non-portable behavior may occur in such case.)

[rmannibucau]

@Ladicek ok,

c.activate();
c.deactivate();

c.activate();
c.deactivate();
c.deactivate();

c.activate();
boolean enabled2 = c.activate();
if (enabled2) {
  c.deactivate();
}
c.deactivate();

Makes this working. Since the enable/disable calls are not always symmetric you can't have a counter or alike, since the calls can be symmetric you can't get a boolean (need to disable at last deactivate call), since there is no enclosing cleanup you can't cleanup lazily (this API works in SE where request context is not automatically cleaned up).
So if deactivate can skip deactivation it needs some data to do so.

What I envision is that the spec tend to say you must lookup a new instance each time you use it but it is a shame in terms of usage - requires to NOT use @Inject and to store controller instance - so I think spec should be refined to be consistent and usable by application - as used today, ie with inject.

[Ladicek]

I assume these are 3 different examples to be considered in isolation. Further, I assume that the request context is not active at the beginning of each example. Then:

// RC not active
c.activate(); // returns true
// RC active
c.deactivate();
// RC not active

// RC not active
c.activate(); // returns true
// RC active
c.deactivate();
// RC not active
c.deactivate(); // throws
// unreachable

// RC not active
c.activate(); // returns true
// RC active
boolean enabled2 = c.activate(); // returns false
// RC active
if (enabled2) {
  // unreachable, enabled2 is false
  c.deactivate();
}
// RC active
c.deactivate();
// RC not active

I don't quite see the problem here. RCC.activate only needs to check whether RC is active, and RCC.deactivate only needs to check whether RC is active && it was this RCC who activated it (for which a simple boolean is enough).

I guess I'm missing something, but I don't know what.

[struberg]

@Ladicek In reality the activation is nested over different independent classes which obviously are not required to know about each other.

[Ladicek]

Please provide a realistic example that exhibits problems, then :-)

[Ladicek]

I mean -- I believe that at this point, we conclusively established that RCC is possible to implement (contrary to this issue's title). We also established that there are code patterns that make RCC usage safe.

We're just debating problematic scenarios that stem from incorrect usage of RCC. That incorrectness stems from the nature of global state and has nothing to do with the CDI specification.

There are certainly ways how to improve the RCC API to make correct RCC usage easier. If we want to debate that, sure -- I'm all for it.

[rmannibucau]

@Ladicek main issue is to use it right in 90% of applications, you must @inject Provider<RCC> rccFactory; instead of RCC directly which is not natural leading most usages to be wrong.

So guess minimum is to make the spec more explicit about the usage (with a sample maybe?) and we should think to a way to avoid the provider probably?

[struberg]

I can see how it can work. But it's not so obvious to users. I think it would be wise to add an example. Most people are probably not aware that they need to use a Provider<RequestContextController> to make it usable. Actually never seen it used that way till today. Otoh I would prefer to not introduce another way to achieve the same, thus the request for adding an example usage to the spec.

[arjantijms]

@struberg just wondering, is it still impossible to implement, or did you manage to do it in OWB 4?

[struberg]

Yes, it's mostly a documentation issue. But it's really tricky to do right. We should document this better. Search for the class on github and you'll find tons of wrong usage

There is a lot of different usage. And most are missing parts.
E.g.:
https://github.com/radcortez/smallrye-fault-tolerance/blob/main/implementation/fault-tolerance/src/main/java/io/smallrye/faulttolerance/internal/RequestScopeActivator.java

or

https://github.com/aseovic/helidon/blob/895695a902b4b843b61afa5e80c8280cec468946/microprofile/fault-tolerance/src/main/java/io/helidon/microprofile/faulttolerance/RequestScopeHelper.java#L100

It's really a pitty, even people really into CDI fall into this trap.

The problem is that correct usage requires to use BOTH the flag AND finally. Without the finally block any Exception will leave the RequestContext open. And without the flag all nested usage is broken.
In most sources I've checked one of them is missing.

So to be more clear: this never was a real implementation issue, but the API design and description did imo imply that no special care is needed. But that's misleading, and we should point this out in the spec. CC-ing @manovotn for improving this in the next spec iteration.

[Ladicek]

The RequestScopeActivator in SmallRye Fault Tolerance is actually correct, because:

• it cannot be called recursively
• the RCC is obtained via CDI.current().select(RequestContextController.class).get() in a constructor of an interceptor, which is @Dependent, so no sharing occurs (it is actually a bit more complex due to MP ConProp integration, which is why we cannot @Inject the RCC directly, but I won't go into detail here)

It is true that using the flag (return value of activate()) would be better, but as far as I can tell, that is not required in this particular case.

Anyway, thanks for confirming that there are no implementation issues. I'll leave this issue open for a few more days and then close if noone objects.

[Azquelt]

I think we should update the example in the Javadoc to include checking the return value from activate and using a finally block before closing the issue.

[Ladicek]

Agreed.

[struberg]

• he RCC is obtained via CDI.current().select(RequestContextController.class).get() in a constructor of an interceptor, which is @Dependent, so no sharing occurs

Sorry but isn't that totally unrelated? The real question is whether the state of the RequestContext is shared. And that SURELY is the case on the very same thread. Regardless how many context controller instances there are - they all control the very same RequestContext. So if you have multiple cases where the RequestContext gets activated in this very thread (even downstream in userland code) you will end up with broken state.
The new sample code looks correct - txs!

[Ladicek]

Regardless how many context controller instances there are - they all control the very same RequestContext.

That is correct.

So if you have multiple cases where the RequestContext gets activated in this very thread (even downstream in userland code) you will end up with broken state.

But that is not.

If an RCC didn't activate the request context -- because it was already active --, calling deactivate() is a noop. An implementation of RCC.activate() does not only have to return whether it activated the request context -- it also has to store that information internally, for the corresponding deactivate() call.

[struberg]

That would mean that the RCC needs an additional ThreadLocal with this information. As the RCC could be used in concurrent situations. And then what happens to the ThreadLocal if the TCC just gets thrown away? This just cries 'memleak' quit loudly and there is no finally block to prevent it.

[rmannibucau]

@struberg not another one cause you can use the same but another state yes.
If we all agree this API is not friendly and its design is wrong we can keep it this way, deprecate it for next release and replace it with a proper API (implementing autocloseable sounds neat for several use cases - all thread based ones - and solving this case too)? Can also maybe open the door in terms of pattern to make it scope friendly for virtual threads