RFR 4613913: Four EventRequest methods are invokable on deleted request

[David Holmes]

Hi Serguei,

On 3/04/2018 7:44 AM, serguei.spitsyn at oracle.com wrote:
> Hi David and Daniil,
> 
> 
> David,
> 
> Thank you for raising this concern.
> You are right.
> 
> I've made a mistake when looked at the EventRequest.isEnabled() spec and 
> thought
> that the following spec lines of the setEnbaled() belong to the isEnabled()
> and other 3 methods as well:
> 
> Throws:
>     |InvalidRequestStateException
>     <https://docs.oracle.com/javase/8/docs/jdk/api/jpda/jdi/com/sun/jdi/request/InvalidRequestStateException.html>|
>     - if this request has been deleted.
> 
> In fact, the JDI spec for methods isEnabled(), getProperty(), 
> putProperty() and suspendPolicy()
> does not say they can throw the InvalidRequestStateException.
> 
> So, now I'd suggest to just relax the test checks by not expecting an
> InvalidRequestStateException from isEnabled(), getProperty(), putProperty()
> and suspendPolicy().
> 
> Would this approach resolve your concern?

Yes. The semantics for these methods was established way back in 2000 under:

https://bugs.openjdk.java.net/browse/JDK-4320478

I think this bug, 4613913, was misguided in expecting all of the methods 
to throw the exception. You could make a case for doing so, but as I 
said that's a spec change that should have been made back then. Changing 
the spec now seems pointless - it gains nothing but introduces an 
incompatible behaviour change. Changing the test is the way to go.

Thanks,
David
-----

> Thanks,
> Serguei
> 
> 
> 
> 
> On 3/29/18 17:12, David Holmes wrote:
>> Daniil,
>>
>> Even as far back as 2007 there was concern that changing the current 
>> behaviour might break existing code. That has to be an even bigger 
>> concern now!
>>
>> Further the spec is sloppy here:
>>
>> " Once the eventRequest is deleted, no operations (for example, 
>> EventRequest.setEnabled(boolean)) are permitted."
>>
>> This is too loose. What is an "operation"? Is a query like isEnabled() 
>> really an "operation"? I would not consider it so. And if we can 
>> delete requests why is there no "isDeleted" query? The spec seems 
>> incomplete and too vague.
>>
>> To me this something that should have been clarified in the spec first 
>> and then the implementation brought into alignment. But that should 
>> have happened many years ago. Changing this now seems risky to me.
>>
>> This change in long standing behaviour also requires a CSR request if 
>> it is to proceed.
>>
>> David
>> -----
>>
>>
>> On 30/03/2018 8:36 AM, Daniil Titov wrote:
>>> Hi Serguei,
>>>
>>> Please review a new version of the fix that has these places corrected.
>>>
>>> Webreb: http://cr.openjdk.java.net/~dtitov/4613913/webrev.03
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-4613913
>>>
>>> Thanks!
>>>
>>> Best regards,
>>> Daniil
>>>
>>> On 3/29/18, 11:46 AM, "serguei.spitsyn at oracle.com" 
>>> <serguei.spitsyn at oracle.com> wrote:
>>>
>>>      Hi Daniil,
>>>           It looks good in general.
>>>      One minor comment is that it would be nice to make a cleanup
>>>      (as we already discussed) for all places like this:
>>>             202             if (isEnabled() || deleted) {
>>>        203                 throw invalidState();
>>>        204             }
>>>           As the isEnabled() now checks for deleted and throws the 
>>> invalidState()
>>>      then we can simplify these fragments to be:
>>>             202             if (isEnabled()) {
>>>        203                 throw invalidState();
>>>        204             }
>>>                Thanks,
>>>      Serguei
>>>                On 3/29/18 10:27, Daniil Titov wrote:
>>>      > Please review the changes that ensure that no operation on 
>>> deleted com.sun.jdi.request.EventRequest objects are permitted as per 
>>> JDI specification for 
>>> com.sun.jdi.request.EventRequestManager.deleteEventRequest(com.sun.jdi.request.EventRequest) 
>>> method.  The fix makes the following 4 methods in class 
>>> com.sun.tools.jdi. EventRequestManagerImpl$EventRequestImpl to throw 
>>> com.sun.jdi.request.InvalidRequestStateException if the request is 
>>> deleted:
>>>      >    - getProperty()
>>>      >    - putProperty(Object, Object)
>>>      >    - suspendPolicy()
>>>      >    - isEnabled()
>>>      >
>>>      > Bug: https://bugs.openjdk.java.net/browse/JDK-4613913
>>>      > Webrev: http://cr.openjdk.java.net/~dtitov/4613913/webrev.02/
>>>      >
>>>      > Best regards,
>>>      > Daniil
>>>      >
>>>      >
>>>
>>>
>