Move file-watching logic into .NET to avoid Node's fs.watch issues on…

… Windows (#128)
MarkPieszak · Jul 6, 2016 · Jul 6, 2016 · Jul 6, 2016 · Jul 6, 2016 · Jul 7, 2016
commit eec370e9389abc6ef32fc3b5109f398f75d023e6
diff --git a/src/Microsoft.AspNetCore.NodeServices/Configuration/Configuration.cs b/src/Microsoft.AspNetCore.NodeServices/Configuration/Configuration.cs
@@ -46,7 +46,7 @@ private static INodeInstance CreateNodeInstance(NodeServicesOptions options)
                 switch (options.HostingModel)
                 {
                     case NodeHostingModel.Http:
-                        return new HttpNodeInstance(options.ProjectPath, /* port */ 0, options.WatchFileExtensions);
+                        return new HttpNodeInstance(options.ProjectPath, options.WatchFileExtensions, /* port */ 0);
                     case NodeHostingModel.Socket:
                         var pipeName = "pni-" + Guid.NewGuid().ToString("D"); // Arbitrary non-clashing string
                         return new SocketNodeInstance(options.ProjectPath, options.WatchFileExtensions, pipeName);

diff --git a/src/Microsoft.AspNetCore.NodeServices/Content/Node/entrypoint-http.js b/src/Microsoft.AspNetCore.NodeServices/Content/Node/entrypoint-http.js
@@ -57,14 +57,9 @@
 	var http = __webpack_require__(2);
 	var path = __webpack_require__(3);
 	var ArgsUtil_1 = __webpack_require__(4);
-	var AutoQuit_1 = __webpack_require__(5);
 	// Webpack doesn't support dynamic requires for files not present at compile time, so grab a direct
 	// reference to Node's runtime 'require' function.
 	var dynamicRequire = eval('require');
-	var parsedArgs = ArgsUtil_1.parseArgs(process.argv);
-	if (parsedArgs.watch) {
-	    AutoQuit_1.autoQuitOnFileChange(process.cwd(), parsedArgs.watch.split(','));
-	}
 	var server = http.createServer(function (req, res) {
 	    readRequestBodyAsJson(req, function (bodyJson) {
 	        var hasSentResult = false;
@@ -117,6 +112,7 @@
 	        }
 	    });
 	});
+	var parsedArgs = ArgsUtil_1.parseArgs(process.argv);
 	var requestedPortOrZero = parsedArgs.port || 0; // 0 means 'let the OS decide'
 	server.listen(requestedPortOrZero, 'localhost', function () {
 	    // Signal to HttpNodeHost which port it should make its HTTP connections on
@@ -170,35 +166,5 @@
 	exports.parseArgs = parseArgs;
 
 
-/***/ },
-/* 5 */
-/***/ function(module, exports, __webpack_require__) {
-
-	"use strict";
-	var fs = __webpack_require__(6);
-	var path = __webpack_require__(3);
-	function autoQuitOnFileChange(rootDir, extensions) {
-	    // Note: This will only work on Windows/OS X, because the 'recursive' option isn't supported on Linux.
-	    // Consider using a different watch mechanism (though ideally without forcing further NPM dependencies).
-	    fs.watch(rootDir, { persistent: false, recursive: true }, function (event, filename) {
-	        var ext = path.extname(filename);
-	        if (extensions.indexOf(ext) >= 0) {
-	            console.log('Restarting due to file change: ' + filename);
-	            // Temporarily, the file-watching logic is in Node, so we signal it's time to restart by
-	            // sending the following message back to .NET. Soon the file-watching logic will move over
-	            // to the .NET side, and this whole file can be removed.
-	            console.log('[Microsoft.AspNetCore.NodeServices:Restart]');
-	        }
-	    });
-	}
-	exports.autoQuitOnFileChange = autoQuitOnFileChange;
-
-
-/***/ },
-/* 6 */
-/***/ function(module, exports) {
-
-	module.exports = require("fs");
-
 /***/ }
 /******/ ])));
diff --git a/src/Microsoft.AspNetCore.NodeServices/Content/Node/entrypoint-socket.js b/src/Microsoft.AspNetCore.NodeServices/Content/Node/entrypoint-socket.js
@@ -44,7 +44,7 @@
 /* 0 */
 /***/ function(module, exports, __webpack_require__) {
 
-	module.exports = __webpack_require__(7);
+	module.exports = __webpack_require__(5);
 
 
 /***/ },
