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

[Daniil Titov]

Hello,

Based on the discussion below I am canceling this review. The issue will be addressed by changing the test that resides out of the open repository.

Thanks!

Best regards,
Daniil

On 4/2/18, 3:02 PM, "David Holmes" <david.holmes at oracle.com> wrote:

    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
    >>>      >
    >>>      >
    >>>
    >>>
    >