Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 510553

Summary: Add possibility to stop a ServerPushSession even if there are still Runnables to process
Product: [RT] RAP Reporter: Wolfgang Pedot <wolfgang.pedot>
Component: RWTAssignee: Project Inbox <rap-inbox>
Status: RESOLVED FIXED QA Contact:
Severity: enhancement    
Priority: P3 CC: ivan
Version: 3.1   
Target Milestone: 3.2 M6   
Hardware: PC   
OS: Windows NT   
Whiteboard:
Attachments:
Description Flags
Full DemoWorkbench class used in test none

Description Wolfgang Pedot CLA 2017-01-17 05:10:29 EST
I spent a few hours hunting frequent exceptions (uiSession parameter must not be null) after workbench shutdown yesterday. Our application uses a ServerPushSession full-time (started before creation of workbench and stopped after PlatformUI.createAndRunWorkbench returns) and what I have found out is that ServerPushSession.stop does not always do what it is supposed to do:

In our code it can happen that a runnable is queued in the display during workbench shutdown (triggered when disposing our WorkbenchWindowAdvisor), if thats the case ServerPushRenderer will not render deactivation because there are still pending runnables. The problem is now that the shutdown completes without ServerPush ever deactivating. Since we redirect the browser after workbench-shutdown that leads to said exception almost every time, if I disable that redirect the browser keeps making ServerPush-requests at a very high rate because there is nothing blocking it on the server-side. The runnable we queue is not executed in both cases.

I have now worked around this by not queueing that runnable under those conditions (its not needed then anyway) but it would be nice to have something like ServerPushSession.destroy to reliably stop ServerPush on shutdown even if there are still runnables or better yet ignore that there are still runnables to process if ServerPushSession is stopped during shutdown.

