Skip to content

Commit

Permalink
allow waiting for metro to be torn down
Browse files Browse the repository at this point in the history
Summary:
X-link: facebook/react-native#46620

The following error was thrown when the test `packages/metro/src/integration_tests/__tests__/server-test.js` was running on Metro:
{F1816961963}

It led me to investigate why don't we wait for Metro to be torn down.

Currently we close metro by closing the http server that it launches.
```
const httpServer = await Metro.runServer(/* ... */);
httpServer.close(callback);
```
While we can listen to the callback fired when the server is closed, it only covers one of the systems running internally in metro. The systems that are not covered are:
* File watchers
* File map workers
* Dependency graph
* Bundler

And many systems that were themselves listening to the above like "eslint file map" or the "dependency analysis".

**These systems are closed by us _after_ the server is closed.** This means that a listener to `server.on('close'` would only get the indication where these systems has started to close rather than actually got closed.
https://www.internalfb.com/code/fbsource/[17e03bc6bd86]/xplat/js/tools/metro/packages/metro/src/index.flow.js?lines=359-361

This diff introduces a way to wait for all of metro to be closed.

In this diff I use that new way of listening to Metro closure to get rid of the jest test warning mentioned above in `packages/metro/src/integration_tests/__tests__/server-test.js`:
```
  let serverClosedPromise;

  beforeEach(async () => {
    config = await Metro.loadConfig({
      config: require.resolve('../metro.config.js'),
    });

    let onCloseResolve;
    serverClosedPromise = new Promise(resolve => (onCloseResolve = resolve));
    httpServer = await Metro.runServer(config, {
      reporter: {update() {}},
      onClose: () => {
        onCloseResolve();
      },
    });
  });

  afterEach(async () => {
    httpServer.close();
    await serverClosedPromise;
  });
```

Changelog: [Feature] add `onClose` to `Metro.runServer` configuration allowing to wait for metro and all associated processes to be closed.

Reviewed By: huntie

Differential Revision: D61594124

fbshipit-source-id: e3c50ef986077503bce0caa42a9f9430efc65272
  • Loading branch information
vzaidman authored and facebook-github-bot committed Sep 26, 2024
1 parent de641a6 commit b3f141f
Show file tree
Hide file tree
Showing 16 changed files with 258 additions and 91 deletions.
11 changes: 11 additions & 0 deletions docs/GettingStarted.md
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,17 @@ We recommend using `runMetro` instead of `runServer`, `runMetro` calls this func

* `host (string)`: Where to host the server on.
* `onReady (Function)`: Called when the server is ready to serve requests.
* `onClose (Function)`: Called when the server and all other Metro processes are closed. You can also observe the server close event directly using `httpServer.on('close', () => {});`.

