Move file-watching logic into .NET to avoid Node's fs.watch issues on Windows (#128)

src/Microsoft.AspNetCore.NodeServices/Configuration/Configuration.cs

@@ -46,7 +46,7 @@ namespace Microsoft.AspNetCore.NodeServices
                 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);

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");
-
 /***/ }
 /******/ ])));

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__(8);
-	var virtualConnectionServer = __webpack_require__(10);
 	// 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 events_1 = __webpack_require__(9);
-	var VirtualConnection_1 = __webpack_require__(12);
+	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");

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 @@ namespace Microsoft.AspNetCore.NodeServices.HostingModels
         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;
+            return $"--port {port}";
-            if (watchFileExtensions != null && watchFileExtensions.Length > 0)
-            {
-                result += " --watch " + string.Join(",", watchFileExtensions);
-            }
-
-            return result;
         }
 
         protected override async Task<T> InvokeExportAsync<T>(NodeInvocationInfo invocationInfo)

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 @@ namespace Microsoft.AspNetCore.NodeServices.HostingModels
                 if (disposing)
                 {
                     _entryPointScript.Dispose();
+                    EnsureFileSystemWatcherIsDisposed();
                 }
 
                 // Make sure the Node process is finished
@@ -93,6 +102,15 @@ namespace Microsoft.AspNetCore.NodeServices.HostingModels
             }
         }
 
+        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 @@ namespace Microsoft.AspNetCore.NodeServices.HostingModels
                     _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 @@ namespace Microsoft.AspNetCore.NodeServices.HostingModels
             _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);

src/Microsoft.AspNetCore.NodeServices/HostingModels/SocketNodeInstance.cs

@@ -21,9 +21,6 @@ namespace Microsoft.AspNetCore.NodeServices.HostingModels
     /// The address of the pipe/socket is selected randomly here on the .NET side and sent to the child process as a
     /// command-line argument (the address space is wide enough that there's no real risk of a clash, unlike when
     /// selecting TCP port numbers).
-    ///
-    /// 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 SocketNodeInstance : OutOfProcessNodeInstance
@@ -38,16 +35,15 @@ namespace Microsoft.AspNetCore.NodeServices.HostingModels
         private StreamConnection _physicalConnection;
         private string _socketAddress;
         private VirtualConnectionClient _virtualConnectionClient;
-        private readonly string[] _watchFileExtensions;
 
         public SocketNodeInstance(string projectPath, string[] watchFileExtensions, string socketAddress): base(
                 EmbeddedResourceReader.Read(
                     typeof(SocketNodeInstance),
                     "/Content/Node/entrypoint-socket.js"),
                 projectPath,
-                MakeNewCommandLineOptions(socketAddress, watchFileExtensions))
+                watchFileExtensions,
+                MakeNewCommandLineOptions(socketAddress))
         {
-            _watchFileExtensions = watchFileExtensions;
             _socketAddress = socketAddress;
 		}
 
@@ -188,15 +184,9 @@ namespace Microsoft.AspNetCore.NodeServices.HostingModels
             }
         }
 
-        private static string MakeNewCommandLineOptions(string listenAddress, string[] watchFileExtensions)
+        private static string MakeNewCommandLineOptions(string listenAddress)
         {
-            var result = "--listenAddress " + listenAddress;
+            return $"--listenAddress {listenAddress}";
-            if (watchFileExtensions != null && watchFileExtensions.Length > 0)
-            {
-                result += " --watch " + string.Join(",", watchFileExtensions);
-            }
-
-            return result;
         }
 
 #pragma warning disable 649 // These properties are populated via JSON deserialization

src/Microsoft.AspNetCore.NodeServices/TypeScript/HttpNodeInstanceEntryPoint.ts

@@ -3,17 +3,11 @@
 import * as http from 'http';
 import * as path from 'path';
 import { parseArgs } from './Util/ArgsUtil';
-import { autoQuitOnFileChange } from './Util/AutoQuit';
 
 // Webpack doesn't support dynamic requires for files not present at compile time, so grab a direct
 // reference to Node's runtime 'require' function.
 const dynamicRequire: (name: string) => any = eval('require');
 
-const parsedArgs = parseArgs(process.argv);
-if (parsedArgs.watch) {
-    autoQuitOnFileChange(process.cwd(), parsedArgs.watch.split(','));
-}
-
 const server = http.createServer((req, res) => {
     readRequestBodyAsJson(req, bodyJson => {
         let hasSentResult = false;
@@ -68,6 +62,7 @@ const server = http.createServer((req, res) => {
     });
 });
 
+const parsedArgs = parseArgs(process.argv);
 const 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

src/Microsoft.AspNetCore.NodeServices/TypeScript/SocketNodeInstanceEntryPoint.ts

@@ -5,16 +5,11 @@ import * as path from 'path';
 import * as readline from 'readline';
 import { Duplex } from 'stream';
 import { parseArgs } from './Util/ArgsUtil';
-import { autoQuitOnFileChange } from './Util/AutoQuit';
 import * as virtualConnectionServer from './VirtualConnections/VirtualConnectionServer';
 
 // Webpack doesn't support dynamic requires for files not present at compile time, so grab a direct
 // reference to Node's runtime 'require' function.
 const dynamicRequire: (name: string) => any = eval('require');
-const parsedArgs = parseArgs(process.argv);
-if (parsedArgs.watch) {
-    autoQuitOnFileChange(process.cwd(), parsedArgs.watch.split(','));
-}
 
 // Signal to the .NET side when we're ready to accept invocations
 const server = net.createServer().on('listening', () => {
@@ -69,6 +64,7 @@ virtualConnectionServer.createInterface(server).on('connection', (connection: Du
 // 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.
 const useWindowsNamedPipes = /^win/.test(process.platform);
+const parsedArgs = parseArgs(process.argv);
 const listenAddress = (useWindowsNamedPipes ? '\\\\.\\pipe\\' : '/tmp/') + parsedArgs.listenAddress;
 server.listen(listenAddress);
 

src/Microsoft.AspNetCore.NodeServices/TypeScript/Util/AutoQuit.ts

@@ -1,18 +0,0 @@
-import * as fs from 'fs';
-import * as path from 'path';
-
-export function autoQuitOnFileChange(rootDir: string, extensions: string[]) {
-    // 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 } as any, (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]');
-        }
-    });
-}

src/Microsoft.AspNetCore.NodeServices/Util/StringAsTempFile.cs

@@ -28,7 +28,7 @@ namespace Microsoft.AspNetCore.NodeServices
             {
                 if (disposing)
                 {
-                    // TODO: dispose managed state (managed objects).
+                    // Would dispose managed state here, if there was any
                 }
 
                 File.Delete(FileName);