Skip to content

Commit eec370e

Browse files
SteveSandersonMSSteveSandersonMS
committed
Move file-watching logic into .NET to avoid Node's fs.watch issues on Windows (aspnet#128)
1 parent ce127f0 commit eec370e

File tree

10 files changed

+111
-150
lines changed

10 files changed

+111
-150
lines changed

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ private static INodeInstance CreateNodeInstance(NodeServicesOptions options)
4646
switch (options.HostingModel)
4747
{
4848
case NodeHostingModel.Http:
49-
return new HttpNodeInstance(options.ProjectPath, /* port */ 0, options.WatchFileExtensions);
49+
return new HttpNodeInstance(options.ProjectPath, options.WatchFileExtensions, /* port */ 0);
5050
case NodeHostingModel.Socket:
5151
var pipeName = "pni-" + Guid.NewGuid().ToString("D"); // Arbitrary non-clashing string
5252
return new SocketNodeInstance(options.ProjectPath, options.WatchFileExtensions, pipeName);

‎src/Microsoft.AspNetCore.NodeServices/Content/Node/entrypoint-http.js‎

Lines changed: 1 addition & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -57,14 +57,9 @@
5757
var http = __webpack_require__(2);
5858
var path = __webpack_require__(3);
5959
var ArgsUtil_1 = __webpack_require__(4);
60-
var AutoQuit_1 = __webpack_require__(5);
6160
// Webpack doesn't support dynamic requires for files not present at compile time, so grab a direct
6261
// reference to Node's runtime 'require' function.
6362
var dynamicRequire = eval('require');
64-
var parsedArgs = ArgsUtil_1.parseArgs(process.argv);
65-
if (parsedArgs.watch) {
66-
AutoQuit_1.autoQuitOnFileChange(process.cwd(), parsedArgs.watch.split(','));
67-
}
6863
var server = http.createServer(function (req, res) {
6964
readRequestBodyAsJson(req, function (bodyJson) {
7065
var hasSentResult = false;
@@ -117,6 +112,7 @@
117112
}
118113
});
119114
});
115+
var parsedArgs = ArgsUtil_1.parseArgs(process.argv);
120116
var requestedPortOrZero = parsedArgs.port || 0; // 0 means 'let the OS decide'
121117
server.listen(requestedPortOrZero, 'localhost', function () {
122118
// Signal to HttpNodeHost which port it should make its HTTP connections on
@@ -170,35 +166,5 @@
170166
exports.parseArgs = parseArgs;
171167

172168

173-
/***/ },
174-
/* 5 */
175-
/***/ function(module, exports, __webpack_require__) {
176-
177-
"use strict";
178-
var fs = __webpack_require__(6);
179-
var path = __webpack_require__(3);
180-
function autoQuitOnFileChange(rootDir, extensions) {
181-
// Note: This will only work on Windows/OS X, because the 'recursive' option isn't supported on Linux.
182-
// Consider using a different watch mechanism (though ideally without forcing further NPM dependencies).
183-
fs.watch(rootDir, { persistent: false, recursive: true }, function (event, filename) {
184-
var ext = path.extname(filename);
185-
if (extensions.indexOf(ext) >= 0) {
186-
console.log('Restarting due to file change: ' + filename);
187-
// Temporarily, the file-watching logic is in Node, so we signal it's time to restart by
188-
// sending the following message back to .NET. Soon the file-watching logic will move over
189-
// to the .NET side, and this whole file can be removed.
190-
console.log('[Microsoft.AspNetCore.NodeServices:Restart]');
191-
}
192-
});
193-
}
194-
exports.autoQuitOnFileChange = autoQuitOnFileChange;
195-
196-
197-
/***/ },
198-
/* 6 */
199-
/***/ function(module, exports) {
200-
201-
module.exports = require("fs");
202-
203169
/***/ }
204170
/******/ ])));

‎src/Microsoft.AspNetCore.NodeServices/Content/Node/entrypoint-socket.js‎

Lines changed: 14 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@
4444
/* 0 */
4545
/***/ function(module, exports, __webpack_require__) {
4646

47-
module.exports = __webpack_require__(7);
47+
module.exports = __webpack_require__(5);
4848

4949

5050
/***/ },
@@ -83,54 +83,19 @@
8383