@@ -83,54 +83,19 @@
 
 /***/ },
 /* 5 */
-/***/ function(module, exports, __webpack_require__) {
-
-	"use strict";
-	var fs = __webpack_require__(6);
-	var path = __webpack_require__(3);
-	function autoQuitOnFileChange(rootDir, extensions) {
-	    // Note: This will only work on Windows/OS X, because the 'recursive' option isn't supported on Linux.
-	    // Consider using a different watch mechanism (though ideally without forcing further NPM dependencies).
-	    fs.watch(rootDir, { persistent: false, recursive: true }, function (event, filename) {
-	        var ext = path.extname(filename);
-	        if (extensions.indexOf(ext) >= 0) {
-	            console.log('Restarting due to file change: ' + filename);
-	            // Temporarily, the file-watching logic is in Node, so we signal it's time to restart by
-	            // sending the following message back to .NET. Soon the file-watching logic will move over
-	            // to the .NET side, and this whole file can be removed.
-	            console.log('[Microsoft.AspNetCore.NodeServices:Restart]');
-	        }
-	    });
-	}
-	exports.autoQuitOnFileChange = autoQuitOnFileChange;
-
-
-/***/ },
-/* 6 */
-/***/ function(module, exports) {
-
-	module.exports = require("fs");
-
-/***/ },
-/* 7 */
 /***/ function(module, exports, __webpack_require__) {
 
 	"use strict";
 	// Limit dependencies to core Node modules. This means the code in this file has to be very low-level and unattractive,
 	// but simplifies things for the consumer of this module.
-	var net = __webpack_require__(8);
+	var net = __webpack_require__(6);
 	var path = __webpack_require__(3);
-	var readline = __webpack_require__(9);
+	var readline = __webpack_require__(7);
 	var ArgsUtil_1 = __webpack_require__(4);
-	var AutoQuit_1 = __webpack_require__(5);
-	var virtualConnectionServer = __webpack_require__(10);
+	var virtualConnectionServer = __webpack_require__(8);
 	// Webpack doesn't support dynamic requires for files not present at compile time, so grab a direct
 	// reference to Node's runtime 'require' function.
 	var dynamicRequire = eval('require');
-	var parsedArgs = ArgsUtil_1.parseArgs(process.argv);
-	if (parsedArgs.watch) {
-	    AutoQuit_1.autoQuitOnFileChange(process.cwd(), parsedArgs.watch.split(','));
-	}
 	// Signal to the .NET side when we're ready to accept invocations
 	var server = net.createServer().on('listening', function () {
 	    console.log('[Microsoft.AspNetCore.NodeServices:Listening]');
@@ -179,29 +144,30 @@
 	// Begin listening now. The underlying transport varies according to the runtime platform.
 	// On Windows it's Named Pipes; on Linux/OSX it's Domain Sockets.
 	var useWindowsNamedPipes = /^win/.test(process.platform);
+	var parsedArgs = ArgsUtil_1.parseArgs(process.argv);
 	var listenAddress = (useWindowsNamedPipes ? '\\\\.\\pipe\\' : '/tmp/') + parsedArgs.listenAddress;
 	server.listen(listenAddress);
 
 
 /***/ },
-/* 8 */
+/* 6 */
 /***/ function(module, exports) {
 
 	module.exports = require("net");
 
 /***/ },
-/* 9 */
+/* 7 */
 /***/ function(module, exports) {
 
 	module.exports = require("readline");
 
 /***/ },
-/* 10 */
+/* 8 */
 /***/ function(module, exports, __webpack_require__) {
 
 	"use strict";
-	var events_1 = __webpack_require__(11);
-	var VirtualConnection_1 = __webpack_require__(12);
+	var events_1 = __webpack_require__(9);
+	var VirtualConnection_1 = __webpack_require__(10);
 	// Keep this in sync with the equivalent constant in the .NET code. Both sides split up their transmissions into frames with this max length,
 	// and both will reject longer frames.
 	var MaxFrameBodyLength = 16 * 1024;
@@ -382,13 +348,13 @@
 
 
 /***/ },
-/* 11 */
+/* 9 */
 /***/ function(module, exports) {
 
 	module.exports = require("events");
 
 /***/ },
-/* 12 */
+/* 10 */
 /***/ function(module, exports, __webpack_require__) {
 
 	"use strict";
@@ -397,7 +363,7 @@
 	    function __() { this.constructor = d; }
 	    d.prototype = b === null ? Object.create(b) : (__.prototype = b.prototype, new __());
 	};
-	var stream_1 = __webpack_require__(13);
+	var stream_1 = __webpack_require__(11);
 	/**
 	 * Represents a virtual connection. Multiple virtual connections may be multiplexed over a single physical socket connection.
 	 */
@@ -438,7 +404,7 @@
 
 
 /***/ },
-/* 13 */
+/* 11 */
 /***/ function(module, exports) {
 
 	module.exports = require("stream");

diff --git a/src/Microsoft.AspNetCore.NodeServices/HostingModels/HttpNodeInstance.cs b/src/Microsoft.AspNetCore.NodeServices/HostingModels/HttpNodeInstance.cs
@@ -16,9 +16,6 @@ namespace Microsoft.AspNetCore.NodeServices.HostingModels
     /// port number is specified as a constructor parameter), and signals which port was selected using the same
     /// input/output-based mechanism that the base class uses to determine when the child process is ready to
     /// accept RPC invocations.
-    ///
-    /// TODO: Remove the file-watching logic from here and centralise it in OutOfProcessNodeInstance, implementing
-    /// the actual watching in .NET code (not Node), for consistency across platforms.
     /// </summary>
     /// <seealso cref="Microsoft.AspNetCore.NodeServices.HostingModels.OutOfProcessNodeInstance" />
     internal class HttpNodeInstance : OutOfProcessNodeInstance
@@ -35,26 +32,21 @@ internal class HttpNodeInstance : OutOfProcessNodeInstance
         private bool _disposed;
         private int _portNumber;
 
-        public HttpNodeInstance(string projectPath, int port = 0, string[] watchFileExtensions = null)
+        public HttpNodeInstance(string projectPath, string[] watchFileExtensions, int port = 0)
             : base(
                 EmbeddedResourceReader.Read(
                     typeof(HttpNodeInstance),
                     "/Content/Node/entrypoint-http.js"),
                 projectPath,
-                MakeCommandLineOptions(port, watchFileExtensions))
+                watchFileExtensions,
+                MakeCommandLineOptions(port))
         {
             _client = new HttpClient();
 		}
 
-        private static string MakeCommandLineOptions(int port, string[] watchFileExtensions)
+        private static string MakeCommandLineOptions(int port)
         {
-            var result = "--port " + port;
-            if (watchFileExtensions != null && watchFileExtensions.Length > 0)
-            {
-                result += " --watch " + string.Join(",", watchFileExtensions);
-            }
-
-            return result;
+            return $"--port {port}";
         }
 
         protected override async Task<T> InvokeExportAsync<T>(NodeInvocationInfo invocationInfo)

diff --git a/src/Microsoft.AspNetCore.NodeServices/HostingModels/OutOfProcessNodeInstance.cs b/src/Microsoft.AspNetCore.NodeServices/HostingModels/OutOfProcessNodeInstance.cs
@@ -1,6 +1,7 @@
 using System;
 using System.Diagnostics;
 using System.IO;
+using System.Linq;
 using System.Threading.Tasks;
 
 namespace Microsoft.AspNetCore.NodeServices.HostingModels
@@ -18,17 +19,24 @@ namespace Microsoft.AspNetCore.NodeServices.HostingModels
     public abstract class OutOfProcessNodeInstance : INodeInstance
     {
         private const string ConnectionEstablishedMessage = "[Microsoft.AspNetCore.NodeServices:Listening]";
-        private const string NeedsRestartMessage = "[Microsoft.AspNetCore.NodeServices:Restart]";
         private readonly TaskCompletionSource<object> _connectionIsReadySource = new TaskCompletionSource<object>();
         private bool _disposed;
         private readonly StringAsTempFile _entryPointScript;
+        private FileSystemWatcher _fileSystemWatcher;
         private readonly Process _nodeProcess;
         private bool _nodeProcessNeedsRestart;
+        private readonly string[] _watchFileExtensions;
 
-        public OutOfProcessNodeInstance(string entryPointScript, string projectPath, string commandLineArguments = null)
+        public OutOfProcessNodeInstance(
+            string entryPointScript,
+            string projectPath,
+            string[] watchFileExtensions,
+            string commandLineArguments)
         {
             _entryPointScript = new StringAsTempFile(entryPointScript);
             _nodeProcess = LaunchNodeProcess(_entryPointScript.FileName, projectPath, commandLineArguments);
+            _watchFileExtensions = watchFileExtensions;
+            _fileSystemWatcher = BeginFileWatcher(projectPath);
             ConnectToInputOutputStreams();
         }
 
@@ -80,6 +88,7 @@ protected virtual void Dispose(bool disposing)
                 if (disposing)
                 {
                     _entryPointScript.Dispose();
+                    EnsureFileSystemWatcherIsDisposed();
                 }
 
                 // Make sure the Node process is finished
@@ -93,6 +102,15 @@ protected virtual void Dispose(bool disposing)
             }
         }
 