```js
const config = await Metro.loadConfig();

const metroHttpServer = await Metro.runServer(config, {
onClose: () => {console.log('metro server and all associated processes are closed')}
});

httpServer.on('close', () => {console.log('metro server is closed')});
```
* `secure (boolean)`: **DEPRECATED** Whether the server should run on `https` instead of `http`.
* `secureKey (string)`: **DEPRECATED** The key to use for `https` when `secure` is on.
* `secureCert (string)`: **DEPRECATED** The cert to use for `https` when `secure` is on.
Expand Down
14 changes: 6 additions & 8 deletions packages/metro-file-map/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -736,7 +736,7 @@ export default class FileMap extends EventEmitter {
try {
await Promise.all(promises);
} finally {
this._cleanup();
await this._cleanup();
}
this._startupPerfLogger?.point('applyFileDelta_process_end');
this._startupPerfLogger?.point('applyFileDelta_add_start');
Expand All @@ -758,12 +758,11 @@ export default class FileMap extends EventEmitter {
this._startupPerfLogger?.point('applyFileDelta_end');
}

_cleanup() {
async _cleanup() {
const worker = this._worker;

if (worker && typeof worker.end === 'function') {
// $FlowFixMe[unused-promise]
worker.end();
await worker.end();
}

this._worker = null;
Expand Down Expand Up @@ -1113,12 +1112,11 @@ export default class FileMap extends EventEmitter {
clearInterval(this._healthCheckInterval);
}

await this._cleanup();

this._crawlerAbortController.abort();

if (!this._watcher) {
return;
}
await this._watcher.close();
await this._watcher?.close();
}

/**
Expand Down
52 changes: 32 additions & 20 deletions packages/metro-file-map/src/watchers/FSEventsWatcher.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ export default class FSEventsWatcher extends EventEmitter {
+dot: boolean;
+doIgnore: (path: string) => boolean;
+fsEventsWatchStopper: () => Promise<void>;
+watcherInitialReaddirPromise: Promise<void>;
_tracked: Set<string>;

static isSupported(): boolean {
Expand All @@ -77,8 +78,7 @@ export default class FSEventsWatcher extends EventEmitter {
dirCallback: (normalizedPath: string, stats: Stats) => void,
fileCallback: (normalizedPath: string, stats: Stats) => void,
symlinkCallback: (normalizedPath: string, stats: Stats) => void,
// $FlowFixMe[unclear-type] Add types for callback
endCallback: Function,
endCallback: () => void,
// $FlowFixMe[unclear-type] Add types for callback
errorCallback: Function,
ignored?: Matcher,
Expand All @@ -91,9 +91,7 @@ export default class FSEventsWatcher extends EventEmitter {
.on('file', FSEventsWatcher._normalizeProxy(fileCallback))
.on('symlink', FSEventsWatcher._normalizeProxy(symlinkCallback))
.on('error', errorCallback)
.on('end', () => {
endCallback();
});
.on('end', endCallback);
}

constructor(
Expand Down Expand Up @@ -132,29 +130,43 @@ export default class FSEventsWatcher extends EventEmitter {
const trackPath = (filePath: string) => {
this._tracked.add(filePath);
};
FSEventsWatcher._recReaddir(
this.root,
trackPath,
trackPath,
trackPath,
// $FlowFixMe[method-unbinding] - Refactor
this.emit.bind(this, 'ready'),
// $FlowFixMe[method-unbinding] - Refactor
this.emit.bind(this, 'error'),
this.ignored,
);
this.watcherInitialReaddirPromise = new Promise(resolve => {
FSEventsWatcher._recReaddir(
this.root,
trackPath,
trackPath,
trackPath,
() => {
this.emit('ready');
resolve();
},
(...args) => {
this.emit('error', ...args);
resolve();
},
this.ignored,
);
});
}

/**
* End watching.
*/
async close(callback?: () => void): Promise<void> {
await this.watcherInitialReaddirPromise;
await this.fsEventsWatchStopper();
this.removeAllListeners();
if (typeof callback === 'function') {
// $FlowFixMe[extra-arg] - Is this a Node-style callback or as typed?
process.nextTick(callback.bind(null, null, true));
}

await new Promise(resolve => {
// it takes around 100ms for fsevents to release its resounces after
// watching is stopped. See __tests__/server-torn-down-test.js
setTimeout(() => {
if (typeof callback === 'function') {
callback();
}
resolve();
}, 100);
});
}

async _handleEvent(filepath: string) {
Expand Down
16 changes: 11 additions & 5 deletions packages/metro-file-map/src/watchers/NodeWatcher.js
Original file line number Diff line number Diff line change
Expand Up @@ -184,18 +184,24 @@ module.exports = class NodeWatcher extends EventEmitter {
/**
* Stop watching a directory.
*/
_stopWatching(dir: string) {
async _stopWatching(dir: string): Promise<void> {
if (this.watched[dir]) {
this.watched[dir].close();
delete this.watched[dir];
await new Promise(resolve => {
this.watched[dir].once('close', () => process.nextTick(resolve));
this.watched[dir].close();
delete this.watched[dir];
});
}
}

/**
* End watching.
*/
async close(): Promise<void> {
Object.keys(this.watched).forEach(dir => this._stopWatching(dir));
const promises = Object.keys(this.watched).map(dir =>
this._stopWatching(dir),
);
await Promise.all(promises);
this.removeAllListeners();
}

Expand Down Expand Up @@ -345,11 +351,11 @@ module.exports = class NodeWatcher extends EventEmitter {
return;
}
this._unregister(fullPath);
this._stopWatching(fullPath);
this._unregisterDir(fullPath);
if (registered) {
this._emitEvent(DELETE_EVENT, relativePath);
}
await this._stopWatching(fullPath);
}
}

Expand Down
1 change: 1 addition & 0 deletions packages/metro-memory-fs/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -1713,6 +1713,7 @@ class FSWatcher extends EventEmitter {
close() {
this._node.watchers.splice(this._node.watchers.indexOf(this._nodeWatcher));
clearInterval(this._persistIntervalId);
this.emit('close');
}

_listener = (eventType: 'change' | 'rename', filePath: string) => {
Expand Down
17 changes: 9 additions & 8 deletions packages/metro/src/Bundler.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,13 @@ export type BundlerOptions = $ReadOnly<{

class Bundler {
_depGraph: DependencyGraph;
_readyPromise: Promise<void>;
_initializedPromise: Promise<void>;
_transformer: Transformer;

constructor(config: ConfigT, options?: BundlerOptions) {
this._depGraph = new DependencyGraph(config, options);

this._readyPromise = this._depGraph
this._initializedPromise = this._depGraph
.ready()
.then(() => {
config.reporter.update({type: 'transformer_load_started'});
Expand All @@ -55,14 +55,15 @@ class Bundler {
}

async end(): Promise<void> {
await this._depGraph.ready();
await this.ready();

this._transformer.end();
this._depGraph.end();
await this._transformer.end();
await this._depGraph.end();
}

async getDependencyGraph(): Promise<DependencyGraph> {
await this._depGraph.ready();
await this.ready();

return this._depGraph;
}

Expand All @@ -74,7 +75,7 @@ class Bundler {
): Promise<TransformResultWithSource<>> {
// We need to be sure that the DependencyGraph has been initialized.
// TODO: Remove this ugly hack!
await this._depGraph.ready();
await this.ready();

return this._transformer.transformFile(
filePath,
Expand All @@ -85,7 +86,7 @@ class Bundler {

// Waits for the bundler to become ready.
async ready(): Promise<void> {
await this._readyPromise;
await this._initializedPromise;
}
}

Expand Down
5 changes: 2 additions & 3 deletions packages/metro/src/DeltaBundler/Transformer.js
Original file line number Diff line number Diff line change
Expand Up @@ -188,9 +188,8 @@ class Transformer {
};
}

end(): void {
// $FlowFixMe[unused-promise]
this._workerFarm.kill();
async end(): Promise<void> {
await this._workerFarm.kill();
}
}

Expand Down
8 changes: 4 additions & 4 deletions packages/metro/src/DeltaBundler/__tests__/resolver-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -316,7 +316,7 @@ function dep(name: string): TransformResultDependency {
type: 'sourceFile',
filePath: p('/root/a.another'),
});
end();
await end();

resolver = await createResolver({
resolver: {sourceExts: ['js', 'another']},
Expand Down Expand Up @@ -1508,7 +1508,7 @@ function dep(name: string): TransformResultDependency {
type: 'sourceFile',
filePath: p('/root/foo.playstation.js'),
});
end();
await end();

resolver = await createResolver(
{resolver: {platforms: ['playstation']}},
Expand Down Expand Up @@ -2181,7 +2181,7 @@ function dep(name: string): TransformResultDependency {
type: 'sourceFile',
filePath: p('/root/hasteModule.ios.js'),
});
end();
await end();

resolver = await createResolver(config, 'android');
expect(
Expand All @@ -2204,7 +2204,7 @@ function dep(name: string): TransformResultDependency {
type: 'sourceFile',
filePath: p('/root/hasteModule.ios.js'),
});
end();
await end();

resolver = await createResolver(config, 'android');
expect(
Expand Down
5 changes: 2 additions & 3 deletions packages/metro/src/IncrementalBundler.js
Original file line number Diff line number Diff line change
Expand Up @@ -79,10 +79,9 @@ class IncrementalBundler {
this._deltaBundler = new DeltaBundler(this._bundler.getWatcher());
}

end(): void {
async end(): Promise<void> {
this._deltaBundler.end();
// $FlowFixMe[unused-promise]
this._bundler.end();
await this._bundler.end();
}

getBundler(): Bundler {
Expand Down
4 changes: 2 additions & 2 deletions packages/metro/src/Server.js
Original file line number Diff line number Diff line change
Expand Up @@ -196,9 +196,9 @@ class Server {
this._nextBundleBuildNumber = 1;
}

end() {
async end() {
if (!this._isEnded) {
this._bundler.end();
await this._bundler.end();
this._isEnded = true;
}
}
Expand Down
2 changes: 1 addition & 1 deletion packages/metro/src/commands/dependencies.js
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ async function dependencies(args: Args, config: ConfigT) {
}
});

server.end();
await server.end();
return args.output != null
? // $FlowFixMe[method-unbinding]
promisify(outStream.end).bind(outStream)()
Expand Down
Loading

0 comments on commit b3f141f

Please sign in to comment.