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

[Sebastian Sickelmann]

On 12/29/2015 09:55 AM, Alan Bateman wrote:
>
> 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.
Yes. That is what i understand from your JBS comment. But maybe i missed
the right patch attachment in JBS.
>
>>
>> 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.
I changed it to your solution. You are right the order of processing
pending events and new events from inotify do not need any guarantee. So
processing pending events (and shutdown) can be done later.

>>
>> 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
I updated the webrev and tried to reduce the time in the test by
generating more watch-keys. On my machine I am now always below 10 secs
and in most cases below 5 sec.

http://cr.openjdk.java.net/~sebastian/8145981/webrev.01/

--
Sebastian