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

DisabledFeatureProxy should proceed hashCode and equals methods to avoid breaking Spring ApplicationContext #196

Closed
YannRobert opened this Issue Jan 17, 2013 · 9 comments

Comments

Projects
None yet
2 participants
@YannRobert

YannRobert commented Jan 17, 2013

Using GLU version 4.6.1

I find random errors in console server's logs.

I think DisabledFeatureProxy should proceed hashCode and equals methods to avoid breaking Spring ApplicationContext

475148 [Thread-7] ERROR org.codehaus.groovy.grails.commons.spring.ReloadAwareAutowireCapableBeanFactory - Destroy method on bean with name 'commandsService' threw an exception
org.linkedin.glu.utils.exceptions.DisabledFeatureException: commands
at org.linkedin.glu.utils.core.DisabledFeatureProxy.invoke(DisabledFeatureProxy.java:42)
at $Proxy9.hashCode(Unknown Source)
at java.util.concurrent.ConcurrentHashMap.remove(ConcurrentHashMap.java:933)
at org.springframework.orm.jpa.support.PersistenceAnnotationBeanPostProcessor.postProcessBeforeDestruction(PersistenceAnnotationBeanPostProcessor.java:355)
at org.springframework.beans.factory.support.DisposableBeanAdapter.destroy(DisposableBeanAdapter.java:166)
at org.springframework.beans.factory.support.DefaultSingletonBeanRegistry.destroyBean(DefaultSingletonBeanRegistry.java:487)
at org.springframework.beans.factory.support.DefaultSingletonBeanRegistry.destroySingleton(DefaultSingletonBeanRegistry.java:463)
at org.springframework.beans.factory.support.DefaultSingletonBeanRegistry.destroySingletons(DefaultSingletonBeanRegistry.java:431)
at org.springframework.context.support.AbstractApplicationContext.destroyBeans(AbstractApplicationContext.java:1048)
at org.springframework.context.support.AbstractApplicationContext.doClose(AbstractApplicationContext.java:1022)
at org.springframework.context.support.AbstractApplicationContext.close(AbstractApplicationContext.java:970)
at org.springframework.web.servlet.FrameworkServlet.destroy(FrameworkServlet.java:737)
at org.codehaus.groovy.grails.web.servlet.GrailsDispatcherServlet.destroy(GrailsDispatcherServlet.java:209)
at org.eclipse.jetty.servlet.ServletHolder.destroyInstance(ServletHolder.java:310)
at org.eclipse.jetty.servlet.ServletHolder.doStop(ServletHolder.java:284)
at org.eclipse.jetty.util.component.AbstractLifeCycle.stop(AbstractLifeCycle.java:80)
at org.eclipse.jetty.servlet.ServletHandler.doStop(ServletHandler.java:195)
at org.eclipse.jetty.util.component.AbstractLifeCycle.stop(AbstractLifeCycle.java:80)
at org.eclipse.jetty.server.handler.HandlerWrapper.doStop(HandlerWrapper.java:106)
at org.eclipse.jetty.security.SecurityHandler.doStop(SecurityHandler.java:350)
at org.eclipse.jetty.util.component.AbstractLifeCycle.stop(AbstractLifeCycle.java:80)
at org.eclipse.jetty.server.handler.HandlerWrapper.doStop(HandlerWrapper.java:106)
at org.eclipse.jetty.server.session.SessionHandler.doStop(SessionHandler.java:125)
at org.eclipse.jetty.util.component.AbstractLifeCycle.stop(AbstractLifeCycle.java:80)
at org.eclipse.jetty.server.handler.HandlerWrapper.doStop(HandlerWrapper.java:106)
at org.eclipse.jetty.server.handler.ContextHandler.doStop(ContextHandler.java:671)
at org.eclipse.jetty.servlet.ServletContextHandler.doStop(ServletContextHandler.java:143)
at org.eclipse.jetty.webapp.WebAppContext.doStop(WebAppContext.java:536)
at org.eclipse.jetty.util.component.AbstractLifeCycle.stop(AbstractLifeCycle.java:80)
at org.eclipse.jetty.server.handler.HandlerCollection.doStop(HandlerCollection.java:247)
at org.eclipse.jetty.util.component.AbstractLifeCycle.stop(AbstractLifeCycle.java:80)
at org.eclipse.jetty.server.handler.HandlerCollection.doStop(HandlerCollection.java:247)
at org.eclipse.jetty.util.component.AbstractLifeCycle.stop(AbstractLifeCycle.java:80)
at org.eclipse.jetty.server.handler.HandlerWrapper.doStop(HandlerWrapper.java:106)
at org.eclipse.jetty.server.Server.doStop(Server.java:300)
at org.eclipse.jetty.util.component.AbstractLifeCycle.stop(AbstractLifeCycle.java:80)
at org.eclipse.jetty.util.thread.ShutdownThread.run(ShutdownThread.java:124)
2013-01-17 16:47:32.868:INFO:/console:Closing Spring root WebApplicationContext

