Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Filter/Servlet/Listener Holders are not started if added during STARTING state. #5378

Closed
sbordet opened this issue Oct 3, 2020 · 12 comments · Fixed by #5397
Closed

Filter/Servlet/Listener Holders are not started if added during STARTING state. #5378

sbordet opened this issue Oct 3, 2020 · 12 comments · Fixed by #5397
Assignees
Labels
Bug For general bugs on Jetty side

Comments

@sbordet
Copy link
Contributor

sbordet commented Oct 3, 2020

Jetty version
10.0.0.beta2

Description
See https://jenkins.webtide.net/job/cometd/job/6.0.x/57/

Apparently it's a NPE due to the latest changes to FilterHolder in #5032 - a request can arrive while the filter has been stopped already.

@sbordet
Copy link
Contributor Author

sbordet commented Oct 3, 2020

@gregw rings a bell?

@joakime
Copy link
Contributor

joakime commented Oct 3, 2020

Could be.
In a discussion with @gregw we changed the destroy order in the FilterHolder.destroyInstance(Object o) to be the same as it's done in the ServletHolder.destroyInstance(Object o)

Can you reproduce this NPE easily enough?
We can switch it back temporarily.

@gregw
Copy link
Contributor

gregw commented Oct 5, 2020

@sbordet the changes to the filter were merged to 9.4, so any idea why you only get this problem on 10?
A filter should not be able to be stopped while requests are received.

@gregw
Copy link
Contributor

gregw commented Oct 5, 2020

Also do you have an idea if this is failing during start up (ie first request) or shutdown (after stop)?

@gregw
Copy link
Contributor

gregw commented Oct 5, 2020

I'm looking at FilterHolder prior to #5032 and there is nothing there to stop a NPE if the holder is stopped while a request is in transit. The last isStarted() check is in ServletHandler.handle (actually because of scoped handler it could have been in ContextHandler.handle), so there is definitely scope for the handler to be stopped in parallel to a request in transit. But this has always been the case.
We probably should have a null check here to protect against that, but not sure why this is suddenly a problem? More info needed

@sbordet
Copy link
Contributor Author

sbordet commented Oct 5, 2020

@gregw @lachlan-roberts this is what I get after Server.start() after calling JettyWebSocketServletContainerInitializer.configure(context, null) which I think it's the right way to setup Jetty WS embedded in 10:

