[PATCH] 8153925: WindowsWatchService hangs on GetOverlappedResult and locks directory

[Alex Kashchenko]

Hi Alan,

On 04/11/2016 04:38 PM, Alan Bateman wrote:
> On 10/04/2016 11:30, Alex Kashchenko wrote:
>> :
>>
>> The issue appeared during the deployment of a WAR from Eclipse+JBoss
>> Tools to local WildFly instance. It caused deployment errors with a
>> message: "Error renaming [tmp/some_file] to [app.war/some_file]"
>>
>> It was tracked down to the directory locked infinitely by the
>> WatchService with the following trace of the poller thread:
>>
>>     WindowsNativeDispatcher.GetOverlappedResult(long, long) line: not
>> available [native method]
>>
>> WindowsWatchService$Poller.releaseResources(WindowsWatchService$WindowsWatchKey)
>> line: 460
>> WindowsWatchService$Poller.access$100(WindowsWatchService$Poller,
>> WindowsWatchService$WindowsWatchKey) line: 244
>>     WindowsWatchService$WindowsWatchKey.invalidate() line: 180
>>     WindowsWatchService$Poller.implCancelKey(WatchKey) line: 495
>>     WindowsWatchService$Poller.run() line: 634
>>     Thread.run() line: 745
>>
>> From ProcMon it was noticed that just before this call poller thread
>> calls ReadDirectoryChangesW that fails with "DELETE_PENDING" status.
>> It looked like in this case overlapped I/O operation failed to start,
>> but poller thread called GetOverlappedResult anyway causing the
>> situation described here:
>> https://blogs.msdn.microsoft.com/oldnewthing/20110303-00/?p=11313
>>
>> The patch tries to detect such situation and avoid calling
>> GetOverlappedResult.
> I've looked at the issue and I agree with your analysis. When
> ReadDirectoryChangesW fails I assume the CancelIo can be skipped too as
> the ReadDirectoryChangesW is the only I/O operation.

Thanks for looking into this. I'll change the patch to skip CancelIo 
call too.

> It would be good if we could get your reproducer into the webrev, this
> means a bit of clean-up and getting consistent with the existing tests.
> I would prefer not that it not be Windows specific as it is useful to
> exercise this type of "interference" issue on other platforms too. The
> simplest is to just let the test hang when running on a non-patched JDK
> and the test harness with cause it to fail when the timeout is reached.
> If possible then changing the test to use the Path/Files API to be
> consistent with the other tests in this area would be good good.
>
> I realize you are interested in jdk8u-dev but we will need to get this
> into JDK 9 first (see JDK 8 Updates Ground Rules [1]). It's okay if your
> patch is against jdk8u-dev for testing purposes but we'll just re-base
> it against jdk9/dev before pushing.

I'll do the cleanup/changes to the test and will include it into 
jdk/test/java/nio/file/WatchService directory and will prepare a webrev 
for jdk8u-dev. I'll proceed with this after April CPUs will be out.


-- 
-Alex