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

AgentServlet updateAgentDetailsIfNeeded isn't thread safe #221

Closed
kingpin2k opened this Issue Oct 9, 2015 · 2 comments

Comments

Projects
None yet
3 participants
@kingpin2k

kingpin2k commented Oct 9, 2015

I've got multiple calls that are lucky enough to happen at the same time before the agent details have been initialized, one will win, the others will lose. I'll try and get around to a PR, but I'll likely hack in a retry until I have free time.

Offending Class:

https://github.com/rhuss/jolokia/blob/master/agent/core/src/main/java/org/jolokia/http/AgentServlet.java

Offending Method:

private void updateAgentDetailsIfNeeded(HttpServletRequest pReq) {
    // Lookup the Agent URL if needed
    AgentDetails details = backendManager.getAgentDetails();
    if (details.isInitRequired()) {
        if (details.isUrlMissing()) {
            String url = getBaseUrl(NetworkUtil.sanitizeLocalUrl(pReq.getRequestURL().toString()),
                                    extractServletPath(pReq));
            details.setUrl(url);
        }
        if (details.isSecuredMissing()) {
            details.setSecured(pReq.getAuthType() != null);
        }
        details.seal();
    }
}

Stacktrace:

java.lang.IllegalStateException: Cannot update agent details because it is already initialized and sealed
 at org.jolokia.discovery.AgentDetails.checkSeal(AgentDetails.java:108)
 at org.jolokia.discovery.AgentDetails.setUrl(AgentDetails.java:97)
 at org.jolokia.http.AgentServlet.updateAgentDetailsIfNeeded(AgentServlet.java:334)
 at org.jolokia.http.AgentServlet.handle(AgentServlet.java:281)
 at org.jolokia.http.AgentServlet.doPost(AgentServlet.java:252)
 at javax.servlet.http.HttpServlet.service(HttpServlet.java:648)
 at javax.servlet.http.HttpServlet.service(HttpServlet.java:729)
 at org.springframework.web.servlet.mvc.ServletWrappingController.handleRequestInternal(ServletWrappingController.java:158)
 at org.springframework.web.servlet.mvc.AbstractController.handleRequest(AbstractController.java:147)
 at org.springframework.boot.actuate.endpoint.mvc.JolokiaMvcEndpoint.handle(JolokiaMvcEndpoint.java:131)
 at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
 at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
 at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
 at java.lang.reflect.Method.invoke(Method.java:497)
 at org.springframework.web.method.support.InvocableHandlerMethod.doInvoke(InvocableHandlerMethod.java:221)
 at org.springframework.web.method.support.InvocableHandlerMethod.invokeForRequest(InvocableHandlerMethod.java:137)
 at org.springframework.web.servlet.mvc.method.annotation.ServletInvocableHandlerMethod.invokeAndHandle(ServletInvocableHandlerMethod.java:111)
 at org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerAdapter.invokeHandlerMethod(RequestMappingHandlerAdapter.java:806)
 at org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerAdapter.handleInternal(RequestMappingHandlerAdapter.java:729)
 at org.springframework.web.servlet.mvc.method.AbstractHandlerMethodAdapter.handle(AbstractHandlerMethodAdapter.java:85)
 at org.springframework.web.servlet.DispatcherServlet.doDispatch(DispatcherServlet.java:959)
 at org.springframework.web.servlet.DispatcherServlet.doService(DispatcherServlet.java:893)
 at org.springframework.web.servlet.FrameworkServlet.processRequest(FrameworkServlet.java:970)
 at org.springframework.web.servlet.FrameworkServlet.doPost(FrameworkServlet.java:872)
 at javax.servlet.http.HttpServlet.service(HttpServlet.java:648)
 at org.springframework.web.servlet.FrameworkServlet.service(FrameworkServlet.java:846)
 at javax.servlet.http.HttpServlet.service(HttpServlet.java:729)
 at org.eclipse.jetty.servlet.ServletHolder.handle(ServletHolder.java:808)
 at org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1669)
 at org.springframework.web.servlet.resource.ResourceUrlEncodingFilter.doFilterInternal(ResourceUrlEncodingFilter.java:51)
 at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:107)
 at org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1652)
 at org.springframework.boot.actuate.autoconfigure.EndpointWebMvcAutoConfiguration$ApplicationContextHeaderFilter.doFilterInternal(EndpointWebMvcAutoConfiguration.java:235)
 at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:107)
 at org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1652)
 at org.springframework.boot.actuate.trace.WebRequestTraceFilter.doFilterInternal(WebRequestTraceFilter.java:102)
 at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:107)
 at org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1652)
 at org.springframework.web.filter.HttpPutFormContentFilter.doFilterInternal(HttpPutFormContentFilter.java:87)
 at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:107)
 at org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1652)
 at org.springframework.web.filter.HiddenHttpMethodFilter.doFilterInternal(HiddenHttpMethodFilter.java:77)
 at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:107)
 at org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1652)
 at org.springframework.web.filter.CharacterEncodingFilter.doFilterInternal(CharacterEncodingFilter.java:85)
 at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:107)
 at org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1652)
 at org.springframework.boot.actuate.autoconfigure.MetricsFilter.doFilterInternal(MetricsFilter.java:69)
 at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:107)
 at org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1652)
 at org.eclipse.jetty.servlet.ServletHandler.doHandle(ServletHandler.java:585)
 at org.eclipse.jetty.server.handler.ScopedHandler.handle(ScopedHandler.java:143)
 at org.eclipse.jetty.security.SecurityHandler.handle(SecurityHandler.java:577)
 at org.eclipse.jetty.server.session.SessionHandler.doHandle(SessionHandler.java:223)
 at org.eclipse.jetty.server.handler.ContextHandler.doHandle(ContextHandler.java:1127)
 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:310)
 at org.eclipse.jetty.server.HttpConnection.onFillable(HttpConnection.java:257)
 at org.eclipse.jetty.io.AbstractConnection$2.run(AbstractConnection.java:540)
 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)
@arnabbiswas1

This comment has been minimized.

Contributor

arnabbiswas1 commented Dec 18, 2015

As per my understanding, it's a classical case of double check across synchronization block. Considering that the initialisation will happen only once, there should not be any considerable performance cost for introducing the synchronised block. Your feedback will be appreciated.

@kingpin2k

This comment has been minimized.

kingpin2k commented Dec 18, 2015

Totally agree, synchronized block is the easiest and appropriate solution.

@rhuss rhuss closed this in 9e0c5ae Jan 15, 2016

@rhuss rhuss added this to the 1.3.3 milestone Jan 18, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment