Merge pull request #167 from JeringTech/fix_file_watcher_firing_extra_events

Fix extra file watcher events causing unnecessary Node.js restarts

Author: JeremyTCD

perf/NodeJS/FileWatchingBenchmarks.cs

@@ -1,5 +1,4 @@
 using BenchmarkDotNet.Attributes;
-using BenchmarkDotNet.Diagnosers;
 using Microsoft.Extensions.DependencyInjection;
 
 namespace Jering.Javascript.NodeJS.Performance
@@ -29,7 +28,7 @@ public void HttpNodeJSService_FileWatching_GracefulShutdownEnabled_MoveToNewProc
         [Benchmark]
         public void HttpNodeJSService_FileWatching_GracefulShutdownEnabled_MoveToNewProcess()
         {
-            _httpNodeJSService!.FileChangedHandler(DUMMY_PATH);
+            _httpNodeJSService!.FileChangedHandler();
         }
 
         [GlobalSetup(Target = nameof(HttpNodeJSService_FileWatching_GracefulShutdownDisabled_MoveToNewProcess))]
@@ -52,7 +51,7 @@ public void HttpNodeJSService_FileWatching_GracefulShutdownDisabled_MoveToNewPro
         [Benchmark]
         public void HttpNodeJSService_FileWatching_GracefulShutdownDisabled_MoveToNewProcess()
         {
-            _httpNodeJSService!.FileChangedHandler(DUMMY_PATH);
+            _httpNodeJSService!.FileChangedHandler();
         }
 
         [GlobalCleanup]

src/NodeJS/NodeJSServiceCollectionExtensions.cs

@@ -31,7 +31,7 @@ public static IServiceCollection AddNodeJS(this IServiceCollection services)
                 AddSingleton(typeof(INodeJSService), INodeJSServiceFactory).
                 AddSingleton<IJsonService, JsonService>().
                 AddSingleton<IEnvironmentService, EnvironmentService>().
-                AddSingleton<IFileWatcherFactory, FileWatcherFactory>().
+                AddSingleton<IFileWatcherService, FileWatcherService>().
                 AddSingleton<ITaskService, TaskService>().
                 AddTransient<IBlockDrainerService, BlockDrainerService>();
 #if NETCOREAPP3_1

src/NodeJS/NodeJSServiceImplementations/OutOfProcess/Http/HttpNodeJSService.cs

@@ -48,7 +48,7 @@ public class HttpNodeJSService : OutOfProcessNodeJSService
         /// <param name="httpNodeJSServiceOptionsAccessor">The <see cref="HttpNodeJSServiceOptions"/> accessor.</param>
         /// <param name="httpContentFactory">The factory for creating <see cref="HttpContent"/>s.</param>
         /// <param name="embeddedResourcesService">The service for retrieving NodeJS Http server scripts.</param>
-        /// <param name="fileWatcherFactory">The service for creating <see cref="IFileWatcher"/>s</param>
+        /// <param name="fileWatcherService">The service for watching files.</param>
         /// <param name="taskService">The service for utilizing tasks.</param>
         /// <param name="blockDrainerService">The service for draining code blocks.</param>
         /// <param name="httpClientService">The service for utilizing <see cref="HttpClient"/>.</param>
