Conversation
|
Review requested:
|
nodejs-github-bot
added
esm
anonrig
force-pushed
the
yagiz/node-module-clear-cache
branch
2 times, most recently
from
90303e6 to
1d0accc
Compare
anonrig
added
semver-minor
|
The
notable-change
Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section. |
|
I’m relatively +1 on having this in Node.js, but I recall having a a lot of discussions about this @GeoffreyBooth and @nodejs/loaders teams about this, and it would massively break the spec, expectations, and invariants regarding ESM. (Note, this is what people have been asking us to add for a long time). My personal objection to this API is that it would inadvertently leak memory at every turn, so while this sounds good in theory, in practice it would significantly backfire in long-running scenarios. An option could be to expose it only behind a flag, putting the user in charge of choosing this behavior. Every single scenario where I saw HMR in Node.js ends up in memory leaks. This is the reason why I had so much interest and hopes for ShadowRealm. |
benjamingr
left a comment
benjamingr
left a comment
There was a problem hiding this comment.
I am still +1 on the feature from a user usability point of view. Code lgtm.
We're giving users a tool, it may be seen as a footgun by some but hopefully libraries that use the API correctly and warn users about incorrect usage emerge. |
|
@mcollina Thanks for the feedback. I agree the ESM semantics concerns are real. This API doesn’t change the core ESM invariants (single instance per URL); it only removes Node's internal cache entries to allow explicit reloads in opt‑in workflows. Even with that, existing references (namespaces, listeners, closures) can keep old graphs alive, so this is still potentially leaky unless the app does explicit disposal. I’ll make sure the docs call out the risks and the fact that this only clears Node’s internal caches, and I’d like loader team input on the final shape of the API. This commit should address some of your concerns. b3bd79a
Thanks for the review @benjamingr. Would you mind re-reviewing again so I can trigger CI? |
|
Thanks a lot for this ❤️ |
|
Rather than violating ESM invariants, can't node just provide a function that imports a url? i.e. While the given example of: const url = new URL('./mod.mjs', import.meta.url);
await import(url.href);
clearCache(url);
await import(url.href); // re-executes the moduleis indeed not spec compliant, it's perfectly legal to have something like: import { clearCache, importModule } from "node:module";
await importModule(someUrl);
clearCache();
await importModule(someUrl); // reexecute |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #61767 +/- ##
==========================================
+ Coverage 89.69% 89.70% +0.01%
==========================================
Files 695 696 +1
Lines 214417 214895 +478
Branches 41059 41149 +90
==========================================
+ Hits 192321 192779 +458
- Misses 14156 14169 +13
- Partials 7940 7947 +7
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
While I am +1 to the idea in general, I am afraid the current API may bring more problem than it solves...see the comments.
(Granted it isn't really a problem unique to this specific design, I think the issue is more that this is not a very well solved problem so far, I don't really know what it should look like, though I think I might be able to point out what it should not look like to avoid adding/re-introducing leaks/use-after-frees that user land workarounds can already manage)
|
I was the one requesting this while sitting next to yagiz today. We take advantage of Module Federation which allows us to distribute code at runtime. However, when parts of the distributed system are updated, it gets stuck in module cache. I've had some workarounds, like attempting to purge require cache - however when it comes to esm, it's a difficult problem. Since we do this distribution primarily in production, and there can be thousands of updates a day, I block esm from being supported because it'll leak memory - which was fine for several years but becoming more problematic in modern tooling. On lambda we cannot just exit a process and bring a new one up without triggering a empty deploy, which has generally been a perf hit to cold start a new lambda vs try and "reset" the module cache for primitive hot reload. Now, I know this might be controversial, or not recommended - but the reality is that many large companies use federation, most fortune 50 companies use it heavily. All of them are relying on userland cobbling I've created. If there is a solution, it would be greatly appreciated by all of my users. I believe this would also be very useful in general for tooling like rspack etc where we have universal dev serves. If invalidation of specific modules causes complexity, I'd be more than happy with a nuclear option like resetModuleCache() which just clears everything entirely. Would be a little slower, but nothing is slower than killing a process and bringing up a new one. "Soft Restart" node without killing it. Don't have much opinion on spec compliance etc, can go through NAPI as well if that would avoid any spec concerns or pushback. |
|
Chiming in to say that re-loading a module is very helpful in tests. We can do this with the fabulous CJS paradigm, but ESM does not have a viable equivalent and it should. |
|
I think there are still quite a few places that need updates/tests - I tried my best to find them, but there are some dusty corners in the module loader that I have never poked at, you might want to take a heap snapshot or write more tests with
|
|
I think I addressed all of your concerns @joyeecheung. Let me know if I missed anything! |
Just pinging @guybedford to speak on the spec concerns. I think we should wait for him or someone similarly knowledgeable about the spec to comment before landing. In general I'm +1 on the feature, assuming it can be safely implemented. My (dim) recollection was that the last time we considered it, it was impossible to modify an ES module after it had been loaded into V8. Has that changed in recent years? How do you handle cases like |
|
@guybedford Are there any minutes for the TC39 modules meeting? Will this topic be discussed at the upcoming TC39 plenary meeting?
update: It is reassuring to know that there are plans to modify the ECMAScript side.
|
There was a problem hiding this comment.
I've got a number of concerns with this that I'd like to see addressed before it lands.
-
There is a V8-level memory leak risk. For the cache-busting URLs with dynamic
import(), old modules should be collectible and there likely is not a problem. However, for static import graphs I believe this introduces a definite memory leak risk. -
The docs should be stronger about the leak risk. Static import graphs create permanent V8-internal references that prevent collection.
-
There's some missing test coverage. For instance, a test verifying what happens when you clear a module that was statically imported by a still-alive parent. The test should either demonstrate that it is leaked or collected.
-
I'm not convinced the loadCJSModuleWithModuleLoad race test covers all the edge cases while a concurrent dynamic
import()of CJS is in-flight. -
It's not clear if the hash_to_module_map cleanup is sufficient for same-URL re-import. There might be a memory leak case in hash_to_module_map that needs to be evaluated.
Demonstrating that the memory-leak risk is inconsequential, or that the existing tests adequately cover it, would be enough to clear this objection.
|
@jasnell Thanks for the detailed review. Addressing each point: 1. V8-level memory leak risk for static import graphs You are correct that this is a real concern. When a parent module P statically imports M, `v8::Module::InstantiateModule` creates a permanent V8-internal strong reference from P's compiled module record to M's. Node.js's `clearCache` only removes M from JS-level caches (`loadCache`, `resolveCache`, `require.cache`). It cannot sever the V8-internal link — M's `ModuleWrap` (and its `v8::Globalv8::Module`) remains alive for as long as P is alive. The leak is bounded, not unbounded. Each clear+reimport cycle with a live static parent produces exactly one stale `ModuleWrap` (the original), which stays alive for P's lifetime. It does not accumulate per cycle — see point 5 below. For dynamic imports (`await import()` with no static parent), the old `ModuleWrap` becomes eligible for GC once it is removed from the loadCache and all JS-land namespace references are dropped. The existing module-wrap-leak test and the new static-import-leak test confirm this. 2. Docs should be stronger about the leak risk Done. Added a dedicated "Memory retention and static imports" section that explicitly states:
3. Missing test coverage for cleared statically-imported modules Added `test/es-module/test-module-clear-cache-static-import-leak.mjs` with a fixture `test/fixtures/module-cache/esm-static-parent.mjs` that statically imports the counter. Part 1 — static parent retention: Loads the static parent (which pins counter v1). Calls `clearCache(counterURL)` then `await import(counterURL)` to get counter v2. Then:
Part 2 — dynamic-only modules are freed: Uses `checkIfCollectableByCounting` across 8×4 cache-busting import/clear cycles to confirm that without a static parent, cleared `ModuleWrap`s are garbage-collected. 4. CJS concurrent import race test coverage Rewrote `test-module-clear-cache-import-cjs-race.mjs` to cover three scenarios:
5. hash_to_module_map cleanup for same-URL re-import Added `test/es-module/test-module-clear-cache-hash-map.mjs` that evaluates this directly. `hash_to_module_map` is an `unordered_multimap<int, ModuleWrap*>`. Entries are added in the `ModuleWrap` constructor and removed in `~ModuleWrap()`. `clearCache` does not directly touch it — cleanup happens when `ModuleWrap` objects are GC'd.
`GetFromModule` compares `v8::Local` handles directly, so the multimap correctly distinguishes old-M from new-M even when both coexist. No correctness issue from multiple entries. The map grows by one per cycle with a static parent, but those entries are removed when the parent is eventually collected — bounded by the number of live static parents. |
github-actions
bot
removed
the
request-ci
|
@nodejs/build any idea why this doesn't work? |
anonrig
force-pushed
the
yagiz/node-module-clear-cache
branch
from
2a72a4f to
6d79dc2
Compare
|
I've squashed last 19 commits, rebased and force pushed. Can someone re-review and trigger |
github-actions
bot
removed
the
request-ci
| cycles. | ||
|
|
||
| For **dynamically imported** modules (`await import('./M.mjs')` with no live | ||
| static parent holding the result), the old `ModuleWrap` becomes eligible for |
There was a problem hiding this comment.
| static parent holding the result), the old `ModuleWrap` becomes eligible for | |
| static parent holding the result), the old module becomes eligible for |
We shouldn't be talking about ModuleWrap in the docs.
| `clearCache` only removes references from the Node.js **JavaScript-level** caches | ||
| (the ESM load cache, resolve cache, CJS `require.cache`, and related structures). | ||
| It does **not** affect V8-internal module graph references. |
There was a problem hiding this comment.
| `clearCache` only removes references from the Node.js **JavaScript-level** caches | |
| (the ESM load cache, resolve cache, CJS `require.cache`, and related structures). | |
| It does **not** affect V8-internal module graph references. | |
| `clearCache` only removes references from the Node.js internal caches | |
| (the ESM load cache, resolve cache, CJS `require.cache`, and related structures). | |
| It does **not** affect JavaScript-level module graph references. |
I feel that from a user's POV, the description is the other way around: JS-level references essentially comes from user code, the JS engine is only holding them on the user's behalf, not that V8 itself needs them for some reason. The cleared references are technically Node.js internals.
There was a problem hiding this comment.
I don't think "JavaScript-level module graph" imparts what you're hoping it does. If I didn't read this diff and associated review comment, I would have really struggled to understand what it means.
In my opinion, there should be an example that shows what this feature accomplishes. I keep getting the impression that it will solve the testing scenario I described above. But it's not clear to me from the explanation if that is true, or if this is meant to solve some other cache problem.
| const request = isURL(specifier) ? specifier.href : specifier; | ||
|
|
||
| if (!parentPath && isRelative(request)) { | ||
| return null; | ||
| } | ||
|
|
||
| const parent = parentPath ? createParentModuleForClearCache(parentPath) : null; | ||
| try { | ||
| const { filename, format } = resolveForCJSWithHooks(request, parent, false, false); | ||
| if (format === 'builtin') { | ||
| return null; | ||
| } | ||
| return filename; | ||
| } catch { | ||
| // Resolution can fail for non-file specifiers without hooks - return null | ||
| // to silently skip clearing rather than throwing. | ||
| return null; | ||
| } |
There was a problem hiding this comment.
| const request = isURL(specifier) ? specifier.href : specifier; | |
| if (!parentPath && isRelative(request)) { | |
| return null; | |
| } | |
| const parent = parentPath ? createParentModuleForClearCache(parentPath) : null; | |
| try { | |
| const { filename, format } = resolveForCJSWithHooks(request, parent, false, false); | |
| if (format === 'builtin') { | |
| return null; | |
| } | |
| return filename; | |
| } catch { | |
| // Resolution can fail for non-file specifiers without hooks - return null | |
| // to silently skip clearing rather than throwing. | |
| return null; | |
| } | |
| const parent = parentPath ? createParentModuleForClearCache(parentPath) : null; | |
| try { | |
| const { filename, format } = resolveForCJSWithHooks(specifier, parent, false, false); | |
| if (format === 'builtin') { | |
| return null; | |
| } | |
| return filename; | |
| } catch { | |
| // Resolution can fail for non-file specifiers without hooks - return null | |
| // to silently skip clearing rather than throwing. | |
| return null; | |
| } |
require doesn't understand URLs, it only accepts paths. If resolver is require and the specifier is a URL object, it should lead to ERR_INVALID_ARG_TYPE.
| // Evaluates the hash_to_module_map memory behaviour across clearCache cycles. | ||
| // | ||
| // hash_to_module_map is a C++ unordered_multimap<int, ModuleWrap*> on the | ||
| // Environment. Every new ModuleWrap adds an entry; the destructor removes it. | ||
| // Clearing the Node-side loadCache does not directly touch hash_to_module_map — | ||
| // entries are removed only when ModuleWrap objects are garbage-collected. | ||
| // | ||
| // We verify two invariants: | ||
| // | ||
| // 1. DYNAMIC imports: after clearCache + GC, the old ModuleWrap is collected | ||
| // and therefore its hash_to_module_map entry is removed. The map does NOT | ||
| // grow without bound for purely-dynamic import/clear cycles. | ||
| // (Verified via checkIfCollectableByCounting.) | ||
| // | ||
| // 2. STATIC imports: when a parent P statically imports M, clearing M from | ||
| // the load cache does not free M's ModuleWrap (the static link keeps it). | ||
| // Each re-import adds one new entry while the old entry stays for the | ||
| // lifetime of P. This is a bounded, expected retention (not an unbounded | ||
| // leak): it is capped at one stale entry per module per live static parent. |
There was a problem hiding this comment.
I feel that we should avoid talking about hash_to_module_map here, that can be changed later and the comment would then become weirdly out of sync in the test. Something more generic like "internal book-keeping" is enough.
| cascadedLoader.deleteResolveCacheEntry = function(specifier, parentURL, importAttributes) { | ||
| capturedCalls.push({ specifier, parentURL, importAttributes }); | ||
| return original.call(this, specifier, parentURL, importAttributes); | ||
| }; |
There was a problem hiding this comment.
Monkey-patching an internal method to check if it's invoked seems rather brittle, and doesn't really guarantee that it's working. I think for the import behavior we can just check actual behaviors (e.g. it re-resolves the next time or that you can observe the registered hooks is called again). For require we only need to check that it doesn't throw.
| assert.ok(cascadedLoader.hasResolveCacheEntry(specifier, parentURL), | ||
| 'resolve cache should have an entry after import'); |
There was a problem hiding this comment.
Do we need this? Other assertions already seem enough. It's better to avoid testing internals as much as possible to avoid the churn whenever they get modified.
| const { | ||
| privateSymbols: { | ||
| module_first_parent_private_symbol, | ||
| module_last_parent_private_symbol, | ||
| }, | ||
| } = internalBinding('util'); |
There was a problem hiding this comment.
I think it's better to avoid referencing internal symbols. Testing each one of known internal references are severed in separate tests is neither necessary nor sufficient to ensure that there are not any remaining references internally - if there are 5 types of references in total, have 4 tests each testing 1 type of reference can be severed still won't ensure that there's no leak, and it won't catch anything if one more type of reference is added internally. On the other hand, if some internal changes just remove one type of them since they are just no longer useful (e.g. we no longer maintain first parent but always just use last parent, as a TODO suggested), then a test like this is just maintenance burden. Something like verifying that required ESM can still be cleared via checkIfCollectableByCounting is probably what's really worth testing & what users would actually care about, and would only break when internal changes introduce a leak that's worth catching.
20 participants
Introduce Module.clearCache() to invalidate CommonJS and ESM module caches with optional resolution context, enabling HMR-like reloads. Document the API and add tests/fixtures to cover cache invalidation behavior.