Bug 54716 - WebSocket implementation "freaks out" if Session closed or exception thrown from onOpen method
Summary: WebSocket implementation "freaks out" if Session closed or exception thrown f...
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 8
Classification: Unclassified
Component: Catalina (show other bugs)
Version: 8.0.x-trunk
Hardware: All All
: P2 major (vote)
Target Milestone: ----
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-18 05:48 UTC by Nick Williams
Modified: 2013-03-19 19:59 UTC (History)
0 users



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Williams 2013-03-18 05:48:12 UTC
If Session#close() is called from an onOpen method, the container throws a NullPointerException, as evidenced below:

Mar 17, 2013 10:57:50 PM org.apache.coyote.AbstractProtocol$AbstractConnectionHandler process
SEVERE: Error reading request, ignored
java.lang.IllegalArgumentException: Failed to call onOpen method of POJO end point for POJO of type [com.wrox.chat.ChatEndpoint]
	at org.apache.tomcat.websocket.pojo.PojoEndpointBase.doOnOpen(PojoEndpointBase.java:54)
	at org.apache.tomcat.websocket.pojo.PojoEndpointServer.onOpen(PojoEndpointServer.java:69)
	at org.apache.tomcat.websocket.server.WsProtocolHandler.init(WsProtocolHandler.java:107)
	at org.apache.coyote.http11.upgrade.AbstractProcessor.<init>(AbstractProcessor.java:51)
	at org.apache.coyote.http11.upgrade.NioProcessor.<init>(NioProcessor.java:31)
	at org.apache.coyote.http11.Http11NioProtocol$Http11ConnectionHandler.createUpgradeProcessor(Http11NioProtocol.java:309)
	at org.apache.coyote.AbstractProtocol$AbstractConnectionHandler.process(AbstractProtocol.java:634)
	at org.apache.coyote.http11.Http11NioProtocol$Http11ConnectionHandler.process(Http11NioProtocol.java:223)
	at org.apache.tomcat.util.net.NioEndpoint$SocketProcessor.doRun(NioEndpoint.java:1581)
	at org.apache.tomcat.util.net.NioEndpoint$SocketProcessor.run(NioEndpoint.java:1540)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1110)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:603)
	at java.lang.Thread.run(Thread.java:722)
Caused by: java.lang.reflect.InvocationTargetException
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:487)
	at org.apache.tomcat.websocket.pojo.PojoEndpointBase.doOnOpen(PojoEndpointBase.java:51)
	... 12 more
Caused by: java.lang.NullPointerException
	at org.apache.tomcat.websocket.BackgroundProcessManager.unregister(BackgroundProcessManager.java:75)
	at org.apache.tomcat.websocket.WsWebSocketContainer.unregisterSession(WsWebSocketContainer.java:283)
	at org.apache.tomcat.websocket.server.WsServerContainer.unregisterSession(WsServerContainer.java:283)
	at org.apache.tomcat.websocket.WsSession.sendCloseMessage(WsSession.java:357)
	at org.apache.tomcat.websocket.WsSession.close(WsSession.java:305)
	at com.wrox.chat.ChatEndpoint.onOpen(ChatEndpoint.java:54)
	... 17 more

If this or any other exceptions are thrown from an onOpen method, even if the session isn't closed, Tomcat starts randomly rejecting a handful of requests with no explanation in the logs, and on shut down dozens of these errors appear on stderr:

