Quarkus: Calling method on target bean from interceptor invokes interceptor again; leads to StackOverflowException

[38leinaD]

We have an aroundInvoke interceptor defined for our beans to do some application-specific permission checks:

@MyPermissionCheck
@Interceptor
@Priority(Interceptor.Priority.APPLICATION)
public class PermissionInterceptor implements Serializable {
    @AroundInvoke
    public Object beforeInvokation(InvocationContext invocationContext) throws Exception {
        BaseBean bean = (BaseBean) invocationContext.getTarget();
        boolean allowed = bean.performCheck(); // Leads to StackOverflowException
        if (allowed) {
            return invocationContext.proceed();
        }
        else {
            throw new RuntimeException("not allowed);
        }
    }
}

@MyPermissionCheck
@Named
@ViewScoped
public class TraceLogSettingsBean extends BaseBean {
}

When this interceptor is called for our bean, it internally, calls a performCheck-method on the bean by getting a bean reference via invocationContext.getTarget().
This bean reference is the Arc proxy and when performCheck is called, it will call the interceptor again and lead to a StackOverflowException.

Expected behavior
As per the handling in Weld and other CDI implementations, when the bean method is called from an interceptor, the interceptor should not be called again.

Actual behavior
StackOverflowException

Environment (please complete the following information):

• Quarkus version or git rev: 1.7.0.Final

[famod]

/cc @mkouba @manovotn

[manovotn]

I can see how this is happening in ArC, but I don't know how to prevent it yet. Every intercepted bean implements a subclass which adds interception logic to methods; what I am not seeing is how can we tell if an invocation of such method was made within an intercepted call.
Maybe @mkouba has some ideas?

[famod]

Maybe invocationContext.getContextData() could be utilized to work around this on the user side. The idea is:

• PermissionInterceptor checks if a flag already exists in the map
• if exists: don't call bean.performCheck()
• if not exists: add the flag and call bean.performCheck()

Btw, @38leinaD could it be that your example interceptor is missing invocationContext.proceed()?

[38leinaD]

@famod
You are right, the example misses the invocationContext.proceed(). I update the example to show the full scenario.

I am not sure i understand how to solve this from the user-side only. For the "if not exists" branch in your explanation, you say "call bean.performCheck()". So, here i will encounter the said problem again. Maybe i dont fully understand you proposed solution.

I actually think, the Arc implementation itself needs to keep some context to know if it is already within an interceptor-chain and based on that, call the bean method without calling the interceptors again.

[manovotn]

Another way to workaround this is to avoid marking performCheck() method for interception. E.g. placing your interceptor binding (@MyPermissionCheck) directly on methods instead of the bean class. OFC this only works so long as you don't need that method intercepted for some other calls.

[38leinaD]

Another way to workaround this is to avoid marking performCheck() method for interception. E.g. placing your interceptor binding (@MyPermissionCheck) directly on methods instead of the bean class. OFC this only works so long as you don't need that method intercepted for some other calls.

Yes, understand what you mean. But as this is a generic mechanism that should intercept all calls, this is not an option in our case. Actually, it would be an option to explicitly exclude a certain method from interception (here: performCheck); but i am unaware that this possible in a JavaEE standard way. I can either annotate the whole class or each method, but i cannot annotate the class and somehow exclude certain methods by writing some annotation on that method !?

[famod]

I am not sure i understand how to solve this from the user-side only. For the "if not exists" branch in your explanation, you say "call bean.performCheck()". So, here i will encounter the said problem again. Maybe i dont fully understand you proposed solution.

In theory, your interceptor _will_ be called again but will then find the flag in the map and will _not_ call performCheck() again.

[manovotn]

@38leinaD the first workaround suggestion by @famod should work as well - the context data can be used to stored anything during the whole interceptor chain. So you could have a flag saying that you already performed bean.performCheck() within this chain and you'd not invoke it again. Something like this:

        @AroundInvoke
        public Object mySuperCoolAroundInvoke(InvocationContext ctx) throws Exception {
            Map<String, Object> contextData = ctx.getContextData();
            if (contextData.containsKey("my_key")) {
                // within this interceptor chain, the method was already called, no need to do it again
                ctx.proceed();
            } else {
                // the check wasn't invoked yet; set a flag and perform it
                contextData.put("my_key", true);
                BaseBean bean = (BaseBean) ctx.getTarget();
                boolean allowed = bean.performCheck(); // Leads to StackOverflowException
                if (allowed) {
                    return ctx.proceed();
                }
                else {
                    throw new RuntimeException("not allowed);
                }
            }
        }

[38leinaD]

Sounds like i viable solution/workaround in general if this is a JEE standard-compliant approach; and it looks like it!

But it does not work currently. I tried, like you described but on each self-invokation of the interceptor, the map seems to be reset and the entry i put in before is gone. In fact, the map always only contains this key: io.quarkus.arc.interceptorBindings. my_key is gone.

[manovotn]

Ah, I see. Just tried it locally. It won't work because each of those method invocations triggers a separate interceptor chain (meaning each of them has their own context data therefore you cannot see the flag you store there). Context data are only shared within one interceptor chain, you would use that, for instance, to carry information between several of your interceptors within one chain. Sadly it won't help here.

As a side note, the key io.quarkus.arc.interceptorBindings is automatically added to all of them.

So you are down to the other workaround I described above.

[38leinaD]

Hm, as I already described above, the other workaround is not an option for us as this is a generic mechanism in our application that does some security checks and it cannot be left to the person implementing the bean to add the annotation to each method that needs it.

What is the argument against adopting the behavior of Weld and other CDI implementations? I understand that Arc is not fully CDI compliant and, to be honest, I am not sure this is mandated by the spec or an implementation detail, but I would hope that, where possible, Arc behaves similar to the popular CDI implementations. Otherwise, people migrating from JEE / CDI will face migration issues like these.

[manovotn]

What is the argument against adopting the behavior of Weld and other CDI implementations?

None at all. This needs to be fixed. We were just suggesting temporary workarounds for you in the meantime :-)

[famod]

Too bad the context map doesn't work here. What about unwrapping the proxy (io.quarkus.arc.ClientProxy)?
PS: I know such things should be avoided whenever possible.

[38leinaD]

In the best case, I would like a workaround where the interceptor can run on a regular JEE appserver as well as on Quarkus.
I was thinking to use a ThreadLocal variable to keep track if the interceptors has already been call (using a ThreadLocal instead of the InvocationContext.getContext()). This should be safe as the interceptors are always called on the same thread, right?

[mkouba]

What about unwrapping the proxy (io.quarkus.arc.ClientProxy)?

@famod That would not help in this case. The instance returned by InvocationContext#getTarget() is not a client proxy but an intercepted subclass (internal container construct as defined by Contextual instance of a bean).

I believe that this behavior is not defined by the spec but I've found a very old TCK test that seems to test the invocation of an instance returned from InvocationContext#getTarget():
https://github.com/eclipse-ee4j/cdi-tck/blob/2.0/impl/src/main/java/org/jboss/cdi/tck/interceptors/tests/contract/invocationContext/Interceptor1.java#L28

A threadLocal-based workaround should work. The problem is that if we decide to "fix" it we would have to find another solution for "self invocation interception" which we currently support (although it's another open question/undefined behavior, see for example https://issues.redhat.com/browse/CDI-414).

[manovotn]

"self invocation interception" which we currently support

I don't think we state this anywhere officially and due to it being a grey area, I don't think we are bound to keep it that way.

Also, just for any onlookers, here is a comparison of Weld to ArC in these two aspects:

• Weld - self invocations are not intercepted && invocation of intercepted method via InvocationContext#getTarget() does not trigger interception
• ArC - self invocations are intercepted && invocation of intercepted method via InvocationContext#getTarget() triggers interception

Also note that Weld has a pretty huge construct built around interceptor invocations to make this work. Some relevant bits are CombinedInterceptorAndDecoratorStackMethodHandler or InterceptionDecorationContext. All in all it is a beast that I am not sure we want to bring over to ArC.