Any ideas on this?
Comment 1 Ivan Furnadjiev CLA 2017-01-18 04:23:02 EST
Wolfgang, can you provide the stack trace of the exception? Is it possible to create a minimal RAP application to reproduce the exception?
Comment 2 Ivan Furnadjiev CLA 2017-01-18 04:26:37 EST
Is the stack trace similar:
java.lang.NullPointerException
	at org.eclipse.rap.rwt.internal.serverpush.ServerPushManager.isSessionExpired(ServerPushManager.java:192)
	at org.eclipse.rap.rwt.internal.serverpush.ServerPushManager.isSessionExpired(ServerPushManager.java:187)
	at org.eclipse.rap.rwt.internal.serverpush.ServerPushManager.canReleaseBlockedRequest(ServerPushManager.java:156)
	at org.eclipse.rap.rwt.internal.serverpush.ServerPushManager.processRequest(ServerPushManager.java:140)
	at org.eclipse.rap.rwt.internal.serverpush.ServerPushServiceHandler.service(ServerPushServiceHandler.java:31)
	at org.eclipse.rap.rwt.engine.RWTServlet.handleValidRequest(RWTServlet.java:135)
	at org.eclipse.rap.rwt.engine.RWTServlet.handleRequest(RWTServlet.java:117)
	at org.eclipse.rap.rwt.engine.RWTServlet.doGet(RWTServlet.java:100)
	at javax.servlet.http.HttpServlet.service(HttpServlet.java:687)
	at javax.servlet.http.HttpServlet.service(HttpServlet.java:790)
	at org.eclipse.rap.rwt.osgi.internal.CutOffContextPathWrapper.service(CutOffContextPathWrapper.java:106)
	at org.eclipse.equinox.http.servlet.internal.HttpServiceRuntimeImpl$LegacyServlet.service(HttpServiceRuntimeImpl.java:1222)
	at org.eclipse.equinox.http.servlet.internal.registration.EndpointRegistration.service(EndpointRegistration.java:148)
	at org.eclipse.equinox.http.servlet.internal.servlet.ResponseStateHandler.processRequest(ResponseStateHandler.java:62)
	at org.eclipse.equinox.http.servlet.internal.context.DispatchTargets.doDispatch(DispatchTargets.java:131)
	at org.eclipse.equinox.http.servlet.internal.servlet.ProxyServlet.service(ProxyServlet.java:74)
	at javax.servlet.http.HttpServlet.service(HttpServlet.java:790)
	at org.eclipse.equinox.http.jetty.internal.HttpServerManager$InternalHttpServiceServlet.service(HttpServerManager.java:287)
	at org.eclipse.jetty.servlet.ServletHolder.handle(ServletHolder.java:845)
	at org.eclipse.jetty.servlet.ServletHandler.doHandle(ServletHandler.java:584)
	at org.eclipse.jetty.server.session.SessionHandler.doHandle(SessionHandler.java:224)
	at org.eclipse.jetty.server.handler.ContextHandler.doHandle(ContextHandler.java:1180)
	at org.eclipse.jetty.servlet.ServletHandler.doScope(ServletHandler.java:512)
	at org.eclipse.jetty.server.session.SessionHandler.doScope(SessionHandler.java:185)
	at org.eclipse.jetty.server.handler.ContextHandler.doScope(ContextHandler.java:1112)
	at org.eclipse.jetty.server.handler.ScopedHandler.handle(ScopedHandler.java:141)
	at org.eclipse.jetty.server.handler.HandlerWrapper.handle(HandlerWrapper.java:134)
	at org.eclipse.jetty.server.Server.handle(Server.java:534)
	at org.eclipse.jetty.server.HttpChannel.handle(HttpChannel.java:320)
	at org.eclipse.jetty.server.HttpConnection.onFillable(HttpConnection.java:251)
	at org.eclipse.jetty.io.AbstractConnection$ReadCallback.succeeded(AbstractConnection.java:273)
	at org.eclipse.jetty.io.FillInterest.fillable(FillInterest.java:95)
	at org.eclipse.jetty.io.SelectChannelEndPoint$2.run(SelectChannelEndPoint.java:93)
	at org.eclipse.jetty.util.thread.strategy.ExecuteProduceConsume.executeProduceConsume(ExecuteProduceConsume.java:303)
	at org.eclipse.jetty.util.thread.strategy.ExecuteProduceConsume.produceConsume(ExecuteProduceConsume.java:148)
	at org.eclipse.jetty.util.thread.strategy.ExecuteProduceConsume.run(ExecuteProduceConsume.java:136)
	at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:671)
	at org.eclipse.jetty.util.thread.QueuedThreadPool$2.run(QueuedThreadPool.java:589)
	at java.lang.Thread.run(Thread.java:745)
Comment 3 Wolfgang Pedot CLA 2017-01-18 08:14:06 EST
Sorry that I did not include this in the original post:


