Review Request: 7195779 javax/management/remote/mandatory/threads/ExecutorTest.java fail intermittently

[Jaroslav Bachorik]

On Wed 19 Sep 2012 03:06:05 PM CEST, Alan Bateman wrote:
> On 19/09/2012 13:39, Jaroslav Bachorik wrote:
>> The ExecutorTest.java fails in cca. 30-40% of runs with NPE in the part
>> testing IIOP connection.
>>
>> The failure is caused by the JMX notification processing - ideally, once
>> the RMIConnector is closed the client JMX notification processing should
>> quit as well and should not try to fetch any more notifications over the
>> already closed remote connection. But achieving this is particularly
>> difficult mostly due to the synchronous nature of the
>> fetchNotifications() method. The RMIConnector can get closed after the
>> fetchNotifications() method was entered but before attempting to invoke
>> its remote counterpart. At this moment the remote connection is
>> effectively closed and any attempt to use it will throw an exception - a
>> NPE in this case.
>>
>> The patch does not solve the core problem as it would probably require
>> significant changes in the client JMX notification processing - rather
>> it targets the NPE which becomes expected when the RMIConnector is
>> closed. This is handled by adding a new "catch (NullPointerException)"
>> block and, inside that block, checking for the connection state and
>> rethrowing the exception in case the connection is still open
>> (RMIConnector is started). For any other state of RMIConnector the NPE
>> is ignored.
>>
>> The webrev is located at:
>> http://washi.ru.oracle.com/jb198685/webrev/7195779
>>
> This webrev looks to be Oracle internal so folks on this list will not
> be able to look at it.

Ok, I am attaching the change (webrev.zip attachment seems to be too big
for the mailing list):

---
old/src/share/classes/com/sun/jmx/remote/internal/ClientNotifForwarder.java
2012-09-19 14:23:56.966032400 +0200
+++
new/src/share/classes/com/sun/jmx/remote/internal/ClientNotifForwarder.java
2012-09-19 14:23:56.814032403 +0200
@@ -594,6 +594,18 @@
                 }

                 return nr;
+            } catch (NullPointerException e) {
+                synchronized(ClientNotifForwarder.this) {
+                    if (state == STARTED) {
+                        throw e;
+                    }
+                    /* When the attached RMIConnector is detached a NPE
+                       can occur since the remote connection closes in
the middle
+                       of fetchNotifs() execution.
+                       The NPE can be safely ignored in that case.
+                    */
+                    return null;
+                }
             } catch (ClassNotFoundException e) {
                 logger.trace("NotifFetcher.fetchNotifs", e);
                 return fetchOneNotif();

>
> In any case, I'm not sure that catching the NPE is a good idea as it
> seems to be masking the real problem. At the lower level then would it

I don't like ignoring an exception as well. But fixing the processing
logic will take substantially longer time as I am not really familiar
with the code. Perhaps adding the test to ProblemList.txt would be
appropriate then?

> be possible to have an IOException thrown if the connection is closed?

Not really - the NPE is thrown within the IIOP generated TIE class. I
really don't feel like sticking my fingers into the CORBA code. The
application code just receives the NPE.

>
> -Alan.
>
>