8484
/***/ },
8585
/* 5 */
86-
/***/ function(module, exports, __webpack_require__) {
87-
88-
"use strict";
89-
var fs = __webpack_require__(6);
90-
var path = __webpack_require__(3);
91-
function autoQuitOnFileChange(rootDir, extensions) {
92-
// Note: This will only work on Windows/OS X, because the 'recursive' option isn't supported on Linux.
93-
// Consider using a different watch mechanism (though ideally without forcing further NPM dependencies).
94-
fs.watch(rootDir, { persistent: false, recursive: true }, function (event, filename) {
95-
var ext = path.extname(filename);
96-
if (extensions.indexOf(ext) >= 0) {
97-
console.log('Restarting due to file change: ' + filename);
98-
// Temporarily, the file-watching logic is in Node, so we signal it's time to restart by
99-
// sending the following message back to .NET. Soon the file-watching logic will move over
100-
// to the .NET side, and this whole file can be removed.
101-
console.log('[Microsoft.AspNetCore.NodeServices:Restart]');
102-
}
103-
});
104-
}
105-
exports.autoQuitOnFileChange = autoQuitOnFileChange;
106-
107-
108-
/***/ },
109-
/* 6 */
110-
/***/ function(module, exports) {
111-
112-
module.exports = require("fs");
113-
114-
/***/ },
115-
/* 7 */
11686
/***/ function(module, exports, __webpack_require__) {
11787

11888
"use strict";
11989
// Limit dependencies to core Node modules. This means the code in this file has to be very low-level and unattractive,
12090
// but simplifies things for the consumer of this module.
121-
var net = __webpack_require__(8);
91+
var net = __webpack_require__(6);
12292
var path = __webpack_require__(3);
123-
var readline = __webpack_require__(9);
93+
var readline = __webpack_require__(7);
12494
var ArgsUtil_1 = __webpack_require__(4);
125-
var AutoQuit_1 = __webpack_require__(5);
126-
var virtualConnectionServer = __webpack_require__(10);
95+
var virtualConnectionServer = __webpack_require__(8);
12796
// Webpack doesn't support dynamic requires for files not present at compile time, so grab a direct
12897
// reference to Node's runtime 'require' function.
12998
var dynamicRequire = eval('require');
130-
var parsedArgs = ArgsUtil_1.parseArgs(process.argv);
131-
if (parsedArgs.watch) {
132-
AutoQuit_1.autoQuitOnFileChange(process.cwd(), parsedArgs.watch.split(','));
133-
}
13499
// Signal to the .NET side when we're ready to accept invocations
135100
var server = net.createServer().on('listening', function () {
136101
console.log('[Microsoft.AspNetCore.NodeServices:Listening]');
@@ -179,29 +144,30 @@
179144
// Begin listening now. The underlying transport varies according to the runtime platform.
180145
// On Windows it's Named Pipes; on Linux/OSX it's Domain Sockets.
181146
var useWindowsNamedPipes = /^win/.test(process.platform);
147+
var parsedArgs = ArgsUtil_1.parseArgs(process.argv);
182148
var listenAddress = (useWindowsNamedPipes ? '\\\\.\\pipe\\' : '/tmp/') + parsedArgs.listenAddress;
183149
server.listen(listenAddress);
184150

185151

186152
/***/ },
187-
/* 8 */
153+
/* 6 */
188154
/***/ function(module, exports) {
189155

190156
module.exports = require("net");
191157

192158
/***/ },
193-
/* 9 */
159+
/* 7 */
194160
/***/ function(module, exports) {
195161

196162
module.exports = require("readline");
197163

198164
/***/ },
199-
/* 10 */
165+
/* 8 */
200166
/***/ function(module, exports, __webpack_require__) {
201167

202168
"use strict";
203-
var events_1 = __webpack_require__(11);
204-
var VirtualConnection_1 = __webpack_require__(12);
169+
var events_1 = __webpack_require__(9);
170+
var VirtualConnection_1 = __webpack_require__(10);
205171
// Keep this in sync with the equivalent constant in the .NET code. Both sides split up their transmissions into frames with this max length,
206172
// and both will reject longer frames.
207173
var MaxFrameBodyLength = 16 * 1024;
@@ -382,13 +348,13 @@
382348

383349

384350
/***/ },
385-
/* 11 */
351+
/* 9 */
386352
/***/ function(module, exports) {
387353

388354
module.exports = require("events");
389355

390356
/***/ },
391-
/* 12 */
357+
/* 10 */
392358
/***/ function(module, exports, __webpack_require__) {
393359

394360
"use strict";
@@ -397,7 +363,7 @@
397363
function __() { this.constructor = d; }
398364
d.prototype = b === null ? Object.create(b) : (__.prototype = b.prototype, new __());
399365
};
400-
var stream_1 = __webpack_require__(13);
366+
var stream_1 = __webpack_require__(11);
401367
/**
402368
* Represents a virtual connection. Multiple virtual connections may be multiplexed over a single physical socket connection.
403369
*/
@@ -438,7 +404,7 @@
438404

439405

440406
/***/ },
441-
/* 13 */
407+
/* 11 */
442408
/***/ function(module, exports) {
443409

444410
module.exports = require("stream");

‎src/Microsoft.AspNetCore.NodeServices/HostingModels/HttpNodeInstance.cs‎

Lines changed: 5 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -16,9 +16,6 @@ namespace Microsoft.AspNetCore.NodeServices.HostingModels
1616
/// port number is specified as a constructor parameter), and signals which port was selected using the same
1717
/// input/output-based mechanism that the base class uses to determine when the child process is ready to
1818
/// accept RPC invocations.
19-
///
20-
/// TODO: Remove the file-watching logic from here and centralise it in OutOfProcessNodeInstance, implementing
21-
/// the actual watching in .NET code (not Node), for consistency across platforms.
2219
/// </summary>
2320
/// <seealso cref="Microsoft.AspNetCore.NodeServices.HostingModels.OutOfProcessNodeInstance" />
2421
internal class HttpNodeInstance : OutOfProcessNodeInstance
@@ -35,26 +32,21 @@ internal class HttpNodeInstance : OutOfProcessNodeInstance
3532
private bool _disposed;
3633
private int _portNumber;
3734

38-
public HttpNodeInstance(string projectPath, int port = 0, string[] watchFileExtensions = null)
35+
public HttpNodeInstance(string projectPath, string[] watchFileExtensions, int port = 0)
3936
: base(
4037
EmbeddedResourceReader.Read(
4138
typeof(HttpNodeInstance),
4239
"/Content/Node/entrypoint-http.js"),
4340
projectPath,
44-
MakeCommandLineOptions(port, watchFileExtensions))
41+
watchFileExtensions,
42+
MakeCommandLineOptions(port))
4543
{
4644
_client = new HttpClient();
4745
}
4846

49-
private static string MakeCommandLineOptions(int port, string[] watchFileExtensions)
47+
private static string MakeCommandLineOptions(int port)
5048
{
51-
var result = "--port " + port;
52-
if (watchFileExtensions != null && watchFileExtensions.Length > 0)
53-
{
54-
result += " --watch " + string.Join(",", watchFileExtensions);
55-
}
56-
57-
return result;
49+
return $"--port {port}";
5850
}
5951

6052
protected override async Task<T> InvokeExportAsync<T>(NodeInvocationInfo invocationInfo)

‎src/Microsoft.AspNetCore.NodeServices/HostingModels/OutOfProcessNodeInstance.cs‎

Lines changed: 83 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
using System;
22
using System.Diagnostics;
33
using System.IO;
4+
using System.Linq;
45
using System.Threading.Tasks;
56

67
namespace Microsoft.AspNetCore.NodeServices.HostingModels
@@ -18,17 +19,24 @@ namespace Microsoft.AspNetCore.NodeServices.HostingModels
1819
public abstract class OutOfProcessNodeInstance : INodeInstance
1920
{
2021
private const string ConnectionEstablishedMessage = "[Microsoft.AspNetCore.NodeServices:Listening]";
21-
private const string NeedsRestartMessage = "[Microsoft.AspNetCore.NodeServices:Restart]";
2222
private readonly TaskCompletionSource<object> _connectionIsReadySource = new TaskCompletionSource<object>();
2323
private bool _disposed;
2424
private readonly StringAsTempFile _entryPointScript;
25+
private FileSystemWatcher _fileSystemWatcher;
2526
private readonly Process _nodeProcess;
2627
private bool _nodeProcessNeedsRestart;
28+
private readonly string[] _watchFileExtensions;
2729

28-
public OutOfProcessNodeInstance(string entryPointScript, string projectPath, string commandLineArguments = null)
30+
public OutOfProcessNodeInstance(
31+
string entryPointScript,
32+
string projectPath,
33+
string[] watchFileExtensions,
34+
string commandLineArguments)
2935
{
3036
_entryPointScript = new StringAsTempFile(entryPointScript);
3137
_nodeProcess = LaunchNodeProcess(_entryPointScript.FileName, projectPath, commandLineArguments);
38+
_watchFileExtensions = watchFileExtensions;
39+
_fileSystemWatcher = BeginFileWatcher(projectPath);
3240
ConnectToInputOutputStreams();
3341
}
3442

@@ -80,6 +88,7 @@ protected virtual void Dispose(bool disposing)
8088
if (disposing)
8189
{
8290
_entryPointScript.Dispose();
91+
EnsureFileSystemWatcherIsDisposed();
8392
}
8493

8594
// Make sure the Node process is finished
@@ -93,6 +102,15 @@ protected virtual void Dispose(bool disposing)
93102
}
94103
}
95104

