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.