[ mx4j-Bugs-855004 ] NotifBroadcasterSupport.sendNotif not thread safe

Bugs item #855004, was opened at 2003-12-05 14:52
Message generated for change (Comment added) made by btscully
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=450647&aid=855004&group_id=47745

Category: JMX implementation
Group: Release 2.0.0
>Status: Closed
>Resolution: Fixed
Priority: 5
Submitted By: Nobody/Anonymous (nobody)
Assigned to: Brian Scully (btscully)
Summary: NotifBroadcasterSupport.sendNotif not thread safe

Initial Comment:
NotificationBroadcasterSupport.sendNotification is not 
thread-safe.  It throws NullPointerException under 
certain conditions (which I have been witnessing fairly 
regularly).

Here's a portion of that method:

synchronized (this)
{
   // Clone the listeners, so we can notify without 
holding any lock
   // It is a shallow copy, below we will clone the pairs 
as well
   // I don't care if in the middle someone else adds or 
remove other pairs
   listeners = (HashMap)m_listeners.clone();
}

...loop is here that iterates around "listeners" map - 
below is inside that loop...

      // Clone again the pairs for this listener.
      // I freezed the listeners with the first clone, if 
someone removes a pair
      // in the middle of notifications I don't care: here I 
clone the actual pairs
      ArrayList pairs = null;
      synchronized (this)
      {
         pairs = (ArrayList)m_listeners.get(listener);  <----
- LOOK HERE!!!!
         pairs = (ArrayList)pairs.clone();
      }

m_listeners is cloned so they don't have to keep "this" 
synchronized through the whole thing.  Read the second 
comment where it says "I freezed the listeners with the 
first clone, if someone removes a pair in the middle, I 
don't care".

I see what is supposed to be done, unfortunately, the 
wrong variable is used.  Yes m_listeners is frozen (in the 
clone listeners) so if m_listeners removes an item it 
doesn't matter because listeners remains intact.  
However, the wrong variable is used when getting 
the "pairs" value.  The clone should be used, not the real 
m_listeners Map.  That line where I have LOOK HERE 
should use "listeners", NOT "m_listeners"; the the bug fix 
should look like this:

         pairs = (ArrayList)listeners.get(listener);

I stepped through this in the debugger and what that 
comment tries to avoid is really happening.  Between the 
time the clone is taking and the time that .get() is 
called, m_listeners has had a listener removed from it.  
So, when that LOOK HERE line is executed (and because 
it isn't using the clone) a null is returned causing a 
NullPointerException on the next line.

Workaround is for my MBean to override sendNotification 
and to call super.sendNotification wrapped in 
a "synchronized(this)" block.  This should be a temporary 
workaround since synchronizing here means the entire 
MBean is blocked from doing anything inside of MX4J 
until all handlers have completed their job (which takes 
an unknown about of time, depending on what the 
listeners need to do and how many of them there are).

John Mazz
Hewlett-Packard, Company
[email protected]

----------------------------------------------------------------------

>Comment By: Brian Scully (btscully)
Date: 2003-12-06 12:24

Message:
Logged In: YES 
user_id=234750

User recommended patch applied & committed to source control.

----------------------------------------------------------------------

Comment By: Brian Scully (btscully)
Date: 2003-12-06 12:17

Message:
Logged In: YES 
user_id=234750

I don't even have to duplicate this one.  It's obvious.  This is a good catch of a potentially insidious bug.  Thanks.

----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=450647&aid=855004&group_id=47745


-------------------------------------------------------
This SF.net email is sponsored by: IBM Linux Tutorials.
Become an expert in LINUX or just sharpen your skills.  Sign up for IBM's
Free Linux Tutorials.  Learn everything from the bash shell to sys admin.
Click now! http://ads.osdn.com/?ad_id=1278&alloc_id=3371&op=click