jmx-dev RFR: 8344976: Remove the jmx.invoke.getters compatibility property [v2]

[Kevin Walls]

On Fri, 24 Jan 2025 23:50:47 GMT, Kevin Walls <kevinw at 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