java.lang.NullPointerException: The parameter 'uiSession' must not be null.
        at org.eclipse.rap.rwt.internal.util.ParamCheck.notNull(ParamCheck.java:29)
        at org.eclipse.rap.rwt.SingletonUtil.getUniqueInstance(SingletonUtil.java:73)
        at org.eclipse.rap.rwt.SingletonUtil.getSessionInstance(SingletonUtil.java:58)
        at org.eclipse.rap.rwt.internal.serverpush.ServerPushManager.getInstance(ServerPushManager.java:54)
        at org.eclipse.rap.rwt.internal.serverpush.ServerPushServiceHandler.service(ServerPushServiceHandler.java:31)
        at org.eclipse.rap.rwt.engine.RWTServlet.handleValidRequest(RWTServlet.java:135)
        at org.eclipse.rap.rwt.engine.RWTServlet.handleRequest(RWTServlet.java:117)
        at org.eclipse.rap.rwt.engine.RWTServlet.doGet(RWTServlet.java:100)
        at javax.servlet.http.HttpServlet.service(HttpServlet.java:687)
        at javax.servlet.http.HttpServlet.service(HttpServlet.java:790)
        at org.eclipse.rap.rwt.osgi.internal.CutOffContextPathWrapper.service(CutOffContextPathWrapper.java:106)
        at org.eclipse.equinox.http.servlet.internal.HttpServiceRuntimeImpl$LegacyServlet.service(HttpServiceRuntimeImpl.java:1217)
        at org.eclipse.equinox.http.servlet.internal.registration.EndpointRegistration.service(EndpointRegistration.java:162)
        at org.eclipse.equinox.http.servlet.internal.servlet.ResponseStateHandler.processRequest(ResponseStateHandler.java:63)
        at org.eclipse.equinox.http.servlet.internal.context.DispatchTargets.doDispatch(DispatchTargets.java:98)
        at org.eclipse.equinox.http.servlet.internal.HttpServiceRuntimeImpl.doDispatch(HttpServiceRuntimeImpl.java:372)
        at org.eclipse.equinox.http.servlet.internal.servlet.ProxyServlet.service(ProxyServlet.java:70)
        at javax.servlet.http.HttpServlet.service(HttpServlet.java:790)
        at org.eclipse.equinox.http.jetty.internal.HttpServerManager$InternalHttpServiceServlet.service(HttpServerManager.java:379)
        at org.eclipse.jetty.servlet.ServletHolder.handle(ServletHolder.java:812)
        at org.eclipse.jetty.servlet.ServletHandler.doHandle(ServletHandler.java:587)
        at org.eclipse.jetty.server.session.SessionHandler.doHandle(SessionHandler.java:221)
        at org.eclipse.jetty.server.handler.ScopedHandler.handle(ScopedHandler.java:143)
        at org.eclipse.jetty.server.handler.HandlerWrapper.handle(HandlerWrapper.java:97)
        at at.finkzeit.zsw.server.jettycustomizer.internal.JettyStatistics.handle(JettyStatistics.java:137)
        at org.eclipse.jetty.server.handler.ContextHandler.doHandle(ContextHandler.java:1129)
        at org.eclipse.jetty.servlet.ServletHandler.doScope(ServletHandler.java:515)
        at org.eclipse.jetty.server.session.SessionHandler.doScope(SessionHandler.java:185)
        at org.eclipse.jetty.server.handler.ContextHandler.doScope(ContextHandler.java:1061)
        at org.eclipse.jetty.server.handler.ScopedHandler.handle(ScopedHandler.java:141)
        at org.eclipse.jetty.server.handler.HandlerWrapper.handle(HandlerWrapper.java:97)
        at org.eclipse.jetty.server.Server.handle(Server.java:499)
        at org.eclipse.jetty.server.HttpChannel.handle(HttpChannel.java:311)
        at org.eclipse.jetty.server.HttpConnection.onFillable(HttpConnection.java:257)
        at org.eclipse.jetty.io.AbstractConnection$2.run(AbstractConnection.java:544)
        at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:635)
        at org.eclipse.jetty.util.thread.QueuedThreadPool$3.run(QueuedThreadPool.java:555)
        at java.lang.Thread.run(Thread.java:745)
Comment 4 Ivan Furnadjiev CLA 2017-01-18 08:30:21 EST
The stack trace shows that you recieve a ServerPush request from the client after the UI session is terminated on the server. As a result, ServerPushManager singleton for this session can't be obtained. What I don't understand is how (why) the client sends this request. Could you provide a minimal application that demostrates the issue?
Comment 5 Wolfgang Pedot CLA 2017-01-18 09:26:13 EST
Like I wrote ServerPushRenderer never renders deactivation of ServerPush because when we call ServerPushSession.stop there is still a runnable in the queue and I dont see another attempt to call it.

The HTTP session gets terminated because of the redirect we do, if I disable that I see a very high rate of ServerPush-Requests because the client never stops sending them as it never receives the deactivation of ServerPush.

ServerPushManager.processRequest does effectively nothing because isCallBackRequestBlocked and mustBlockCallBackRequest both return false.

I am no heading into a meeting, maybe I can whip up a test afterwards.
Comment 6 Wolfgang Pedot CLA 2017-02-06 13:50:37 EST
After further investigation I have found out that the issue I mention here is only responsible for a relatively small amount of those exceptions.

