Buggy exception handling in ServerNotifForwarder#removeNotificationListener

[Kevin Walls]

Hi -

Yes, looks like we intend to keep the first Exception thrown, and throw that after the loop, but not to stop the loop attempting all removeNotificationListener() calls.  So it would make sense to assign the caught Exception to re, only if re IS null. 

So currently this method never throws an Exception.  Let me know if you've tested the change or plan to log a bug.  We should proceed with care in case this change causes any surprises, as it  has been like this forever.

Thanks
Kevin


-----Original Message-----
From: serviceability-dev <serviceability-dev-retn at openjdk.java.net> On Behalf Of Andrey Turbanov
Sent: 18 August 2021 12:52
To: serviceability-dev at openjdk.java.net
Subject: Buggy exception handling in ServerNotifForwarder#removeNotificationListener

Hello.

During investigation of IDEA inspections I found suspicious code in a method com.sun.jmx.remote.internal.ServerNotifForwarder#removeNotificationListener
https://github.com/openjdk/jdk/blob/master/src/java.management/share/classes/com/sun/jmx/remote/internal/ServerNotifForwarder.java#L167

Exception re = null;
for (int i = 0 ; i < listenerIDs.length ; i++) {
    try {
        removeNotificationListener(name, listenerIDs[i]);
    } catch (Exception e) {
        // Give back the first exception
        //
        if (re != null) {
            re = e;
        }
    }
}
if (re != null) {
    throw re;
}

Variable 're' set initially to 'null', but then in a catch block it checked to be '!= null'.
As you can see this condition can never be true. And exceptions from inner calls are not propagated as expected.
It seems that it should check if (re == null) inside the catch block.


Andrey Turbanov