105+
private void EnsureFileSystemWatcherIsDisposed()
106+
{
107+
if (_fileSystemWatcher != null)
108+
{
109+
_fileSystemWatcher.Dispose();
110+
_fileSystemWatcher = null;
111+
}
112+
}
113+
96114
private static Process LaunchNodeProcess(string entryPointFilename, string projectPath, string commandLineArguments)
97115
{
98116
var startInfo = new ProcessStartInfo("node")
@@ -143,13 +161,6 @@ private void ConnectToInputOutputStreams()
143161
_connectionIsReadySource.SetResult(null);
144162
initializationIsCompleted = true;
145163
}
146-
else if (evt.Data == NeedsRestartMessage)
147-
{
148-
// Temporarily, the file-watching logic is in Node, so look out for the
149-
// signal that we need to restart. This can be removed once the file-watching
150-
// logic is moved over to the .NET side.
151-
_nodeProcessNeedsRestart = true;
152-
}
153164
else if (evt.Data != null)
154165
{
155166
OnOutputDataReceived(evt.Data);
@@ -177,6 +188,69 @@ private void ConnectToInputOutputStreams()
177188
_nodeProcess.BeginErrorReadLine();
178189
}
179190

191+
private FileSystemWatcher BeginFileWatcher(string rootDir)
192+
{
193+
if (_watchFileExtensions == null || _watchFileExtensions.Length == 0)
194+
{
195+
// Nothing to watch
196+
return null;
197+
}
198+
199+
var watcher = new FileSystemWatcher(rootDir)
200+
{
201+
IncludeSubdirectories = true,
202+
NotifyFilter = NotifyFilters.LastWrite | NotifyFilters.FileName | NotifyFilters.DirectoryName
203+
};
204+
watcher.Changed += OnFileChanged;
205+
watcher.Created += OnFileChanged;
206+
watcher.Deleted += OnFileChanged;
207+
watcher.Renamed += OnFileRenamed;
208+
watcher.EnableRaisingEvents = true;
209+
return watcher;
210+
}
211+
212+
private void OnFileChanged(object source, FileSystemEventArgs e)
213+
{
214+
if (IsFilenameBeingWatched(e.FullPath))
215+
{
216+
RestartDueToFileChange(e.FullPath);
217+
}
218+
}
219+
220+
private void OnFileRenamed(object source, RenamedEventArgs e)
221+
{
222+
if (IsFilenameBeingWatched(e.OldFullPath) || IsFilenameBeingWatched(e.FullPath))
223+
{
224+
RestartDueToFileChange(e.OldFullPath);
225+
}
226+
}
227+
228+
private bool IsFilenameBeingWatched(string fullPath)
229+
{
230+
if (string.IsNullOrEmpty(fullPath))
231+
{
232+
return false;
233+
}
234+
else
235+
{
236+
var actualExtension = Path.GetExtension(fullPath) ?? string.Empty;
237+
return _watchFileExtensions.Any(actualExtension.Equals);
238+
}
239+
}
240+
241+
private void RestartDueToFileChange(string fullPath)
242+
{
243+
// TODO: Use proper logger
244+
Console.WriteLine($"Node will restart because file changed: {fullPath}");
245+
246+
_nodeProcessNeedsRestart = true;
247+
248+
// There's no need to watch for any more changes, since we're already restarting, and if the
249+
// restart takes some time (e.g., due to connection draining), we could end up getting duplicate
250+
// notifications.
251+
EnsureFileSystemWatcherIsDisposed();
252+
}
253+
180254
~OutOfProcessNodeInstance()
181255
{
182256
Dispose(false);

0 commit comments

Comments
 (0)