@@ -59,7 +59,7 @@ public HttpNodeJSService(IOptions<OutOfProcessNodeJSServiceOptions> outOfProcess
             IOptions<HttpNodeJSServiceOptions> httpNodeJSServiceOptionsAccessor,
             IHttpContentFactory httpContentFactory,
             IEmbeddedResourcesService embeddedResourcesService,
-            IFileWatcherFactory fileWatcherFactory,
+            IFileWatcherService fileWatcherService,
             ITaskService taskService,
             IBlockDrainerService blockDrainerService,
             IHttpClientService httpClientService,
@@ -70,7 +70,7 @@ public HttpNodeJSService(IOptions<OutOfProcessNodeJSServiceOptions> outOfProcess
                 logger,
                 outOfProcessNodeJSServiceOptionsAccessor,
                 embeddedResourcesService,
-                fileWatcherFactory,
+                fileWatcherService,
                 taskService,
                 blockDrainerService,
                 typeof(HttpNodeJSService).GetTypeInfo().Assembly,

src/NodeJS/NodeJSServiceImplementations/OutOfProcess/OutOfProcessNodeJSService.cs

@@ -31,7 +31,7 @@ public abstract class OutOfProcessNodeJSService : INodeJSService
         private readonly IEmbeddedResourcesService _embeddedResourcesService;
         private readonly ITaskService _taskService;
         private readonly IBlockDrainerService _blockDrainerService;
-        private readonly IFileWatcherFactory _fileWatcherFactory;
+        private readonly IFileWatcherService _fileWatcherService;
         private readonly INodeJSProcessFactory _nodeProcessFactory;
         private readonly string _serverScriptName;
         private readonly Assembly _serverScriptAssembly;
@@ -48,7 +48,6 @@ public abstract class OutOfProcessNodeJSService : INodeJSService
 
         private bool _disposed;
         private volatile INodeJSProcess? _nodeJSProcess; // Volatile since it's used in a double checked lock
-        private IFileWatcher? _fileWatcher;
 
         /// <summary>
         /// <para>This regex is used to determine successful initialization of the process.</para>
@@ -63,7 +62,7 @@ public abstract class OutOfProcessNodeJSService : INodeJSService
         /// <param name="logger"></param>
         /// <param name="optionsAccessor"></param>
         /// <param name="embeddedResourcesService"></param>
-        /// <param name="fileWatcherFactory"></param>
+        /// <param name="fileWatcherService"></param>
         /// <param name="taskService"></param>
         /// <param name="blockDrainerService"></param>
         /// <param name="serverScriptAssembly"></param>
@@ -72,13 +71,13 @@ protected OutOfProcessNodeJSService(INodeJSProcessFactory nodeProcessFactory,
             ILogger logger,
             IOptions<OutOfProcessNodeJSServiceOptions> optionsAccessor,
             IEmbeddedResourcesService embeddedResourcesService,
-            IFileWatcherFactory fileWatcherFactory,
+            IFileWatcherService fileWatcherService,
             ITaskService taskService,
             IBlockDrainerService blockDrainerService,
             Assembly serverScriptAssembly,
             string serverScriptName)
         {
-            _fileWatcherFactory = fileWatcherFactory;
+            _fileWatcherService = fileWatcherService;
             _taskService = taskService;
             _blockDrainerService = blockDrainerService;
 
@@ -383,13 +382,6 @@ internal virtual async ValueTask ConnectIfNotConnectedAsync(CancellationToken ca
                     return;
                 }
 
-                // No need to listen for file events while connecting - the new NodeJS process will reload all files.
-                if (_debugLoggingEnabled)
-                {
-                    Logger.LogDebug(string.Format(Strings.LogDebug_OutOfProcessNodeJSService_StoppingFileWatcher, _nodeJSProcess?.SafeID));
-                }
-                _fileWatcher?.Stop();
-
                 await CreateNewProcessAndConnectAsync().ConfigureAwait(false);
             }
             finally
@@ -458,7 +450,6 @@ internal virtual async Task CreateNewProcessAndConnectAsync()
                         {
                             Logger.LogDebug(string.Format(Strings.LogDebug_OutOfProcessNodeJSService_StartingFileWatcher, _nodeJSProcess.SafeID));
                         }
-                        _fileWatcher?.Start();
 
                         _nodeJSProcess.SetConnected();
 
@@ -533,7 +524,7 @@ internal virtual (bool trackInvokeTasks, ConcurrentDictionary<Task, object?> tra
                 return default;
             }
 
-            _fileWatcher = _fileWatcherFactory.Create(_options.WatchPath, _options.WatchSubdirectories, _options.WatchFileNamePatterns, FileChangedHandler);
+            _fileWatcherService.AddFileChangedListenerAsync(FileChangedHandler);
 
             if (!_options.GracefulProcessShutdown)
             {
@@ -578,19 +569,12 @@ internal virtual (bool trackInvokeTasks, ConcurrentDictionary<Task, object?> tra
         }
 
         // FileSystemWatcher handles file events synchronously (one after another), storing pending events in a buffer - https://github.com/dotnet/runtime/blob/master/src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.Win32.cs.
-        internal virtual void FileChangedHandler(string path)
+        internal virtual void FileChangedHandler()
         {
-            if (_infoLoggingEnabled)
-            {
-                Logger.LogInformation(string.Format(Strings.LogInformation_FileChangedMovingtoNewNodeJSProcess, path));
-            }
-
-            // Immediately stop the fileWatcher so we don't get redundant events (see FileWatcher.Stop)
             if (_debugLoggingEnabled)
             {
-                Logger.LogDebug(string.Format(Strings.LogDebug_OutOfProcessNodeJSService_StoppingFileWatcher, _nodeJSProcess?.SafeID));
+                Logger.LogDebug(string.Format(Strings.LogDebug_FileChangedHandlerInvokedForProcess, _nodeJSProcess?.SafeID));
             }
-            _fileWatcher?.Stop();
 
 #pragma warning disable CS4014
             // No need to await
@@ -760,7 +744,6 @@ protected virtual void Dispose(bool disposing)
             if (disposing)
             {
                 _nodeJSProcess?.Dispose();
-                _fileWatcher?.Dispose();
                 _connectingLock?.Dispose();
                 _blockDrainerService?.Dispose();
             }

src/NodeJS/Strings.Designer.cs

@@ -218,4 +218,16 @@ Exception:
   <data name="LogDebug_OutOfProcessNodeJSService_StoppingFileWatcher" xml:space="preserve">
     <value>Stopping FileWatcher. Current process: {0}.</value>
   </data>
+  <data name="LogDebug_InternalFileChangedHandlerCallDebounced" xml:space="preserve">
+    <value>Internal file changed handler call debounced, file path: "{0}".</value>
+  </data>
+  <data name="LogDebug_InternalFileChangedHandlerCalled" xml:space="preserve">
+    <value>Internal file changed handler called, file path: "{0}".</value>
+  </data>
+  <data name="LogInformation_InvokingRegisteredFileChangedHandlers" xml:space="preserve">
+    <value>Invoking registered file changed handlers, file path: "{0}".</value>
+  </data>
+  <data name="LogDebug_FileChangedHandlerInvokedForProcess" xml:space="preserve">
+    <value>File changed handler invoked for process: {0}.</value>
+  </data>
 </root>