It seems that a far greater portion comes from users just closing the browser-tab or navigating to another site without properly quitting the RAP application inside. Depending on the browser (FF does this a lot, Chrome and Edge appearantly not so much) I can produce this exception easily. Closing the tab leads to a direct shutdown of the uiSession and an immediate empty response. I have tried adding a static deactivation of ServerPush to LifeCycleServiceHandler.writeEmptyMessage and that seems to help but it does not really strike me as a very clean solution.

It appears Chrome usually cancels all requests immediately when navigating away or closing the tab but FF (51.0.1 tested) sends at least one more ServerPush GET-Request most of the time.

I have traced 324 of 355 exceptions in a sample-log to FF browsers, most of them version 51.

It seems proper termination of ServerPush can be quite tricky....
Comment 7 Ivan Furnadjiev CLA 2017-02-07 02:34:31 EST
Wolfgang, is it possiblr this issue to be related to this NG post -> https://www.eclipse.org/forums/index.php/t/1084101/?
Comment 8 Wolfgang Pedot CLA 2017-02-07 07:43:32 EST
(In reply to Ivan Furnadjiev from comment #7)
> Wolfgang, is it possiblr this issue to be related to this NG post ->
> https://www.eclipse.org/forums/index.php/t/1084101/?

I dont think so, I see the same errors when a ServerPush-GET request returns but I dont see why that should alter the behaviour of the browser. Setting a content-type of text/html in the ServerPush-responses got rid of the errors but does not do anything regarding this issue.

I see two points here:

1.) ServerPush may not be terminated even though it has been stopped during application shutdown. I´ll try and get a small example together this afternoon.

2.) ServerPush is never terminated when a shutdown request is received from the browser (for example via onUnload) because the browser is not expected to send anything after that. I have traced the network traffic with FF and Chrome when closing a tab and FF sends another GET-request for ServerPush after receiving the final empty answer for the shutdown-request. 

Both cases can lead to the same exception on the server-side and while one may regard this as mostly a cosmetic issue I dont like hundrets of exceptions per day in my logfiles....
Comment 9 Ivan Furnadjiev CLA 2017-02-07 08:47:44 EST
We can deactivate the server push on the client before the shutdown request is send. Add in Display#_onUnload:
---
_onUnload : function() {
    this._document.removeEventListener( "windowresize", this._onResize, this );
    this._document.removeEventListener( "keypress", this._onKeyPress, this );
    this._server.removeEventListener( "send", this._onSend, this );
    rwt.client.ServerPush.getInstance().setActive( false );
    this._sendShutdown();
},
---
This should stop the GET-request for ServerPush after the session is terminated. What do you think?
Comment 10 Wolfgang Pedot CLA 2017-02-07 10:44:45 EST
That would fix point 2 nicely.

As for 1.) I am having some troubles to reproduce the behaviour in the WorkbenchDemo, right now I only get the issue if I dont explicitly dispose the display. I am looking into that some more and will report once I have more information.
Comment 11 Wolfgang Pedot CLA 2017-02-07 12:04:34 EST
Its really strange (at least for me):

After further testing the (current) difference between my application and a patched demo is that in my application the ServerPushManager.FORCE_PUSH attribute is set in ServiceStore when ServerPushRenderer.render is called for the last time and that forces ServerPush to stay active. Since there are lots and lots of ServiceStore Objects created it is a little hard to track down the one the gets queried for that attribute when ServerPushRenderer.render is called for the last time.

Any ideas how to reproduce that?

Will continue tomorrow.
Comment 12 Wolfgang Pedot CLA 2017-02-07 12:32:14 EST
Ok, just found the difference:

By using this piece of code in DemoWorkbench.createUI:


ServerPushSession pushSession = new ServerPushSession();
pushSession.start();
int result = PlatformUI.createAndRunWorkbench( display, worbenchAdvisor );
display.asyncExec( new Runnable() {

  @Override
  public void run() {
    System.out.println( "RUNNING" );
  }
} );
pushSession.stop();


I can get that FORCE_PUSH attribute in the "right" ServiceStore. That means that ServerPushRenderer will not render a deactivation and the browser will go into a request-loop once you terminate the workbench. Navigating to another page will then produce the famous exception.