@YannRobert

This comment has been minimized.

YannRobert commented Jan 17, 2013

what do you think of this ? https://gist.github.com/4557761

@ypujante

This comment has been minimized.

Member

ypujante commented Jan 17, 2013

Thanks for the report and the gist. I think your proposal looks good although I am just wondering if in the end all methods inherited from Object should not be "passed through" (toString, wait, notify, etc...).

I am just curious how you reproduce the problem so that I can make sure that the problem is fixed (I did not see this exception during my testing).

Thanks
Yan

@YannRobert

This comment has been minimized.

YannRobert commented Jan 17, 2013

I don't think it is mandatory to let pass other methods from Object.class.
I will do more testing on our setup to verify my patch is fixing the issue.
Because glu is a bit customized here, I don't know how you can reproduce, as well as I don't know how to change our settings for this issue to disapear. I'll try to work on that.

@YannRobert

This comment has been minimized.

YannRobert commented Jan 18, 2013

Found in code, 2 different conditions that leads to the usage of DisabledFeatureProxy
for 2 different Interfaces

./console/org.linkedin.glu.console-webapp/grails-app/conf/spring/resources.groovy
if(consoleConfig.isFeatureEnabled("commands"))
=> proxy on interface CommandsService

./agent/org.linkedin.glu.agent-server-impl/src/main/groovy/org/linkedin/glu/agent/server/AgentMain.groovy
if(Config.getOptionalBoolean(_config, "${prefix}.agent.features.commands.enabled", false))
=> proxy on interface CommandManager

Are those conditions equivalent ?

@YannRobert

This comment has been minimized.

YannRobert commented Jan 18, 2013

On DisabledFeatureProxy implementation, I think the approche is to pass the Interface to be disabled in the DisabledFeatureProxy constructor, then the proxy should check if the invoked method's declaring class is the aforementioned Interface. If so, then throw the Exception, otherwise execute the method on itself.

@ypujante

This comment has been minimized.

Member

ypujante commented Jan 18, 2013

In regards to your question "Are those conditions equivalent?", the answer is yes: one is for the console, the other one for the agent.

In regards to fixing the issue, I really need to know how to reproduce the problem in the first place. Otherwise I cannot test that the fix addresses the issue, no matter what the fix is.

@ypujante

This comment has been minimized.

Member

ypujante commented Jan 21, 2013

Fixed in 4.6.2. Can you please regress? I wrote a test but since I could never reproduce the problem, I am unclear whether the changes actually fixes your issue.

Thanks

@ypujante ypujante closed this Jan 21, 2013

@YannRobert

This comment has been minimized.

YannRobert commented Jan 22, 2013

To reproduce, try to set the "commands" feature off by using

features: [
commands: false
]
(or just omit to mention the features object to use the default value)

and trigger the Spring Application destroy by using command line console-cli.sh stop

I guess by reading the stacktrace that the destroy method will trigger the "postProcessBeforeDestruction" method of all DestructionAwareBeanPostProcessor.
We do not have any customization at that level, so I guess there is already one.

If not, to reproduce, I suggest, you should ensure that the ApplicationContext contains such a DestructionAwareBeanPostProcessor (and maybe any BeanPostProcessor that will be triggered after the DisabledFeatureProxy is added to the Context).

Following your release, I tested on version 4.6.2 and did not encountered the bug.
Your change fixes the issue.

Thanks a lot.

@ypujante

This comment has been minimized.

Member

ypujante commented Jan 22, 2013

Thanks for the regression test!

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