RFR: 8145981 (fs) LinuxWatchService can reports events against wrong directory

[Alan Bateman]

On 28/12/2015 18:51, Sebastian Sickelmann wrote:
> Hi Alan,
>
> i am not fully sure, but i think your patch does not change anything.
It moves the handling of requests to after the processing of the inotify 
data. The ordering doesn't matter and okay to process some events even 
if there is a queued request to cancel or shutdown.

>
> But I think I understand what you have written in the comment. I think
> there is a third possible solution to the two you mentioned.
>
> I was not sure, if it was done intentionally (shutdown before
> processing) so I have keeped the order of processing and shutdown and
> only resorted the order of the reading in the special case.
>
> Your suggested solution may be easier to read and maintain. What do you
> think?
I think the simplest is to just move the handling of requests. Your fix 
will work too but it means that the update/shutdown requests would be 
handled in two places and so just a little harder to maintain.

>
> http://cr.openjdk.java.net/~sebastian/8145981/webrev.00/
>
> The backporting to older releases should be straight forward and I will
> produce changsets for those when we are ready with the review for jdk9.
We need to agree the changes for jdk9/dev first, then we can think about 
jdk8u-dev after the changes have baked in jdk9/dev for a while. In this 
case then the change should be fine but the test might need test to bed 
down. The test needs to prove that it is stable on all platforms for 
example.

>
> On my box it takes 20-30 seconds to tickle the bug. After the fix i had
> ran the test for about 10 minutes.
>
> Do you have any good idea how to make the test more robust, so that an
> regression could be detected on all your linux boxes in a short time?
>
The test currently takes ~110s and would be good to get it down to <30s 
to avoid it slowing down the test runs. Your updated tests seems to fail 
for me in 10-20s on two machines that I tried so we might not have to do 
too much to do. Sometimes it can be useful to introduce noise or have 
the test running concurrently in a thread pool to provoke the issue 
quicker but it might not be needed here.

A few other things on the test:

1. It would be good to choose a name for the test that is consistent 
with the naming in this area. UpdateInterference might work, you might 
have other ideas.

2. In the jtreg tags then I assume @library is not needed (I can't see 
it using any the test infrastructure). Also the /timeout=120 is not 
needed either.

3. It would be good to keep the style consistent if you can. If line 
lengths are too long then it makes it hard to do side-by-side diffs for 
example. If you static import StandardWatchEventKinds.* then it should 
reduce some of the lines.

-Alan