RFR(S): JDK-7041183 Improve error handling in Improve error handling in src/share/vm/services/management.cpp

[David Holmes]

On 23/11/2015 12:35 PM, Jini George wrote:
> Hi David,
>
> Thanks much for the review and for the suggestions offline. I have addressed your comments and have a new webrev at:
>
> http://cr.openjdk.java.net/~dsamersoff/sponsorship/jingeorg/JDK-7041183/webrev.02/

src/share/vm/services/management.cpp

  424     THROW_MSG_(vmSymbols::java_lang_IllegalStateException(),
  425                "Memory Pool should exist",
  426                NULL);

why did you use the three arg THROW_MSG_ instead of the two-arg THROW_MSG?

  428   return (MemoryPool*) mpool;

Cast is not needed.

That aside I think we're still miscomunicating on this. Here's what I 
last said in our IM chat:

(4:24:02 PM) david.holmes: Basic premise: MemoryPoolMXBeans should never 
be invalid.
VM code returns NULL if they are found to be invalid.
JMM library code converts NULL to InternalError because it should not 
happen. If it does it is a VM bug.
You found a situation where they were invalid so the cause of that needs 
to be determined and either fixed, or else the specifications and code 
updated to reflect that there are circumstances where they can be invalid.

---

Changing the inner layer to throw IllegalStateException does not change 
the basic presumption that these pools should never be invalid - in 
which case the InternalError seems far more appropriate. Otherwise, as I 
said, you would need to update the specification and the code to allow 
for this IllegalStateException - you can't just change the code.

David
-----

> (Thanks, Dmitry for hosting). Further comments are inlined below.
>
>> -----Original Message-----
>>
>> On 30/10/2015 9:01 PM, Dmitry Samersoff wrote:
>>> Everybody,
>>>
>>> (* On behalf of Jini George <jini.george at oracle.com> *)
>>>
>>> Please review the fix
>>>
>>> http://cr.openjdk.java.net/~dsamersoff/sponsorship/jingeorg/JDK-
>> 704118
>>> 3/webrev.01/
>>
>> If you throw IllegalArgumentException it should be obvious at the Java
>> level which argument is illegal and why. I can't tell at the Java level
>> how that can come about - these seem to be methods invoked on a
>> MemoryPoolMXBean - see for example:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8025089
>>
>> so the "illegal argument" would seem to be "this" ???
>
> [[Jini]] This has been changed to the more valid IllegalStateException as you suggested. JDK-8025089 is not reproducible anymore and has been closed now.
>
>> I don't see any changes to the code that would currently throw Internal
>> Error ??
>
> [[Jini]] I removed the code throwing the Internal Error from the upper layer now since this would have masked the exception thrown from the inner layer.
>
>> The asserts for pool!=NULL seem rather pointless as the method has been
>> changed to never return null. If you really want the assert add it to
>> the end of the method being called, rather than placing at all the call
>> sites.
> [[Jini]] I removed these asserts now.
>
> Thanks,
> Jini.
>