This is not exactly the issue I reported originally but it is another boolean that will prevent ServerPush.deactivation from being rendered (just like ServerPushManager.hasRunnables) and its easier to reproduce.

There are essentially 3 conditions that must be met to render ServerPush deactivation (ServerPushManager.isServerPushActive, ServerPushManager.hasRunnables and ServerPushManager.forceServerPushForPendingRunnables) and the application can only directly control one of them (ServerPushManager.isServerPushActive)
Comment 13 Wolfgang Pedot CLA 2017-02-08 03:58:54 EST
Created attachment 266720 [details]
Full DemoWorkbench class used in test
Comment 14 Wolfgang Pedot CLA 2017-02-08 04:11:37 EST
Comment on attachment 266720 [details]
Full DemoWorkbench class used in test

I have now attached the full DemoWorkbench.java I used.

While trying to reproduce the issue in the demo I found that the Display was not disposed by the code in my application but only died because of onUnload or HTTP session termination. I dont really know why this was the case, that particular code is quite old (originally written for RAP 1.2 and adapted all the way up to 3.1) so I guess this is some old oversight.  

The case that ServerPush does not render deactivation due to pending runnables does not seem to be reproducible any more after adding a call to display.dispose() because that flag is cleared on dispose.

The only one left is now the one reproducible with the file I attached. The test-code may seem a bit odd, in reality the path to display.asyncExec in my application is much more convolulted of course....
Comment 15 Ivan Furnadjiev CLA 2017-02-08 04:23:07 EST
(In reply to Wolfgang Pedot from comment #14)
> Comment on attachment 266720 [details]
> Full DemoWorkbench class used in test
> 
> I have now attached the full DemoWorkbench.java I used.
> 
> While trying to reproduce the issue in the demo I found that the Display was
> not disposed by the code in my application but only died because of onUnload
> or HTTP session termination. I dont really know why this was the case, that
> particular code is quite old (originally written for RAP 1.2 and adapted all
> the way up to 3.1) so I guess this is some old oversight.  
> 
> The case that ServerPush does not render deactivation due to pending
> runnables does not seem to be reproducible any more after adding a call to
> display.dispose() because that flag is cleared on dispose.
> 
> The only one left is now the one reproducible with the file I attached. The
> test-code may seem a bit odd, in reality the path to display.asyncExec in my
> application is much more convolulted of course....

Wolfgang, did you tried the proposed solution (Gerrit change attached to the bug)? Does it work for you?
Comment 16 Ivan Furnadjiev CLA 2017-02-08 04:25:00 EST
... I'm talking about this change: https://git.eclipse.org/r/#/c/90590/
Comment 17 Wolfgang Pedot CLA 2017-02-08 04:40:26 EST
(In reply to Ivan Furnadjiev from comment #16)
> ... I'm talking about this change: https://git.eclipse.org/r/#/c/90590/

Did I overlook that?
Looks very promising, that seems to fix the last point on my list.
Comment 18 Ivan Furnadjiev CLA 2017-02-08 05:06:39 EST
(In reply to Wolfgang Pedot from comment #17)
> (In reply to Ivan Furnadjiev from comment #16)
> > ... I'm talking about this change: https://git.eclipse.org/r/#/c/90590/
> 
> Did I overlook that?
> Looks very promising, that seems to fix the last point on my list.

Actually, this fix solves both issue - when display is disposed by shutdown request or by code the ServerPush on the client will be deactivated.
Comment 19 Wolfgang Pedot CLA 2017-02-08 05:25:20 EST
(In reply to Ivan Furnadjiev from comment #18)
> 
> Actually, this fix solves both issue - when display is disposed by shutdown
> request or by code the ServerPush on the client will be deactivated.

I dont think so, the client always receives an emtpy response when sending the shutdown-request. See LifeCycleServiceHandler.processUIRequest.
Comment 20 Ivan Furnadjiev CLA 2017-02-08 05:33:46 EST
Yes... correct. I'll put the client solution back.
Comment 21 Ivan Furnadjiev CLA 2017-02-09 06:21:23 EST
Fixed with change https://git.eclipse.org/r/#/c/90590/