[2019-08] [System] Make FileSystemWatcher backend non-static (#16926)

monojenkins · web-flow · commit 75eb342f5321 · 2019-09-19T14:45:54.000-04:00
[2019-08] [System] Make FileSystemWatcher backend non-static The FileSystemWatcher `watcher` field is the backend IFileWatcher that's supposed to be used for the actual operations. Each backend has a `bool GetInstance (out watcher)` method that returns a singleton object, so each new instance of FileSystemWatcher gets the same IFileWatcher instance. (They will get different `watcher_handle` instances which are used by some backends to uniquely identify this FileSystemWatcher). However when the first FileSystemWatcher instance is `Dispose`d, it will set `watcher = null` which will prevent the remaining instances from calling watcher?.StopDispatching (watcher_handle); watcher?.Dispose (watcher_handle); which means that the backend won't properly cleanup resources for any other FSW instances that have other `watcher_handle` values. Addresses #16709  Backport of #16922. /cc @steveisok @lambdageek
diff --git a/mcs/class/System/System.IO/FileSystemWatcher.cs b/mcs/class/System/System.IO/FileSystemWatcher.cs
@@ -61,7 +61,7 @@ public class FileSystemWatcher : Component, ISupportInitialize {
 		SearchPattern2 pattern;
 		bool disposed;
 		string mangledFilter;
-		static IFileWatcher watcher;
+		IFileWatcher watcher;
 		object watcher_handle;
 		static object lockobj = new object ();
 
diff --git a/mcs/class/System/Test/System.IO/FileSystemWatcherTest.cs b/mcs/class/System/Test/System.IO/FileSystemWatcherTest.cs
@@ -11,6 +11,7 @@
 using NUnit.Framework;
 using System;
 using System.IO;
+using System.Reflection;
 
 using MonoTests.Helpers;
 
@@ -109,7 +110,72 @@ public void LargeNumberOfInstances ()
 				}
 			}
 		}
+
+		[Test]
+		public void CreateTwoAndDispose ()
+		{
+			// Create two FSW instances and dispose them.  Verify
+			// that the backend IFileWatcher's Dispose
+			// (watcher_handle) method got called.
+
+			// FIXME: This only works for the
+			// CoreFXFileSystemWatcherProxy not the other backends.
+
+			using (var tmp = new TempDirectory ()) {
+				// have to use reflection to poke at the private fields of FileSystemWatcher.
+				var watcherField = typeof (FileSystemWatcher).GetField ("watcher", BindingFlags.Instance | BindingFlags.NonPublic);
+				Assert.IsNotNull (watcherField);
+				var watcherHandleField = typeof (FileSystemWatcher).GetField ("watcher_handle", BindingFlags.Instance | BindingFlags.NonPublic);
+				Assert.IsNotNull (watcherHandleField);
+				var proxyType = typeof (FileSystemWatcher).Assembly.GetType ("System.IO.CoreFXFileSystemWatcherProxy");
+				Assert.IsNotNull (proxyType);
+
+				var fsw1 = new FileSystemWatcher (tmp.Path, "*");
+				var fsw2 = new FileSystemWatcher (tmp.Path, "*");
+
+				object handle1 = null;
+				object handle2 = null;
+
+				// using "using" to ensure that Dispose gets called even if we throw an exception
+				using (var fsw11 = fsw1)
+				using (var fsw22 = fsw2) {
+					// at this point watcher and watcher_handle should be set
+
+					var watcher = watcherField.GetValue (fsw1);
+					Assert.IsNotNull (watcher);
+					if (!proxyType.IsAssignableFrom (watcher.GetType ()))
+						Assert.Ignore ("Testing only CoreFXFileSystemWatcherProxy FSW backend");
+
+					handle1 = watcherHandleField.GetValue (fsw1);
+					handle2 = watcherHandleField.GetValue (fsw2);
+
+					Assert.IsNotNull (handle1);
+					Assert.IsNotNull (handle2);
+
+				}
+
+				// Dispose was called, now watcher_handle should be null
+
+				Assert.IsNull (watcherHandleField.GetValue (fsw1));
+				Assert.IsNull (watcherHandleField.GetValue (fsw2));
+
+				// and moreover, the CoreFXFileSystemWatcherProxy shouldn't have entries for either handle1 or handle2
+
+				var proxyTypeInternalMapField = proxyType.GetField ("internal_map", BindingFlags.Static | BindingFlags.NonPublic);
+				Assert.IsNotNull (proxyTypeInternalMapField);
+				var internal_map = proxyTypeInternalMapField.GetValue (null)
+					as global::System.Collections.Generic.IDictionary<object, global::System.IO.CoreFX.FileSystemWatcher>;
+				Assert.IsNotNull (internal_map);
+
+				// This pair are the critical checks: after we call Dispose on fsw1 and fsw2, the
+				// backend's internal map shouldn't have anything keyed on handle1 and handle2.
+				// Therefore System.IO.CoreFX.FileSystemWatcher instances will be disposed of, too.
+				Assert.IsFalse (internal_map.ContainsKey (handle1));
+				Assert.IsFalse (internal_map.ContainsKey (handle2));
+			}
+		}
+
 	}
 }
 
-#endif
+#endif