+        private void EnsureFileSystemWatcherIsDisposed()
+        {
+            if (_fileSystemWatcher != null)
+            {
+                _fileSystemWatcher.Dispose();
+                _fileSystemWatcher = null;
+            }
+        }
+
         private static Process LaunchNodeProcess(string entryPointFilename, string projectPath, string commandLineArguments)
         {
             var startInfo = new ProcessStartInfo("node")
@@ -143,13 +161,6 @@ private void ConnectToInputOutputStreams()
                     _connectionIsReadySource.SetResult(null);
                     initializationIsCompleted = true;
                 }
-                else if (evt.Data == NeedsRestartMessage)
-                {
-                    // Temporarily, the file-watching logic is in Node, so look out for the
-                    // signal that we need to restart. This can be removed once the file-watching
-                    // logic is moved over to the .NET side.
-                    _nodeProcessNeedsRestart = true;
-                }
                 else if (evt.Data != null)
                 {
                     OnOutputDataReceived(evt.Data);
@@ -177,6 +188,69 @@ private void ConnectToInputOutputStreams()
             _nodeProcess.BeginErrorReadLine();
         }
 
+        private FileSystemWatcher BeginFileWatcher(string rootDir)
+        {
+            if (_watchFileExtensions == null || _watchFileExtensions.Length == 0)
+            {
+                // Nothing to watch
+                return null;
+            }
+
+            var watcher = new FileSystemWatcher(rootDir)
+            {
+                IncludeSubdirectories = true,
+                NotifyFilter = NotifyFilters.LastWrite | NotifyFilters.FileName | NotifyFilters.DirectoryName
+            };
+            watcher.Changed += OnFileChanged;
+            watcher.Created += OnFileChanged;
+            watcher.Deleted += OnFileChanged;
+            watcher.Renamed += OnFileRenamed;
+            watcher.EnableRaisingEvents = true;
+            return watcher;
+        }
+
+        private void OnFileChanged(object source, FileSystemEventArgs e)
+        {
+            if (IsFilenameBeingWatched(e.FullPath))
+            {
+                RestartDueToFileChange(e.FullPath);
+            }
+        }
+
+        private void OnFileRenamed(object source, RenamedEventArgs e)
+        {
+            if (IsFilenameBeingWatched(e.OldFullPath) || IsFilenameBeingWatched(e.FullPath))
+            {
+                RestartDueToFileChange(e.OldFullPath);
+            }
+        }
+
+        private bool IsFilenameBeingWatched(string fullPath)
+        {
+            if (string.IsNullOrEmpty(fullPath))
+            {
+                return false;
+            }
+            else
+            {
+                var actualExtension = Path.GetExtension(fullPath) ?? string.Empty;
+                return _watchFileExtensions.Any(actualExtension.Equals);
+            }
+        }
+
+        private void RestartDueToFileChange(string fullPath)
+        {
+            // TODO: Use proper logger
+            Console.WriteLine($"Node will restart because file changed: {fullPath}");
+
+            _nodeProcessNeedsRestart = true;
+
+            // There's no need to watch for any more changes, since we're already restarting, and if the
+            // restart takes some time (e.g., due to connection draining), we could end up getting duplicate
+            // notifications.
+            EnsureFileSystemWatcherIsDisposed();
+        }
+
         ~OutOfProcessNodeInstance()
         {
             Dispose(false);