On Fri, 24 Jan 2025 23:50:47 GMT, Kevin Walls <kev...@openjdk.org> wrote:
>> src/java.management/share/classes/com/sun/jmx/mbeanserver/PerInterface.java
>> line 149:
>>
>>> 147: throws MBeanException, ReflectionException {
>>> 148:
>>> 149: // Construct the exception that we will throw
>>
>> Although your changes look fine, the existing code for constructing this
>> exception is odd in that it artificially introduces a `cause` exception. It
>> seems to mostly want to capture additional msg information, but most of it
>> seems to be duplicated in the msg passed in, and all this could instead just
>> be handled with a single msg constructed at the call site (and the
>> ReflectionException could be allocated at the call site). Also, what it the
>> @SuppressWarnings("removal") for?
>
> Thanks -
>
> Ah, "removal" was added for SM removal, that can surely go now. Actually I
> don't see anything in what we're removing which required it, seems to have
> been unnecessary.
>
> The method is almost trivial now, and only used in two places. Yes it is a
> bit odd to manufacture a new cause, rather than throw something immediately.
>
> Elsewhere in this file, e.g. the getAttribute method does just throw new
> AttributeNotFoundException(msg),
> but here in invoke() we use "return noSuchMethod(...)" which effectively does:
>
> throw new ReflectionException(new NoSuchMethodException(operation +
> sigString(signature)), msg);
>
> (Hah, yes unlike a new Exception(String message, Throwable cause),
> ReflectionException uses new ReflectionException(Exception e, String
> message).)
>
> Two lines like that mean that the noSuchMethod method can be removed. I'm
> trying that out...
>
> But changing the behaviour (throwing a different Exception) might come with
> some compatibilty risk. I think it's good not to change the behaviour in
> this property removal change.
Updated with noSuchMethod method removed, and the two places t was called now
create their own Exception.
This makes it clearer just how odd it is, to create a NoSuchMethodException and
stash it in a ReflectionException (the invoke() method throws MBeanException,
ReflectionException).
This behaviour has been here for many years and we don't really want to sneak
in a behaviour change here. But I quite like this update for the clarity.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23132#discussion_r1930251123
