[ mx4j-Bugs-1554142 ] Timing hole in Monitor stop() start() processing

Bugs item #1554142, was opened at 2006-09-07 10:21
Message generated for change (Comment added) made by kevanm
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=450647&aid=1554142&group_id=47745

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: JMX implementation
Group: None
Status: Open
Resolution: None
Priority: 5
Submitted By: Kevan Miller (kevanm)
Assigned to: Nobody/Anonymous (nobody)
Summary: Timing hole in Monitor stop() start() processing

Initial Comment:
I'm seeing occasional Monitor failures in some of the testing performed 
for Geronimo. The basic scenario is:

1) start Monitor
2) perform some processing
3) stop Monitor
4) sleep for a while
5) start Monitor
6) do some stuff...
7) test the number of Notifications

There is a timing window in the mx4j implementation. In certain cases, 
a notification can be scheduled for delivery after Monitor.stop() has 
been invoked. In this case, MX4J will set the notification status of the 
Monitor to true. If the Monitor is subsequently started (step 5 above), 
MX4J will not deliver a notification for the Monitor (because MX4J will 
mistakenly believe a notification has already been delivered).

Notification status is cleared in MX4JMonitor.stop(), which is a 
synchronized method. However, the scheduling of Notifications by
MX4JMonitor$MonitorTask.run() is unsynchronized and only checks 
Monitor state at the beginning of the method. So, there's a pretty large 
window for a Notification to occur after the Monitor has been 
stopped... 

The JMX specification is a bit fuzzy on the proper behavior for this 
scenario, but I'm pretty sure that a notification should always be 
delivered in this scenario after step 5. As a user, I would expect a 
notification and my interpretation of the intent of the mx4j 
implementation is to always deliver a notification in this case...

A true fix is to synchronize the stop/start and notification processing 
for Monitors. A less robust, but likely to fix my problem, solution 
would be to always clear the notifcation status for a monitor when 
*starting* the monitor (not just when stopping).

Happy to work up the fix for this and/or test... Just let me know...


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

>Comment By: Kevan Miller (kevanm)
Date: 2006-09-29 11:22

Message:
Logged In: YES 
user_id=1592769

Hi Simone,
All JMX tests look good. Thanks!

--kevan

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

Comment By: Kevan Miller (kevanm)
Date: 2006-09-29 09:22

Message:
Logged In: YES 
user_id=1592769

Hi Simone,
Apologies for my slow turn around. That definitely fixes the problem. I'm 
currently running the entire suite of JMX tests, to make sure there aren't any 
regressions due to other changes. Will let you know once they complete.

Any plans for a service release?  

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

Comment By: Simone Bordet (simonebordet)
Date: 2006-09-20 10:36

Message:
Logged In: YES 
user_id=1170988

I moved the reset of the notification status from stop() to
start() and committed to CVS head.
Can confirm that the fix resolves the bug ?
Thanks !

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

Comment By: Kevan Miller (kevanm)
Date: 2006-09-14 11:26

Message:
Logged In: YES 
user_id=1592769

Hi Simone,
I'm afraid I can't share the test case with you. I did try to generate a test case 
which would illustrate the problem, but my initial attempt did not work. I'll 
have another look at it. It's hard to get the timing right to illustrate the timing 
hole...

You are correct that MonitorInfo only maintains an errorNotified state. 
However, MX4JStringMonitor, MX4JGaugeMonitor and MX4JCounterMonitor 
each have internal classes which extend MonitorInfo. They override the 
clearNotificationStatus() method and maintain additional "notification 
state" (e.g. differNotified and matchNotified in StringMonitorInfo).

I think it's OK that a notification can be delivered after Monitor.stop() has 
returned. The spec seems silent on the matter. I could imagine more strict 
behavior (ala JMS MessageListener.onMessage()). Given that there is no 
mandate by the spec, users should not be assuming one way or the other...

Notifications for a started Monitor are a different matter. IMO, if a notification 
condition is met, then a notification should always be delivered for a started 
Monitor (assuming that the Monitor remains started for a long enough 
time...). In this case, a notification is not being delivered. 

Here's a more specific scenario:

1. A StringMonitor is created (setNotifyMatch(true) and setStringToCompare()) 
and a matching MonitorTarget is created. 
2. [Thread 1] MX4JMonitor.start() is called
3. [Thread 2] MonitorTask.run() is entered and passes the isActive() check.
4. [Thread 1] MX4JMonitor.stop() is called
5. [Thread 1] StringMonitorInfo.clearNotificationStatus() is called (set's 
matchNotified and differNotified to false)
5. [Thread 2] MX4JStringMonitor.compareAndSendNotification is called and 
determines that there is a match
6. [Thread 2] StringMonitorInfo.setMatchNotified(true) is called. So, 
StringMonitorInfo.matchNotified is now true.
7. [Thread 2] MX4JMonitor.sendNotification() is called

As discussed previously, I'm fine with the above. The problem is as follows:

8. [Thread 1] StringMonitor.start() is called
9. [Thread 2] MX4JStringMonitor.compareAndSendNotification() is called. A 
Match Notification should be sent. However, matchNotified is set to true. So, 
a MX4JStringMonitor.compareAndSendNotification() will incorrectly believe 
that a match notification has already been sent.

I hope the above more clearly identifies the problem.

Adding the following to MX4JMonitor.start() will fix this problem:

      for (Iterator i = infos.values().iterator(); i.hasNext();)
      {
         MonitorInfo info = (MonitorInfo)i.next();
         info.clearNotificationStatus();
      }

There may be some pathological cases (multiple starts and stops of a Monitor 
along with updates to the MonitorTarget) where things don't work propertly. I 
haven't given it much thought... I don't think I'll be losing any sleep over it, 
either... :-)

 

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

Comment By: Simone Bordet (simonebordet)
Date: 2006-09-13 17:04

Message:
Logged In: YES 
user_id=1170988

Kevan,

I am not sure I understood completely the problem. If you
have already a test case (in code) will greatly help me to
understand.

Monitor has no "notification status"; MonitorInfo has a
method "clearNotificationStatus", probably misnamed, which
is used to avoid to send multiple error notifications when
one has already been sent (as required by the spec).
>From your report you don't mention MonitorInfo, but you do
mention Monitor, so I am confused on what you mean by
"notification status".

Also, it is not clear to me if you like the fact that a
notification can be sent after the Monitor has been stopped
or not. My take on the issue is that if the
MonitorTask.run() is entered by a thread before another
thread calls stop(), the notification should be sent. I
think this is the current behavior.

If you can clarify with a test case, will be great.

Thanks !

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

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

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys -- and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV