DeltaBundler: Consistently delete unreachable modules and their depen…

…dencies

Summary:
Fixes a bug that can lead to "unknown module" errors at runtime after an incremental build.

The bug occurs when we *defer* the deletion of an unreachable module (because it's a "possible cycle root" = "buffered" by the GC algorithm) but still *eagerly* delete its outbound edges. If the same module becomes reachable again before cycle collection, and is not itself queued for traversal ( = hasn't changed on disk), it would be sent to the client with an empty dependency map, regardless of whether its code has any references to other modules.

(See the added regression test for an annotated example.)

This bug traces to my interpretation of the paper on which I based Metro's GC algorithm in D36403390 (9065257) (*Concurrent Cycle Collection in Reference Counted Systems* - snippets shown below). In the paper, it isn't clear why `Release(s)` calls `Free` conditionally; I now think this isn't fundamental, but is simply an artifact of how the algorithm in the paper does its bookkeeping.

{F803521688}
{F803521380}

In a conventional GC, *objects can't become reachable once they've become unreachable* - or if they can, it's via a weak reference that is checked for validity before dereferencing. So it's safe for original algorithm to mutate an unreachable object without freeing it, but it isn't safe for Metro, where unreachable modules can become reachable before the next GC pass.

There are actually two possible ways to resolve this:
1. Defer mutating the module entirely: If releasing a node that's marked as a possible cycle root, *do nothing* and let the cycle collection pass handle it if necessary.
2. Free the module immediately: Ignore the fact that the node is marked as a possible cycle root, and just delete it as soon as its reference count reaches 0. If something else turns out to depend on the same module, we can recreate it from scratch (at the cost of re-reading from the transformer cache, re-resolving dependencies, etc).

This diff implements approach (2) to be consistent with how we (currently) handle the acyclic case.

NOTE: It might be worth going to the other extreme and deferring *all* deletions (cyclic and acyclic) to avoid recreating modules that haven't actually changed. This might speed up large incremental builds (major changes to `node_modules`, changing source control revisions while Metro is running, etc). That would be a more invasive change so I'm not tackling it here.

Reviewed By: jacdebug, huntie

Differential Revision: D41500887

fbshipit-source-id: 586e9bfdcfcdfc87826ea2e1dbdf3805457522b3

packages/metro/src/DeltaBundler/Graph.js

