Add Module Reloading On File Change #69
Conversation
This was my first idea but it was not an option for us because we have other While the old module instances themselves should have been garbage collected, unfortunately it seems not everything gets cleaned up. |
|
That's interesting. What if we force garbage collection after clearing the cache? |
|
Scratch that, I think the problem is that HttpServer.ts has a reference to the fs module export (along with some other built in module exports). Will have to invalidate all those references. |
|
Can confirm forcing garbage collection doesn't help. A simpler way to demonstrate the problem is with const now = Date.now();
setInterval(() => console.log(now), 5000);It should only be possible to have one of those running at once, but clearing the module cache and reloading the module allows more and more to build up. There are probably plenty of other async operations that will cause the same problem. Perhaps it's possible to track all async handles with |
|
Ah okay, so the NodeJS runtime keeps direct references to callback methods and continues to call them when events trigger.
This is interesting. Read through quickly and it looks like we can't manually destroy
Yeah, high risk of memory leaks. We'll have to take a look at NodeJS' source code and perhaps do some memory profiling to figure out if we can reload modules cleanly. If it isn't possible we'll just restart the process. Will have to figure out how to handle queued invocations that get canceled in that case. Am not satisfied with the current reliance on retries. |
Module Reloading ApproachesAt present, users can use NodeJS's fs to watch files and restart the process when they change, thereby reloading all modules. While this works, module reloading is more popular than expected, so we should provide an out-of-the-box solution. Two possibilities: Delete Cached ModulesDelete modules from require.cache so they're reloaded:
ProsFast. Avoids restarting the process, avoids terminating invocations midway through. ConsMemory leaks. Even if we delete a module from the cache, as pointed out by @rosslovas, the NodeJS runtime may hold references to the module. This is typically due to async stuff - the runtime holds references to callbacks, which in turn hold references to the modules that created them. Did a quick check to see if NodeJS exposes the structures holding callbacks. Found several cases where it doesn't. E.g if you call How we can make this workWhen we need to reload a module, we can call a custom method, e.g module.exports = {
doSomething: (callback) => {
... // Do something
callback(null, result);
},
cleanup: () => {
... // Cleanup async stuff, e.g clearTimeout()
}
}HttpServer.ts would have the following logic: fs.watch(watchDirectory, { recursive: watchRecursive }, (_, filename) => {
var moduleID = ... // We can easily tell from within NodeJS if the changed file is a loaded module, its ID is just its absolute file path
var module = require.cache[moduleID];
if (typeof module.cleanup === 'function') {
module.cleanup(); // Cleanup async stuff
}
delete require.cache[moduleID];
... // Optionally cleanup and delete its dependencies
});As noted in the comments, we can easily tell if a .js file that changes is a loaded module, and automatically reload it. Users will have to associate other kinds of files with modules to reload though, e.g if x.html changes, reload module x.js. Users who aren't using async stuff or are just using one-off callbacks like setTimeout might not need Restart ProcessNukes the entire process, effectively reloading all modules. ProsEasy to implement. Don't need to worry about memory leaks. ConsSlow. Restarting the process takes several hundred ms. We could start a new process while the previous one is cleaning up, though if a user changes files rapidly this could cause a bunch of NodeJS processes. Unstable. If we restart the process immediately after a file changes, invocations may terminate midway through. Users have to be wary of this. Also, when an invocation fails because of a restart, the error is a generic We could allow the previous process to complete while sending new requests to a new process. Again, if a user changes files rapidly this could cause a bunch of NodeJS processes. ConclusionNeither solution is clean. Deleting cached modules is much better for development situations, restarting the process is better for long running applications that don't update files often. Perhaps we can implement both solutions eventually, but which one to implement first? Suggestions welcome. |
JeremyTCD
changed the title
|
Since this is uncontrolled userland code that's being handled by this library, restarting the process seems like the simpler of the two options. You are correct that if any modules leak event listeners that there won't be a reliable way to detect them. There also could be child processes that are spawned (child_process.spawn), which should be cleaned up. |
|
@dustinsoftware Yeah agreed, slightly quicker module reloads aren't worth the extra complexity. I've implemented process restarting on file change, just got to clean up, test and document. |
JeremyTCD
force-pushed
the
Add_File_Watching
branch
4 times, most recently
from
4a76943 to
8e16b5c
Compare
- For process restarting to work, OutOfProcessNodeJSService will have to control multiple processes at once. This means it's output data and error data string builders are no longer sufficient - each process needs its own string builders. So we move all message building to NodeJSProcess.
- NodeJSProcess was implementing Object.Finalizer unecessarily. Under the hood, the Process type handles unmanaged resource disposal using a SafeHandle.
…ce.TryInvokeCoreAsync<T> - When a file changes, we stop sending invocations to the last process, we create and connect to a new process and send invocations to it instead. For optimum responsiveness, we want to create an dconnect to the new process immediately after the file changes, not when the first invocation comes in after the file changes. - To do that, we need connection logic with retries and exception handling in its own method. - A serendipitous side effect is that we can add a ConnectionException specifically for connection issues.
JeremyTCD
force-pushed
the
Add_File_Watching
branch
2 times, most recently
from
7c95ae9 to
bb2a969
Compare
- Only relevant to NodeJSServiceImplementations.
- Only relevant to out-of-process implementations.
JeremyTCD
force-pushed
the
Add_File_Watching
branch
from
bb2a969 to
3fecc21
Compare
Codecov Report
@@ Coverage Diff @@
## master #69 +/- ##
==========================================
- Coverage 97.52% 97.02% -0.51%
==========================================
Files 18 23 +5
Lines 566 807 +241
==========================================
+ Hits 552 783 +231
- Misses 14 24 +10
Continue to review full report at Codecov.
|
JeremyTCD
force-pushed
the
Add_File_Watching
branch
4 times, most recently
from
178f52f to
7ffb903
Compare
JeremyTCD
force-pushed
the
Add_File_Watching
branch
3 times, most recently
from
a67e1c6 to
d4f10e2
Compare
|
🚀 5.4.0 released with file watching. Enabling file watching:services.Configure<OutOfProcessNodeJSOptions>(options => options.EnableFileWatching = true);(General info on configuring options) More file watching optionsOutOfProcessNodeJSOptions (WatchPath, WatchSubdirectories, WatchFileNamePatterns, WatchGracefulShutdown). Benchmarks
|
|
This is great!! Thanks for shipping it. |
Details: #69 (comment)