Re: RFR 4613913: Four EventRequest methods are invokable on deleted request

[serguei.spitsyn]

Hi David,

Somehow I can see your message from my smart phone only...
Thank you for for confirming that you are agree with this approach!

Thanks,
Serguei

Sent from my Verizon Wireless 4G LTE smartphone


-------- Original message --------
From: David Holmes <david.holmes at oracle.com> 
Date: 04/02/2018  15:02  (GMT-08:00) 
To: serguei.spitsyn at oracle.com, Daniil Titov <daniil.x.titov at oracle.com>, serviceability-dev at openjdk.java.net 
Subject: Re: RFR 4613913: Four EventRequest methods are invokable on deleted
  request 

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
>>>      >
>>>      >
>>>
>>>
> 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20180402/93d69d35/attachment-0001.html>