+= o.e.j.s.ServletContextHandler@6492fab5{/,null,AVAILABLE} - STARTED
|  += org.eclipse.jetty.server.session.SessionHandler15094126==dftMaxIdleSec=-1 - STARTED
|  |  += ServletHandler@5f7f2382{STARTED} - STARTED
|  |  |  +> listeners ServletHandler@5f7f2382{STARTED} size=0
|  |  |  +> filters ServletHandler@5f7f2382{STARTED} size=1
|  |  |  |  +> WebSocketUpgradeFilter@5707c1cb==org.eclipse.jetty.websocket.util.server.WebSocketUpgradeFilter,inst=false,async=true - STOPPED
|  |  |  |     +> class org.eclipse.jetty.websocket.util.server.WebSocketUpgradeFilter
|  |  |  |     +> initParams size=1
|  |  |  |        +> org.eclipse.jetty.websocket.util.server.internal.WebSocketMapping.key=org.eclipse.jetty.websocket.util.server.internal.WebSocketMapping
|  |  |  +> filterMappings ServletHandler@5f7f2382{STARTED} size=1
|  |  |  |  +> [/*]/[]/[REQUEST]=>WebSocketUpgradeFilter
|  |  |  +> servlets ServletHandler@5f7f2382{STARTED} size=2
|  |  |  |  +> org.cometd.server.CometDServlet-70ab2d48@87b85f27==org.cometd.server.CometDServlet,jsp=null,order=1,inst=true,async=true - STARTED
|  |  |  |  |  +> org.cometd.server.CometDServlet@28a2a3e7
|  |  |  |  |  +> initParams size=3
|  |  |  |  |     +> transports=org.cometd.server.websocket.jetty.JettyWebSocketTransport,org.cometd.server.http.JSONTransport
|  |  |  |  |     +> timeout=10000
|  |  |  |  |     +> ws.cometdURLMapping=/cometd/*
|  |  |  |  +> org.eclipse.jetty.servlet.ServletHandler$Default404Servlet-578524c3@9ecf9925==org.eclipse.jetty.servlet.ServletHandler$Default404Servlet,jsp=null,order=-1,inst=false,async=true - STARTED
|  |  |  |     +> class org.eclipse.jetty.servlet.ServletHandler$Default404Servlet
|  |  |  +> servletMappings ServletHandler@5f7f2382{STARTED} size=2
|  |  |     +> [/cometd/*]=>org.cometd.server.CometDServlet-70ab2d48
|  |  |     +> [/]=>org.eclipse.jetty.servlet.ServletHandler$Default404Servlet-578524c3
|  |  += org.eclipse.jetty.server.session.DefaultSessionCache@3f2049b6[evict=-1,removeUnloadable=false,saveOnCreate=false,saveOnInactiveEvict=false] - STARTED
|  |  |  += org.eclipse.jetty.server.session.NullSessionDataStore@10b3df93[passivating=false,graceSec=3600] - STARTED
|  |  +~ DefaultSessionIdManager@62da83ed{STARTED}[worker=node0] - STARTED
|  +- org.eclipse.jetty.servlet.listener.ContainerInitializer$ServletContainerInitializerServletContextListener@3dddefd8
|  += JettyServerFrameHandlerFactory@60094a13{STARTED} - STARTED
|  |  +> java.util.concurrent.ConcurrentHashMap@0{size=0}
|  += JettyWebSocketServerContainer@5aceec94{STARTED} - STARTED
|  |  += SessionTracker@ea27e34{STARTED} - STARTED
|  |     +> java.util.Collections$SetFromMap@0(size=0)

At line 6, the WSUF is STOPPED, while all the rest is STARTED.

gregw added a commit that referenced this issue Oct 5, 2020
@gregw
Copy link
Contributor

gregw commented Oct 5, 2020

I've added some tests in 6d69e31 but cannot reproduce.
The added holder should be started by the initialize method in updateMappings or ServletContextHandler doStart???

@gregw
Copy link
Contributor

gregw commented Oct 5, 2020

The test for calling initialize is isStarted() rather than isRunning(), so if you are adding these filters after the ServletContextHandler would have started them, but before it is actually started, there could be a window.

@gregw
Copy link
Contributor

gregw commented Oct 5, 2020

@sbordet turns out we can use our API to add filters and servlets when STOPPED or STARTED, but not when STARTING.
So I think it is a bug in jetty-servlet. However, @lachlan-roberts still would be best to avoid ensureFilter call from another servlet if possible... so if we can work out the filter is needed earlier, then that would be good.

@gregw gregw self-assigned this Oct 5, 2020
@gregw gregw changed the title NPE is WebSocketUpgradeFilter Filter/Servlet/Listener Holders are not started if added during STARTING state. Oct 5, 2020
@gregw gregw added this to To do in Jetty 9.4.33 via automation Oct 5, 2020
gregw added a commit that referenced this issue Oct 5, 2020
Holders are now started/initialized if needed by a new utility method
@gregw gregw linked a pull request Oct 5, 2020 that will close this issue
@gregw gregw added the Bug For general bugs on Jetty side label Oct 5, 2020
@lachlan-roberts
Copy link
Contributor

We only really know the WebSocketUpgradeFilter is needed when addMapping() is called on the JettyWebSocketServerContainer, and someone could call this at any time, and the first chance to call it is after startup.

So we could go back to always creating the WebSocketUpgradeFilter in the SCI, but it may not be used if only using the upgrade servlet. I don't think there is any other way to know if it is needed ahead of time.

@gregw
Copy link
Contributor

gregw commented Oct 6, 2020

@lachlan-roberts Let's see how this PR helps (or not).

lachlan-roberts added a commit that referenced this issue Oct 6, 2020
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@lachlan-roberts
Copy link
Contributor

@gregw I have written tests to reproduce the cometd failure in the jetty tests (see 5e1e93d).
I tested it against your fix in PR #5397 and it makes the test pass.

gregw added a commit that referenced this issue Oct 6, 2020
Holders are now started/initialized if needed by a new utility method
lachlan-roberts added a commit that referenced this issue Oct 7, 2020
…he filter

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
lachlan-roberts added a commit that referenced this issue Oct 7, 2020
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
lachlan-roberts added a commit that referenced this issue Oct 7, 2020
…he filter

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@gregw gregw closed this as completed in 721a05e Oct 7, 2020
Jetty 9.4.33 automation moved this from To do to Done Oct 7, 2020
lachlan-roberts added a commit that referenced this issue Oct 8, 2020
…Filter

Issue #5378 - add test to reproduce issue with WebSocketUpgradeFilter not started
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side
Projects
No open projects
Jetty 9.4.33
  
Done
Development

Successfully merging a pull request may close this issue.

4 participants