Skip to content

Commit

Permalink
Process files before adding them to FileSystem (re-land)
Browse files Browse the repository at this point in the history
Summary:
*This is a second attempt at D42846676 (1a81060), which was backed out due to bad Haste move handling - details in test plan.*

`metro-file-map` takes raw crawl or watch results, "processes" them with a worker pool (compute hashes, read symlink targets), and makes metadata available to consumers.

Previously, files would be added to the file `Map` before processing, and then additional metadata would be set, and finally (when watching) an event would be emitted.

## Problem

This created a potential bug because files briefly exist, accessible in the `FileSystem` interface, in an intermediate state. Consider:
1. `foo.js` exists and has a SHA1 pre-calculated.
2. A change is made to `foo.js`, watcher backend emits a change event.
3. `FileSystem` is updated with the metadata of the unprocessed file, and an async worker starts processing.
4. Metro receives a bundle request for `foo.js`. It [fails hard](https://github.com/facebook/metro/blob/main/packages/metro/src/node-haste/DependencyGraph.js#L250) because the file has no calculated hash.
5. The worker completes and we populate the SHA1, and emit a 'change' event.

This also complicates `TreeFS`, because symlink targets
aren't guaranteed to be available to it.

## This diff

This diff switches things around so that the `FileSystem` implementation is only updated with ready-processed `FileMetaData`, and events are emitted to `metro-file-map` consumers synchronously with updates to `FileSystem`.

That means `FileSystem` will expose stale, complete state instead of fresher, incomplete state. Besides the fix above, this is okay and indeed preferable, because:
  - Two calls to `getSha1()` for the same file are guaranteed to return the same result unless there's been a 'change' event emitted on that file in the meantime. This is much more predictable.
   - In any case, any file system representation is *always* potentially stale - I/O isn't instant, OS events take time to propagate, so well behaved consumers should already be treating it as such.

## Summary
Currently we
1. Remove all deleted files from the Haste map and file map simultaneously.
2. Update new/changed files in the file map (with incomplete metadata).
3. "Process" new/changed files, updating the file map with complete metadata and adding entries to the module map.

In this diff:
1. Remove all deleted files from the Haste map and file map simultaneously.
2. Process new/changed files, filling in gaps in metadata and adding entries to the module map.
3. Update new/changed files in the file map (with complete metadata).

*(In D42846676 (1a81060), 1 and 2 were reversed)*

Changelog: **[Fix]** Race condition where a very recently modified file might have missing metadata.

Reviewed By: huntie

Differential Revision: D42930236

fbshipit-source-id: 983af37bc8829ebc8b403483177211b578905ffc
  • Loading branch information
robhogan authored and facebook-github-bot committed Feb 6, 2023
1 parent 27c2112 commit baf28ab
Show file tree
Hide file tree
Showing 4 changed files with 89 additions and 122 deletions.
38 changes: 0 additions & 38 deletions packages/metro-file-map/src/HasteFS.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import type {
Glob,
MutableFileSystem,
Path,
VisitMetadata,
} from './flow-types';

import H from './constants';
Expand Down Expand Up @@ -51,33 +50,6 @@ export default class HasteFS implements MutableFileSystem {
this.#files.set(this._normalizePath(filePath), metadata);
}

setVisitMetadata(
filePath: Path,
visitResult: $ReadOnly<VisitMetadata>,
): void {
const metadata = this._getFileData(filePath);
if (!metadata) {
throw new Error('Visited file not found in file map: ' + filePath);
}
metadata[H.VISITED] = 1;

if (visitResult.hasteId != null) {
metadata[H.ID] = visitResult.hasteId;
}

if ('sha1' in visitResult) {
metadata[H.SHA1] = visitResult.sha1;
}

if (visitResult.dependencies != null) {
metadata[H.DEPENDENCIES] = visitResult.dependencies;
}

if (visitResult.symlinkTarget != null) {
metadata[H.SYMLINK] = visitResult.symlinkTarget;
}
}

getSerializableSnapshot(): FileData {
return new Map(
Array.from(this.#files.entries(), ([k, v]: [Path, FileMetaData]) => [
Expand All @@ -97,16 +69,6 @@ export default class HasteFS implements MutableFileSystem {
return (fileMetadata && fileMetadata[H.SIZE]) ?? null;
}

getSymlinkTarget(file: Path): ?string {
const fileMetadata = this._getFileData(file);
if (fileMetadata == null) {
return null;
}
return typeof fileMetadata[H.SYMLINK] === 'string'
? fileMetadata[H.SYMLINK]
: null;
}

getType(file: Path): ?('f' | 'l') {
const fileMetadata = this._getFileData(file);
if (fileMetadata == null) {
Expand Down
16 changes: 11 additions & 5 deletions packages/metro-file-map/src/__tests__/index-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1530,17 +1530,20 @@ describe('HasteMap', () => {

hm_it('does not emit duplicate change events', async hm => {
const e = mockEmitters[path.join('/', 'project', 'fruits')];
mockFs[path.join('/', 'project', 'fruits', 'Tomato.js')] = `
// Tomato!
`;
e.emit(
'all',
'change',
'tomato.js',
'Tomato.js',
path.join('/', 'project', 'fruits'),
MOCK_CHANGE_FILE,
);
e.emit(
'all',
'change',
'tomato.js',
'Tomato.js',
path.join('/', 'project', 'fruits'),
MOCK_CHANGE_FILE,
);
Expand Down Expand Up @@ -1808,8 +1811,11 @@ describe('HasteMap', () => {
);
});

hm_it('ignore directories', async hm => {
hm_it('ignore directory events (even with file-ish names)', async hm => {
const e = mockEmitters[path.join('/', 'project', 'fruits')];
mockFs[path.join('/', 'project', 'fruits', 'tomato.js', 'index.js')] = `
// Tomato!
`;
e.emit(
'all',
'change',
Expand All @@ -1820,8 +1826,8 @@ describe('HasteMap', () => {
e.emit(
'all',
'change',
'tomato.js',
path.join('/', 'project', 'fruits', 'tomato.js', 'index.js'),
path.join('tomato.js', 'index.js'),
path.join('/', 'project', 'fruits'),
MOCK_CHANGE_FILE,
);
const {eventsQueue} = await waitForItToChange(hm);
Expand Down
9 changes: 0 additions & 9 deletions packages/metro-file-map/src/flow-types.js
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,6 @@ export interface FileSystem {
getModuleName(file: Path): ?string;
getSerializableSnapshot(): FileData;
getSha1(file: Path): ?string;
getSymlinkTarget(file: Path): ?string;
getType(file: Path): ?('f' | 'l');

matchFiles(pattern: RegExp | string): Array<Path>;
Expand Down Expand Up @@ -216,7 +215,6 @@ export interface MutableFileSystem extends FileSystem {
remove(filePath: Path): void;
addOrModify(filePath: Path, fileMetadata: FileMetaData): void;
bulkAddOrModify(addedOrModifiedFiles: FileData): void;
setVisitMetadata(filePath: Path, metadata: $ReadOnly<VisitMetadata>): void;
}

export type Path = string;
Expand All @@ -238,13 +236,6 @@ export type ReadOnlyRawModuleMap = $ReadOnly<{
mocks: $ReadOnlyMap<string, Path>,
}>;

export type VisitMetadata = {
hasteId?: string,
sha1?: ?string,
dependencies?: string,
symlinkTarget?: string,
};

export type WatchmanClockSpec =
| string
| $ReadOnly<{scm: $ReadOnly<{'mergebase-with': string}>}>;
Expand Down
Loading

0 comments on commit baf28ab

Please sign in to comment.