Bug 40246 - HierarchyDynamicMBean missing unregister MBean
Summary: HierarchyDynamicMBean missing unregister MBean
Status: RESOLVED FIXED
Alias: None
Product: Log4j - Now in Jira
Classification: Unclassified
Component: Other (show other bugs)
Version: 1.2
Hardware: Other other
: P2 enhancement
Target Milestone: ---
Assignee: log4j-dev
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-08-14 09:59 UTC by wing tung Leung
Modified: 2008-08-14 20:34 UTC (History)
0 users



Attachments
Patch JMX MBean classes to perform automatic cleanup (4.10 KB, patch)
2006-08-14 12:25 UTC, wing tung Leung
Details | Diff
log4j 1.2 version of the mbean deregister patch (4.81 KB, patch)
2007-04-29 16:19 UTC, Paul Smith
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description wing tung Leung 2006-08-14 09:59:25 UTC
(log4j-1.2.13)

I'm using log4j in a WebLogic J2EE application and added an JMX interface using
the existing HierarchyDynamicMBean. During and after the first registration of
the MBean, everything seems to work fine. But when I unregister the MBean and
register it again (because of redeploying the application), I get a stacktrace
indicating that the appender MBean is already registered.

As far as I understand by reading the code, the HierarchyDynamicMBean registers
the logger(s) but never unregisters them. I would expected automatic
unregistration of the logger(s) when I unregister the HierarchyDynamicMBean,
probably somewhere in preDeregister().

Since the addLoggerMBean() returns you the object name, the calling code can
perform the cleanup itself as well, but that is IMHO not the option to prefer.
In case you want to stick to this behaviour, the method needs at least some
explicit javadoc comment explaining it.

If someone gives a hint which direction is the best, I'm willing to write a
small patch.
Comment 1 wing tung Leung 2006-08-14 12:25:34 UTC
Created attachment 18715 [details]
Patch JMX MBean classes to perform automatic cleanup

This patch solves my problem and unregisters all MBean instances during
undeployment. My previously described workaround is not working because some
MBean instances are added automatically without needing the explicit
addLoggerMBean() call.

Please review for inclusion.
Comment 2 Paul Smith 2007-04-23 16:20:15 UTC
1.2.15 consideration

Although this patch is targetted for trunk, it would need slight rework for
inclusion in the 1.2 tree, because it uses JDK 1.2+ Collections framework plus
the use of the getLogger() calls.  

If we consider this for the 1.2 tree, then it'll need to port that to standard
java.util.Vector class, and use LogLog instead.  If the original author was able
to, a patch against the 1.2 tree would be well received, otherwise I might take
a stab at it later.
Comment 3 Paul Smith 2007-04-29 15:21:36 UTC
Reassigning this one back to log4j-dev list, it was my mistake to completely
reassign this issue, it prevents the comments from being seen on the mailing list.
Comment 4 Paul Smith 2007-04-29 16:19:59 UTC
Created attachment 20072 [details]
log4j 1.2 version of the mbean deregister patch

I modified the original patch (which I had to apply by hand, wasn't quite a
unified diff format) to make it log4j 1.2 compatible.

I've had difficulty testing this however, because I can't seem to get the
preDeregister method to fire at all.  I can't work out exactly what the correct
way to 'shutdown' the JMX agent in my test harness app I have local.

If someone else knew what to do to verify behaviour, could they take the patch
and try it out?
Comment 5 Thorbjørn Ravn Andersen 2008-08-02 10:03:13 UTC
Help needed to verify the functionality of the code.
Comment 6 Paul Smith 2008-08-05 15:29:59 UTC
I'd actually like to see this one done for 1.2.16 personally. 
Comment 7 Paul Smith 2008-08-14 20:34:01 UTC
Fixed in revision 686131.  Tested by creating a local test harness class that instantiates an MBeanServer, and registers the Hierarchy Mbean and then unregister it.  The Loggers and Appenders appear to register and unregister successfully.

Thanks for the patch, it's only been 2 years and 1 day in applying!