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
- 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?
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, "[email protected]"
<[email protected]> 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
>
>
|