@@ -81,6 +81,9 @@ export type Result<T> = {
  * modified ones (which is useful for things like Hot Module Reloading).
  **/
 type Delta = $ReadOnly<{
+  // `added` and `deleted` are mutually exclusive.
+  // Internally, a module can be in both `modified` and (either) `added` or
+  // `deleted`. We fix this up before returning the delta to the client.
   added: Set<string>,
   modified: Set<string>,
   deleted: Set<string>,
@@ -198,8 +201,8 @@ export class Graph<T = MixedOutput> {
 
     const modified = new Map<string, Module<T>>();
     for (const path of delta.modified) {
-      // Only report a module as modified if we're not already reporting it as added.
-      if (!delta.added.has(path)) {
+      // Only report a module as modified if we're not already reporting it as added or deleted.
+      if (!delta.added.has(path) && !delta.deleted.has(path)) {
         modified.set(path, nullthrows(this.dependencies.get(path)));
       }
     }
@@ -381,7 +384,6 @@ export class Graph<T = MixedOutput> {
           } else {
             // Mark the addition in the added set.
             delta.added.add(path);
-            delta.modified.delete(path);
           }
           delta.earlyInverseDependencies.set(path, new CountingSet());
 
@@ -638,18 +640,15 @@ export class Graph<T = MixedOutput> {
     this.#gc.color.set(module.path, 'black');
   }
 
-  // Delete an unreachable module from the graph immediately, unless it's queued
-  // for later deletion as a potential cycle root. Delete the module's outbound
-  // edges.
+  // Delete an unreachable module (and its outbound edges) from the graph
+  // immediately.
   // Called when the reference count of a module has reached 0.
   _releaseModule(module: Module<T>, delta: Delta, options: InternalOptions<T>) {
     for (const [key, dependency] of module.dependencies) {
       this._removeDependency(module, key, dependency, delta, options);
     }
     this.#gc.color.set(module.path, 'black');
-    if (!this.#gc.possibleCycleRoots.has(module.path)) {
-      this._freeModule(module, delta);
-    }
+    this._freeModule(module, delta);
   }
 
   // Delete an unreachable module from the graph.
@@ -660,7 +659,6 @@ export class Graph<T = MixedOutput> {
     } else {
       // Mark the deletion in the deleted set.
       delta.deleted.add(module.path);
-      delta.modified.delete(module.path);
     }
 
     // This module is not used anywhere else! We can clear it from the bundle.

packages/metro/src/DeltaBundler/__tests__/Graph-test.js

@@ -2254,6 +2254,71 @@ describe('edge cases', () => {
       deleted: new Set(['/foo', '/bar', '/baz']),
     });
   });
+
+  it('deleting a cycle root, then reintroducing the same module, does not corrupt its dependencies', async () => {
+    Actions.createFile('/quux');
+    Actions.removeDependency('/foo', '/baz');
+    Actions.addDependency('/bar', '/foo');
+    Actions.addDependency('/bundle', '/baz');
+    Actions.addDependency('/foo', '/quux');
+    files.clear();
+
+    /*
+    ┌─────────┐     ┌──────┐     ┌───────┐     ┌──────┐
+    │ /bundle │ ──▶ │ /baz │     │       │ ──▶ │ /bar │
+    └─────────┘     └──────┘     │ /foo  │     └──────┘
+      │                          │       │       │
+      └────────────────────────▶ │       │ ◀─────┘
+                                 └───────┘
+                                   │
+                                   │
+                                   ▼
+                                 ┌───────┐
+                                 │ /quux │
+                                 └───────┘
+    */
+    await graph.initialTraverseDependencies(options);
+
+    // This is a regression test for a bug: Originally `/quux` would get deleted
+    // incorrectly as a result of `/foo` temporarily being unreachable (and not
+    // itself marked for traversal, which would have "rediscovered" `/quux`).
+
+    // The following exact order of operations reproduced the bug:
+    Actions.removeDependency('/bar', '/foo'); // (1)
+    // ^ Deletes an inbound edge while there's at least another one remaining,
+    //   which marks `/foo` as a possible cycle root.
+
+    Actions.removeDependency('/bundle', '/foo'); // (2)
+    // ^ Leaves `/foo` with no inbound edges. With the bug, this would delete
+    //   `/foo`'s dependencies immediately but defer freeing `/foo` itself until
+    //   the cycle collection pass.
+
+    Actions.addDependency('/baz', '/foo'); // (3)
+    // ^ `/foo` has an inbound edge again! If we'd freed `/quux` in (2), it
+    //   would now be missing.
+
+    /*
+    ┌─────────┐     ┌──────┐ (3) ┌───────┐     ┌──────┐
+    │ /bundle │ ──▶ │ /baz │ ━━▶ │       │ ──▶ │ /bar │
+    └─────────┘     └──────┘     │ /foo  │     └──────┘
+      ┆          /               │       │   \   ┆
+      └┈┈┈┈┈┈┈┈┈/┈┈┈┈┈┈┈┈┈┈┈┈┈┈▷ │       │ ◁┈┈\┈┈┘ (1)
+               /  (2)            └───────┘     \
+                                   │
+                                   │
+                                   ▼
+                                 ┌───────┐
+                                 │ /quux │
+                                 └───────┘
+    */
+    expect(
+      getPaths(await graph.traverseDependencies([...files], options)),
+    ).toEqual({
+      added: new Set([]),
+      modified: new Set(['/bundle', '/bar', '/baz']),
+      deleted: new Set([]),
+    });
+  });
 });
 
 describe('require.context', () => {