8029516: (fs) WatchKey cancel unreliable on Windows
Reviewed-by: chegar
diff --git a/jdk/src/windows/classes/sun/nio/fs/WindowsNativeDispatcher.java b/jdk/src/windows/classes/sun/nio/fs/WindowsNativeDispatcher.java
index a380c3d..47d28f8 100644
--- a/jdk/src/windows/classes/sun/nio/fs/WindowsNativeDispatcher.java
+++ b/jdk/src/windows/classes/sun/nio/fs/WindowsNativeDispatcher.java
@@ -37,6 +37,17 @@
private WindowsNativeDispatcher() { }
/**
+ * HANDLE CreateEvent(
+ * LPSECURITY_ATTRIBUTES lpEventAttributes,
+ * BOOL bManualReset,
+ * BOOL bInitialState,
+ * PCTSTR lpName
+ * );
+ */
+ static native long CreateEvent(boolean bManualReset, boolean bInitialState)
+ throws WindowsException;
+
+ /**
* HANDLE CreateFile(
* LPCTSTR lpFileName,
* DWORD dwDesiredAccess,
@@ -1041,6 +1052,25 @@
long pOverlapped)
throws WindowsException;
+
+ /**
+ * CancelIo(
+ * HANDLE hFile
+ * )
+ */
+ static native void CancelIo(long hFile) throws WindowsException;
+
+ /**
+ * GetOverlappedResult(
+ * HANDLE hFile,
+ * LPOVERLAPPED lpOverlapped,
+ * LPDWORD lpNumberOfBytesTransferred,
+ * BOOL bWait
+ * );
+ */
+ static native int GetOverlappedResult(long hFile, long lpOverlapped)
+ throws WindowsException;
+
/**
* BackupRead(
* HANDLE hFile,
diff --git a/jdk/src/windows/classes/sun/nio/fs/WindowsWatchService.java b/jdk/src/windows/classes/sun/nio/fs/WindowsWatchService.java
index 4ad1024..cf3a6de 100644
--- a/jdk/src/windows/classes/sun/nio/fs/WindowsWatchService.java
+++ b/jdk/src/windows/classes/sun/nio/fs/WindowsWatchService.java
@@ -25,9 +25,16 @@
package sun.nio.fs;
-import java.nio.file.*;
import java.io.IOException;
-import java.util.*;
+import java.nio.file.NotDirectoryException;
+import java.nio.file.Path;
+import java.nio.file.StandardWatchEventKinds;
+import java.nio.file.WatchEvent;
+import java.nio.file.WatchKey;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Set;
+
import com.sun.nio.file.ExtendedWatchEventModifier;
import sun.misc.Unsafe;
@@ -42,7 +49,6 @@
extends AbstractWatchService
{
private final static int WAKEUP_COMPLETION_KEY = 0;
- private final Unsafe unsafe = Unsafe.getUnsafe();
// background thread to service I/O completion port
private final Poller poller;
@@ -82,7 +88,7 @@
/**
* Windows implementation of WatchKey.
*/
- private class WindowsWatchKey extends AbstractWatchKey {
+ private static class WindowsWatchKey extends AbstractWatchKey {
// file key (used to detect existing registrations)
private final FileKey fileKey;
@@ -169,15 +175,9 @@
return completionKey;
}
- // close directory and release buffer
- void releaseResources() {
- CloseHandle(handle);
- buffer.cleaner().clean();
- }
-
- // Invalidate key by closing directory and releasing buffer
+ // Invalidate the key, assumes that resources have been released
void invalidate() {
- releaseResources();
+ ((WindowsWatchService)watcher()).poller.releaseResources(this);
handle = INVALID_HANDLE_VALUE;
buffer = null;
countAddress = 0;
@@ -193,7 +193,7 @@
public void cancel() {
if (isValid()) {
// delegate to poller
- poller.cancel(this);
+ ((WindowsWatchService)watcher()).poller.cancel(this);
}
}
}
@@ -241,18 +241,25 @@
/**
* Background thread to service I/O completion port.
*/
- private class Poller extends AbstractPoller {
+ private static class Poller extends AbstractPoller {
+ private final static Unsafe UNSAFE = Unsafe.getUnsafe();
+
/*
* typedef struct _OVERLAPPED {
- * DWORD Internal;
- * DWORD InternalHigh;
- * DWORD Offset;
- * DWORD OffsetHigh;
- * HANDLE hEvent;
+ * ULONG_PTR Internal;
+ * ULONG_PTR InternalHigh;
+ * union {
+ * struct { DWORD Offset; DWORD OffsetHigh; };
+ * PVOID Pointer;
+ * };
+ * HANDLE hEvent;
* } OVERLAPPED;
*/
private static final short SIZEOF_DWORD = 4;
private static final short SIZEOF_OVERLAPPED = 32; // 20 on 32-bit
+ private static final short OFFSETOF_HEVENT =
+ (UNSAFE.addressSize() == 4) ? (short) 16 : 24;
+
/*
* typedef struct _FILE_NOTIFY_INFORMATION {
@@ -276,10 +283,10 @@
private final long port;
// maps completion key to WatchKey
- private final Map<Integer,WindowsWatchKey> ck2key;
+ private final Map<Integer, WindowsWatchKey> ck2key;
// maps file key to WatchKey
- private final Map<FileKey,WindowsWatchKey> fk2key;
+ private final Map<FileKey, WindowsWatchKey> fk2key;
// unique completion key for each directory
// native completion key capacity is 64 bits on Win64.
@@ -393,8 +400,13 @@
long overlappedAddress = bufferAddress + size - SIZEOF_OVERLAPPED;
long countAddress = overlappedAddress - SIZEOF_DWORD;
+ // zero the overlapped structure
+ UNSAFE.setMemory(overlappedAddress, SIZEOF_OVERLAPPED, (byte)0);
+
// start async read of changes to directory
try {
+ createAndAttachEvent(overlappedAddress);
+
ReadDirectoryChangesW(handle,
bufferAddress,
CHANGES_BUFFER_SIZE,
@@ -403,6 +415,7 @@
countAddress,
overlappedAddress);
} catch (WindowsException x) {
+ closeAttachedEvent(overlappedAddress);
buffer.release();
return new IOException(x.getMessage());
}
@@ -421,7 +434,7 @@
// 2. release existing key's resources (handle/buffer)
// 3. re-initialize key with new handle/buffer
ck2key.remove(existing.completionKey());
- existing.releaseResources();
+ releaseResources(existing);
watchKey = existing.init(handle, events, watchSubtree, buffer,
countAddress, overlappedAddress, completionKey);
}
@@ -436,6 +449,42 @@
}
}
+ /**
+ * Cancels the outstanding I/O operation on the directory
+ * associated with the given key and releases the associated
+ * resources.
+ */
+ private void releaseResources(WindowsWatchKey key) {
+ try {
+ CancelIo(key.handle());
+ GetOverlappedResult(key.handle(), key.overlappedAddress());
+ } catch (WindowsException expected) {
+ // expected as I/O operation has been cancelled
+ }
+ CloseHandle(key.handle());
+ closeAttachedEvent(key.overlappedAddress());
+ key.buffer().cleaner().clean();
+ }
+
+ /**
+ * Creates an unnamed event and set it as the hEvent field
+ * in the given OVERLAPPED structure
+ */
+ private void createAndAttachEvent(long ov) throws WindowsException {
+ long hEvent = CreateEvent(false, false);
+ UNSAFE.putAddress(ov + OFFSETOF_HEVENT, hEvent);
+ }
+
+ /**
+ * Closes the event attached to the given OVERLAPPED structure. A
+ * no-op if there isn't an event attached.
+ */
+ private void closeAttachedEvent(long ov) {
+ long hEvent = UNSAFE.getAddress(ov + OFFSETOF_HEVENT);
+ if (hEvent != 0 && hEvent != INVALID_HANDLE_VALUE)
+ CloseHandle(hEvent);
+ }
+
// cancel single key
@Override
void implCancelKey(WatchKey obj) {
@@ -451,9 +500,8 @@
@Override
void implCloseAll() {
// cancel all keys
- for (Map.Entry<Integer, WindowsWatchKey> entry: ck2key.entrySet()) {
- entry.getValue().invalidate();
- }
+ ck2key.values().forEach(WindowsWatchKey::invalidate);
+
fk2key.clear();
ck2key.clear();
@@ -462,8 +510,7 @@
}
// Translate file change action into watch event
- private WatchEvent.Kind<?> translateActionToEvent(int action)
- {
+ private WatchEvent.Kind<?> translateActionToEvent(int action) {
switch (action) {
case FILE_ACTION_MODIFIED :
return StandardWatchEventKinds.ENTRY_MODIFY;
@@ -487,18 +534,18 @@
int nextOffset;
do {
- int action = unsafe.getInt(address + OFFSETOF_ACTION);
+ int action = UNSAFE.getInt(address + OFFSETOF_ACTION);
// map action to event
WatchEvent.Kind<?> kind = translateActionToEvent(action);
if (key.events().contains(kind)) {
// copy the name
- int nameLengthInBytes = unsafe.getInt(address + OFFSETOF_FILENAMELENGTH);
+ int nameLengthInBytes = UNSAFE.getInt(address + OFFSETOF_FILENAMELENGTH);
if ((nameLengthInBytes % 2) != 0) {
- throw new AssertionError("FileNameLength.FileNameLength is not a multiple of 2");
+ throw new AssertionError("FileNameLength is not a multiple of 2");
}
char[] nameAsArray = new char[nameLengthInBytes/2];
- unsafe.copyMemory(null, address + OFFSETOF_FILENAME, nameAsArray,
+ UNSAFE.copyMemory(null, address + OFFSETOF_FILENAME, nameAsArray,
Unsafe.ARRAY_CHAR_BASE_OFFSET, nameLengthInBytes);
// create FileName and queue event
@@ -508,7 +555,7 @@
}
// next event
- nextOffset = unsafe.getInt(address + OFFSETOF_NEXTENTRYOFFSET);
+ nextOffset = UNSAFE.getInt(address + OFFSETOF_NEXTENTRYOFFSET);
address += (long)nextOffset;
} while (nextOffset != 0);
}
diff --git a/jdk/src/windows/native/sun/nio/fs/WindowsNativeDispatcher.c b/jdk/src/windows/native/sun/nio/fs/WindowsNativeDispatcher.c
index 94e060f..8075d04 100644
--- a/jdk/src/windows/native/sun/nio/fs/WindowsNativeDispatcher.c
+++ b/jdk/src/windows/native/sun/nio/fs/WindowsNativeDispatcher.c
@@ -195,6 +195,17 @@
}
}
+JNIEXPORT jlong JNICALL
+Java_sun_nio_fs_WindowsNativeDispatcher_CreateEvent(JNIEnv* env, jclass this,
+ jboolean bManualReset, jboolean bInitialState)
+{
+ HANDLE hEvent = CreateEventW(NULL, bManualReset, bInitialState, NULL);
+ if (hEvent == NULL) {
+ throwWindowsException(env, GetLastError());
+ }
+ return ptr_to_jlong(hEvent);
+}
+
JNIEXPORT jstring JNICALL
Java_sun_nio_fs_WindowsNativeDispatcher_FormatMessage(JNIEnv* env, jclass this, jint errorCode) {
WCHAR message[255];
@@ -1230,23 +1241,38 @@
}
JNIEXPORT void JNICALL
+Java_sun_nio_fs_WindowsNativeDispatcher_CancelIo(JNIEnv* env, jclass this, jlong hFile) {
+ if (CancelIo((HANDLE)jlong_to_ptr(hFile)) == 0) {
+ throwWindowsException(env, GetLastError());
+ }
+}
+
+JNIEXPORT jint JNICALL
+Java_sun_nio_fs_WindowsNativeDispatcher_GetOverlappedResult(JNIEnv *env, jclass this,
+ jlong hFile, jlong lpOverlapped)
+{
+ BOOL res;
+ DWORD bytesTransferred = -1;
+
+ res = GetOverlappedResult((HANDLE)jlong_to_ptr(hFile),
+ (LPOVERLAPPED)jlong_to_ptr(lpOverlapped),
+ &bytesTransferred,
+ TRUE);
+ if (res == 0) {
+ throwWindowsException(env, GetLastError());
+ }
+
+ return (jint)bytesTransferred;
+}
+
+JNIEXPORT void JNICALL
Java_sun_nio_fs_WindowsNativeDispatcher_ReadDirectoryChangesW(JNIEnv* env, jclass this,
jlong hDirectory, jlong bufferAddress, jint bufferLength, jboolean watchSubTree, jint filter,
jlong bytesReturnedAddress, jlong pOverlapped)
{
BOOL res;
BOOL subtree = (watchSubTree == JNI_TRUE) ? TRUE : FALSE;
-
- /* Any unused members of [OVERLAPPED] structure should always be initialized to zero
- before the structure is used in a function call.
- Otherwise, the function may fail and return ERROR_INVALID_PARAMETER.
- http://msdn.microsoft.com/en-us/library/windows/desktop/ms684342%28v=vs.85%29.aspx
-
- The [Offset] and [OffsetHigh] members of this structure are not used.
- http://msdn.microsoft.com/en-us/library/windows/desktop/aa365465%28v=vs.85%29.aspx
-
- [hEvent] should be zero, other fields are the return values. */
- ZeroMemory((LPOVERLAPPED)jlong_to_ptr(pOverlapped), sizeof(OVERLAPPED));
+ LPOVERLAPPED ov = (LPOVERLAPPED)jlong_to_ptr(pOverlapped);
res = ReadDirectoryChangesW((HANDLE)jlong_to_ptr(hDirectory),
(LPVOID)jlong_to_ptr(bufferAddress),
diff --git a/jdk/test/java/nio/file/WatchService/LotsOfCancels.java b/jdk/test/java/nio/file/WatchService/LotsOfCancels.java
new file mode 100644
index 0000000..cb7f9ef
--- /dev/null
+++ b/jdk/test/java/nio/file/WatchService/LotsOfCancels.java
@@ -0,0 +1,119 @@
+/*
+ * Copyright (c) 2014, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/* @test
+ * @bug 8029516
+ * @summary Bash on WatchKey.cancel with a view to causing a crash when
+ * an outstanding I/O operation on directory completes after the
+ * directory has been closed
+ */
+
+import java.nio.file.ClosedWatchServiceException;
+import java.nio.file.FileSystems;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.WatchKey;
+import java.nio.file.WatchService;
+import static java.nio.file.StandardWatchEventKinds.*;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+
+public class LotsOfCancels {
+
+ // set to true for any exceptions
+ static volatile boolean failed;
+
+ public static void main(String[] args) throws Exception {
+
+ // create a bunch of directories. Create two tasks for each directory,
+ // one to bash on cancel, the other to poll the events
+ ExecutorService pool = Executors.newCachedThreadPool();
+ try {
+ Path top = Files.createTempDirectory("work");
+ top.toFile().deleteOnExit();
+ for (int i=1; i<=16; i++) {
+ Path dir = Files.createDirectory(top.resolve("dir-" + i));
+ WatchService watcher = FileSystems.getDefault().newWatchService();
+ pool.submit(() -> handle(dir, watcher));
+ pool.submit(() -> poll(watcher));
+ }
+ } finally {
+ pool.shutdown();
+ }
+
+ // give thread pool lots of time to terminate
+ if (!pool.awaitTermination(5L, TimeUnit.MINUTES))
+ throw new RuntimeException("Thread pool did not terminate");
+
+ if (failed)
+ throw new RuntimeException("Test failed, see log for details");
+ }
+
+ /**
+ * Stress the given WatchService, specifically the cancel method, in
+ * the given directory. Closes the WatchService when done.
+ */
+ static void handle(Path dir, WatchService watcher) {
+ try {
+ try {
+ Path file = dir.resolve("anyfile");
+ for (int i=0; i<2000; i++) {
+ WatchKey key = dir.register(watcher, ENTRY_CREATE, ENTRY_DELETE);
+ Files.createFile(file);
+ Files.delete(file);
+ key.cancel();
+ }
+ } finally {
+ watcher.close();
+ }
+ } catch (Exception e) {
+ e.printStackTrace();
+ failed = true;
+ }
+ }
+
+ /**
+ * Polls the given WatchService in a tight loop. This keeps the event
+ * queue drained, it also hogs a CPU core which seems necessary to
+ * tickle the original bug.
+ */
+ static void poll(WatchService watcher) {
+ try {
+ for (;;) {
+ WatchKey key = watcher.take();
+ if (key != null) {
+ key.pollEvents();
+ key.reset();
+ }
+ }
+ } catch (ClosedWatchServiceException expected) {
+ // nothing to do
+ } catch (Exception e) {
+ e.printStackTrace();
+ failed = true;
+ }
+ }
+
+}
+
