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