Mar 17, 2013 11:22:11 PM org.apache.tomcat.util.modeler.Registry unregisterComponent
SEVERE: Error unregistering mbean 
javax.management.RuntimeOperationsException: Object name cannot be null
	at com.sun.jmx.interceptor.DefaultMBeanServerInterceptor.isRegistered(DefaultMBeanServerInterceptor.java:569)
	at com.sun.jmx.mbeanserver.JmxMBeanServer.isRegistered(JmxMBeanServer.java:628)
	at org.apache.tomcat.util.modeler.Registry.unregisterComponent(Registry.java:420)
	at org.apache.coyote.AbstractProtocol$AbstractConnectionHandler.unregister(AbstractProtocol.java:768)
	at org.apache.coyote.AbstractProtocol$RecycledProcessors.clear(AbstractProtocol.java:820)
	at org.apache.coyote.AbstractProtocol$AbstractConnectionHandler.recycle(AbstractProtocol.java:578)
	at org.apache.tomcat.util.net.NioEndpoint.releaseCaches(NioEndpoint.java:318)
	at org.apache.tomcat.util.net.NioEndpoint.unbind(NioEndpoint.java:481)
	at org.apache.tomcat.util.net.AbstractEndpoint.destroy(AbstractEndpoint.java:675)
	at org.apache.coyote.AbstractProtocol.destroy(AbstractProtocol.java:522)
	at org.apache.catalina.connector.Connector.destroyInternal(Connector.java:1020)
	at org.apache.catalina.util.LifecycleBase.destroy(LifecycleBase.java:305)
	at org.apache.catalina.core.StandardService.destroyInternal(StandardService.java:584)
	at org.apache.catalina.util.LifecycleBase.destroy(LifecycleBase.java:305)
	at org.apache.catalina.core.StandardServer.destroyInternal(StandardServer.java:835)
	at org.apache.catalina.util.LifecycleBase.destroy(LifecycleBase.java:305)
	at org.apache.catalina.startup.Catalina.stop(Catalina.java:719)
	at org.apache.catalina.startup.Catalina.start(Catalina.java:680)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:487)
	at org.apache.catalina.startup.Bootstrap.start(Bootstrap.java:357)
	at org.apache.catalina.startup.Bootstrap.main(Bootstrap.java:491)
Caused by: java.lang.IllegalArgumentException: Object name cannot be null
	... 24 more
Comment 1 Mark Thomas 2013-03-19 14:23:14 UTC
Thanks for the report. Should be fixed now. I made onError and onClose handling more robust too. There might still be a way to breaks things. Feel free to re-open if you find one.
Comment 2 Nick Williams 2013-03-19 15:30:13 UTC
This definitely improves robustness, but it does not solve the problem where a NullPointerException is thrown in BackgroundProcessManeger#unregister(...) if a Session#close(...) is called in an onOpen method:

Caused by: java.lang.NullPointerException
	at org.apache.tomcat.websocket.BackgroundProcessManager.unregister(BackgroundProcessManager.java:75)
	at org.apache.tomcat.websocket.WsWebSocketContainer.unregisterSession(WsWebSocketContainer.java:283)
	at org.apache.tomcat.websocket.server.WsServerContainer.unregisterSession(WsServerContainer.java:283)
	at org.apache.tomcat.websocket.WsSession.sendCloseMessage(WsSession.java:357)
	at org.apache.tomcat.websocket.WsSession.close(WsSession.java:305)
	at com.wrox.chat.ChatEndpoint.onOpen(ChatEndpoint.java:54)
	... 17 more

Calling Session#close(...) from onOpen is a valid use-case: At onOpen I may check to make sure the user is authenticated, and if not, I need to close the session.

The root-cause of this NullPointerException is that, in WsWebSocketContainer and WsProtocolHandler, WsWebSocketContainer#registerSession(...) is called AFTER onOpen. registerSession ultimately calls BackgroundProcessManager#register(...), which instantiates the background thread if it does not already exist. So, if there are NO active Sessions, and a new Session is created, and that Session is closed onOpen, the background thread will not already exist when BackgroundProcessManeger#unregister(...) is called, and that causes the NullPointerException.

There are two possible solutions to this that I can see, though there very well may be others:

1) In BackgroundProcessManeger#unregister(...), make sure the background thread is not null before calling #halt().
2) In WsWebSocketContainer and WsProtocolHandler, call registerSession BEFORE onOpen. This doesn't seem correct to me, but I don't understand it fully.
Comment 3 Nick Williams 2013-03-19 15:35:29 UTC
Assuming the solution is 1, the patch is simply changing this:

            if (processes.size() == 0) {
                wsBackgroundThread.halt();
                wsBackgroundThread = null;
            }

To this:

            if (wsBackgroundThread != null && processes.size() == 0) {
                wsBackgroundThread.halt();
                wsBackgroundThread = null;
            }
Comment 4 Mark Thomas 2013-03-19 19:59:06 UTC
The solution is 1) plus an additional change so that sessions closed during onOpen are not registered.