Jaroslav Bachorik wrote:
On 01/09/2013 09:40 AM, shanliang wrote:
I still have no idea why the test failed, but I do not see why a longer
timeout can fix the test. Have you reproduced the problem and tested
your fix? if yes then possible the long timeout hided a real problem.
Yes, I can reproduce the problem (using the fastbuild bits and -Xcomp
switch) and verify that the fix makes the test pass.
The ClientNotifForwarder scans the notifications for
MBeanServerNotification.UNREGISTRATION_NOTIFICATION and removes the
appropriate notification listeners in a separate thread. Thus, calling
"removeNotificationListener" on the main thread is prone to racing.
It is true that ClientNotifForwarder scans the notifications for
MBeanServerNotification.UNREGISTRATION_NOTIFICATION and removes the
appropriate notification listeners in a separate thread. This is for a
client connection to do clean if a user never calls
removeNotificationListener.
But calling directly removeNotificationListener from a client should
still get exception if the clean has not been done. As I said, if the
client checked and found the listener was still there, then the client
sent a request to its server to remove the listener at server side, the
server should find that the MBean in question was not registered, so the
server should throw an exception. The bug might be here.
Shanliang
The timeout you made longer was used to wait a notification which should
never arrive.
Well, it can be used to allow more time to process the "unregister"
notification, too.
When I think more of this I am more inclined to fix the race condition.
An updated webrev will follow.
To remove a listener from a client side, we did:
1) at client side, check whether it was added in the client side
2) at server side, check whether the MBean in question was registered in
the MBeanServer (!!!)
3) at server side, check whether the listener was added.
So 2) tells that we did not rely on a "unregister" notification. Anyway,
if you use a SAME thread to call "unregister" operation to unregister an
mbean, then any following call (without any time break) to use the mbean
should fail, like "removeNotificationListener", "isRegistered" etc.
I do see a bug here, if we remove a listener from a non-existing MBeam,
we get "ListenerNotFoundException" at a client side, but get
"InstanceNotFoundException" at server side, I think we should create a
bug, because both implemented the same interface MBeanServerConnection.
Yes, it is rather inconsistent.
-JB-
Shanliang
Jaroslav Bachorik wrote:
Looking for review and a sponsor.
Webrev at http://cr.openjdk.java.net/~jbachorik/7170447/webrev.00
In this issue the timing is the problem. MBeanServer.unregisterMBean()
fires the "unregister" notification which is sent to the server
asynchronously. Thus it may happen that the "unregister" notification
has not been yet processed at the time of invoking
removeNotificationListener() and the notification listeners hasn't been
cleaned up leading to the test failure.
There is no synchronization between the client and the server and such
race condition can occur occasionally. Normally, the execution is fast
enough to behave like the "unregister" notification is processed
synchronously with the unregisterMBean() operation but it seems that
using fastdebug Server VM bits with the -Xcomp option strains the CPU
enough to make this problem appear.
There is no proper fix for this - the only thing that work is waiting a
bit longer in the main thread to give the notification processing thread
some time to clean up the listeners.
Regards,
-JB-
