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.hol...@oracle.com> wrote: Hi Serguei, On 3/04/2018 7:44 AM, serguei.spit...@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.spit...@oracle.com" >>> <serguei.spit...@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 >>> > >>> > >>> >>> >