Interceptor not always called before atmosphere endpoint invocation

[reda-alaoui]

Hello,

We have one interceptor which opens and closes thread local resources such as security context, jdbc connection and logging context.

We noticed that there is a few paths where @managedservice methods (@message and @disconnect) are not invoked through the configured interceptors.

We managed to correct almost all paths by replacing the default atmosphere executors to apply our interceptor behaviour.

Now, here is the stacktrace of the last noticed path - RealtimeEndpoint.disconnect is annotated @disconnect - where interceptors are not called before disconnect:

at MyRealtimeEndpoint.disconnect(RealtimeEndpoint.java:164) 
    at sun.reflect.GeneratedMethodAccessor1601.invoke(Unknown Source) 
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) 
    at java.lang.reflect.Method.invoke(Method.java:497) 
    at org.atmosphere.util.Utils.invoke(Utils.java:196) 
    at org.atmosphere.config.managed.ManagedAtmosphereHandler.invoke(ManagedAtmosphereHandler.java:335) 
    at org.atmosphere.config.managed.ManagedAtmosphereHandler.onStateChange(ManagedAtmosphereHandler.java:209) 
    at org.atmosphere.cpr.AsynchronousProcessor.invokeAtmosphereHandler(AsynchronousProcessor.java:514) 
    at org.atmosphere.cpr.AsynchronousProcessor.completeLifecycle(AsynchronousProcessor.java:463) 
    at org.atmosphere.cpr.AsynchronousProcessor.endRequest(AsynchronousProcessor.java:570) 
    at org.atmosphere.websocket.DefaultWebSocketProcessor.executeClose(DefaultWebSocketProcessor.java:677) 
    at org.atmosphere.websocket.DefaultWebSocketProcessor.close(DefaultWebSocketProcessor.java:625) 
    at org.atmosphere.container.JSR356Endpoint.onClose(JSR356Endpoint.java:269) 
    at org.apache.tomcat.websocket.WsSession.fireEndpointOnClose(WsSession.java:527) 
    at org.apache.tomcat.websocket.WsSession.onClose(WsSession.java:511) 
    at org.apache.tomcat.websocket.WsFrameBase.processDataControl(WsFrameBase.java:342) 
    at org.apache.tomcat.websocket.WsFrameBase.processData(WsFrameBase.java:284) 
    at org.apache.tomcat.websocket.WsFrameBase.processInputBuffer(WsFrameBase.java:130) 
    at org.apache.tomcat.websocket.server.WsFrameServer.onDataAvailable(WsFrameServer.java:60) 
    at org.apache.tomcat.websocket.server.WsHttpUpgradeHandler$WsReadListener.onDataAvailable(WsHttpUpgradeHandler.java:203) 
    at org.apache.coyote.http11.upgrade.AbstractServletInputStream.onDataAvailable(AbstractServletInputStream.java:198) 
    at org.apache.coyote.http11.upgrade.AbstractProcessor.upgradeDispatch(AbstractProcessor.java:96) 
    at org.apache.coyote.AbstractProtocol$AbstractConnectionHandler.process(AbstractProtocol.java:668) 
    at org.apache.tomcat.util.net.NioEndpoint$SocketProcessor.doRun(NioEndpoint.java:1500) 
    at org.apache.tomcat.util.net.NioEndpoint$SocketProcessor.run(NioEndpoint.java:1456) 
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142) 
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617) 
    at org.apache.tomcat.util.threads.TaskThread$WrappingRunnable.run(TaskThread.java:61) 
    at java.lang.Thread.run(Thread.java:745)

Should we consider the upper example as a bug?

Should we consider a bug the fact that any atmosphere endpoint method could be called without using the interceptors?
If not, is there an alternative provided by atmosphere to replicate the http filter behaviour and what is then the usecase of interceptors?

[jfarcand]

We noticed that there is a few paths where @ManagedService methods (@Message and @Disconnect) are not invoked through the configured interceptors.

