Skip to content

Commit

Permalink
Fix edge case where files may briefly "disappear" during processing
Browse files Browse the repository at this point in the history
Summary:
Currently, when a file is modified on disk
 1. The watcher backend emits a change event
 2. `FileMap` receives the event, and deletes the in-memory record of the file (including from the Haste map, etc)
 3. A worker processes the file
 4. `FileMap` re-adds the file with new metadata

If Metro is currently working (eg, building a bundle) it may attempt to read from the Haste or file maps between 2 and 4, and find that the file appears not to exist, which could fail the build either during resolution or transformation.

This bug was introduced in [61c6066](61c6066) - prior to that, consumers would only receive snapshots of the data taken at "safe" points.

Instead, it'd better for the file map to return the last known state of the file until it has updated state, as long as all exposed data is self-consistent. With this change, we move all removal logic under the `'delete'` event handler.

```
 - **[Fix]**: Fix edge case where files may appear missing during bundling/resolution while being modified.
```

Reviewed By: huntie

Differential Revision: D67762435

fbshipit-source-id: 85a1799662aa0adc8bfb9a326db93608b85dc97f
  • Loading branch information
robhogan authored and facebook-github-bot committed Jan 3, 2025
1 parent dd3e56c commit 2a0b548
Show file tree
Hide file tree
Showing 2 changed files with 73 additions and 11 deletions.
68 changes: 67 additions & 1 deletion packages/metro-file-map/src/__tests__/index-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,10 @@ jest.mock('../crawlers/watchman', () =>
) {
const relativeFilePath = path.relative(rootDir, file);
if (list[file]) {
const hash = computeSha1 ? mockHashContents(list[file]) : null;
const isSymlink = typeof list[file].link === 'string';
if (!isSymlink || includeSymlinks) {
const hash =
!isSymlink && computeSha1 ? mockHashContents(list[file]) : null;
changedFiles.set(relativeFilePath, [
'',
32,
Expand Down Expand Up @@ -153,6 +154,16 @@ jest.mock('fs', () => ({
},
}));

jest.mock('../worker.js', () => ({
worker: mockWorkerFn,
}));

const mockWorkerFn = jest
.fn()
.mockImplementation((...args) =>
jest.requireActual('../worker').worker(...args),
);

const object = data => Object.assign(Object.create(null), data);
const createMap = obj => new Map(Object.entries(obj));
const assertFileSystemEqual = (fileSystem: FileSystem, fileData: FileData) => {
Expand Down Expand Up @@ -1677,6 +1688,61 @@ describe('FileMap', () => {
expect(eventsQueue).toHaveLength(1);
});

fm_it(
'file data is still available during processing',
async hm => {
const e = mockEmitters[path.join('/', 'project', 'fruits')];
const {fileSystem, hasteMap} = await hm.build();
// Pre-existing file
const bananaPath = path.join('/', 'project', 'fruits', 'Banana.js');
expect(fileSystem.linkStats(bananaPath)).toEqual({
fileType: 'f',
modifiedTime: 32,
});
const originalHash = fileSystem.getSha1(bananaPath);
expect(typeof originalHash).toBe('string');

mockFs[bananaPath] = `
// Modified banana!
`;
e.emitFileEvent({
event: 'touch',
relativePath: 'Banana.js',
metadata: {
type: 'f',
modifiedTime: 33,
size: 500,
},
});

// Wait for the a worker job to be enqueued, but not yet resolved
const initialWorkerCalls = mockWorkerFn.mock.calls.length;
await null;
expect(mockWorkerFn).toHaveBeenCalledTimes(initialWorkerCalls + 1);

// Initially, expect same data as before
expect(fileSystem.linkStats(bananaPath)).toEqual({
fileType: 'f',
modifiedTime: 32,
});
expect(fileSystem.getSha1(bananaPath)).toBe(originalHash);
expect(hasteMap.getModule('Banana')).toBe(bananaPath);

const {eventsQueue} = await waitForItToChange(hm);
expect(eventsQueue).toHaveLength(1);

// After the 'change' event is emitted, we should have new data
expect(fileSystem.linkStats(bananaPath)).toEqual({
fileType: 'f',
modifiedTime: 33,
});
const newHash = fileSystem.getSha1(bananaPath);
expect(typeof newHash).toBe('string');
expect(newHash).not.toBe(originalHash);
},
{config: {computeSha1: true}},
);

fm_it(
'suppresses backend symlink events if enableSymlinks: false',
async hm => {
Expand Down
16 changes: 6 additions & 10 deletions packages/metro-file-map/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -963,16 +963,6 @@ export default class FileMap extends EventEmitter {
return null;
};

// If it's not an addition, delete the file and all its metadata
if (linkStats != null) {
this._removeIfExists(
fileSystem,
hasteMap,
mockMap,
relativeFilePath,
);
}

// If the file was added or modified,
// parse it and update the haste map.
if (change.event === 'touch') {
Expand Down Expand Up @@ -1017,6 +1007,12 @@ export default class FileMap extends EventEmitter {
// This is expected for deletion of an ignored file.
return null;
}
this._removeIfExists(
fileSystem,
hasteMap,
mockMap,
relativeFilePath,
);
enqueueEvent({
modifiedTime: null,
size: null,
Expand Down

0 comments on commit 2a0b548

Please sign in to comment.