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

[David Holmes]

On 23/11/2015 6:03 PM, Jini George wrote:
> Thank you David for the review.
>
> The bug report (JDK-7041183) mentions this:
>
>
> =======Begin bug report snippet ================================
> JNIEXPORT jobject JNICALL
>   Java_sun_management_MemoryPoolImpl_getUsage0
>     (JNIEnv *env, jobject pool)
>   {
>       jobject usage = jmm_interface->GetMemoryPoolUsage(env, pool);
>       if (usage == NULL) {
>           // Throw internal error since this implementation expects the
>           // pool will never become invalid.
>           JNU_ThrowInternalError(env, "Memory Pool not found");
>       }
>       return usage;
>   }
>
>   This function as well as Java_sun_management_MemoryPoolImpl_getMemoryManagers0, Java_sun_management_MemoryPoolImpl_getPeakUsage0 should propagate the exception that may be returned by the jmm call rather than throw InternalError.
>
> =======End bug report snippet ==================================

That assumes there are expected exceptions coming from the lower layer 
and was likely based on the fact that these InternalErrors were being 
thrown. But as I keep saying either it is a bug if the memory pools are 
invalid (in which case InternalError should be thrown) or we have to 
identify the circumstances under which they can be invalid and then 
update the spec to reflect that and to state what exceptions should be 
thrown.

> I could not find a scenario where the MemoryPool could not be found, due to some GC related fixes having gone in in the past. (Hence the closure of the related https://bugs.openjdk.java.net/browse/JDK-8025089 ). As you said, that is a scenario which shouldn’t exist. My changes made were to cater to having the exception thrown from the jmm layer being seen, as mentioned in the bug report (if in case, the MemoryPool is not found, maybe due to some corruption or some unforeseen bug which might get introduced later).
>
> Still, having said that, if you and others feel that it is better to retain the Internal Error, I can close the bug as no change.

If we have not identified legitimate reasons for the pool being invalid 
then I would close this as not a bug.

Cheers,
David
-----

> Thanks,
> Jini.
>
>
>> -----Original Message-----
>> From: David Holmes
>> Sent: Monday, November 23, 2015 12:14 PM
>> To: Jini George; serviceability-dev at openjdk.java.net; Dmitry Samersoff
>> Subject: Re: RFR(S): JDK-7041183 Improve error handling in Improve
>> error handling in src/share/vm/services/management.cpp
>>
>> 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-
>> 704118
>>> 3/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.
>>>