@reda-alaoui Can you elaborate on that, and more important, can you do pull request with your fixes?

Should we consider a bug the fact that any atmosphere endpoint method could be called without using the interceptors? 

Interceptors will be called when a the close message protocol is sent by the client, but won't be called, as you noticed, for hard close. Yes that could be considered a bug, but nobody ever asked for such feature hence I wound't enable it by default if I had to implement it.

My proposal is fix it (just call the method in DefaultWebSocketProcessor) and make it configurable using a property. Do a pull request and we will review and integrate it.

[reda-alaoui]

@jfarcand, we found 3 'incorrect' paths.

2nd path

at MyRealtimeEndpoint.message(MyRealtimeEndpoint.java:114) 
    at sun.reflect.GeneratedMethodAccessor9997.invoke(Unknown Source) 
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) 
    at java.lang.reflect.Method.invoke(Method.java:498) 
    at org.atmosphere.config.managed.Invoker.invokeMethod(Invoker.java:49) 
    at org.atmosphere.config.managed.ManagedAtmosphereHandler.message(ManagedAtmosphereHandler.java:366) 
    at org.atmosphere.config.managed.ManagedAtmosphereHandler.onStateChange(ManagedAtmosphereHandler.java:233) 
    at org.atmosphere.cpr.DefaultBroadcaster.invokeOnStateChange(DefaultBroadcaster.java:1047) 
    at org.atmosphere.cpr.DefaultBroadcaster$WriteOperation.call(DefaultBroadcaster.java:1088) 
    at org.atmosphere.cpr.DefaultBroadcaster.prepareInvokeOnStateChange(DefaultBroadcaster.java:1062) 
    at org.atmosphere.cpr.DefaultBroadcaster.executeAsyncWrite(DefaultBroadcaster.java:872) 
    at org.atmosphere.cpr.DefaultBroadcaster$2.run(DefaultBroadcaster.java:474) 
    at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511) 
    at java.util.concurrent.FutureTask.run(FutureTask.java:266) 
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142) 
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617) 
    at java.lang.Thread.run(Thread.java:745)

We corrected this one by wrapping the default atmosphere thread executors.

3rd path

I don't have the stack trace now, but it originates from IdleInterceptor.idleResources at websocket.close():

WebSocket webSocket = AtmosphereResourceImpl.class.cast(r).webSocket();
                        if (webSocket != null) {
                            webSocket.close();
                        } else {
                            AsynchronousProcessor.class.cast(config.framework().getAsyncSupport()).endRequest(AtmosphereResourceImpl.class.cast(r), true);
                        }

The websocket.close() triggers in the same thread the @disconnect handler method without using any interceptor.
What we did to correct this behaviour is replacing the IdleInterceptor with our own extended IdleInterceptor which wraps the IdleInterceptor.idleResources() call.

Do you consider those 2 paths as eligible to interception?

[reda-alaoui]

The 3rd path could be corrected by readjusting the order of interceptors call. I don't know if we are able to add easily interceptors to be called before default ones.

[jfarcand]

@reda-alaoui I'm not convinced interceptor needs to be called on every scenario you describe, specially when you use the Broadcaster. That will kill the performance

For the order, you can always set the priority using the InvokationOrder.

Like I said if you can submit a PR with an option to enable what you want, I've no problem reviewing it and integrating it.

[reda-alaoui]

@jfarcand, is it normal that when firing the broadcaster from the server, the server endpoint itself receives the broadcast?
I looks strange to me. You can see it in the stack trace of the 2nd path.

I will submit you a PR, but before that I want to be sure to understand the system.

[jfarcand]

@reda-alaoui There is an issue on that where you broadcast the same object that maps against your resource (search issue)

[reda-alaoui]

Is it #2051 ?

[jfarcand]

@reda-alaoui yes

[reda-alaoui]

I think it is more about #1620