Skip to content

Commit

Permalink
Fix "unexpected null" handling overlapping file changes (#1083)
Browse files Browse the repository at this point in the history
Summary:
Pull Request resolved: #1083

Fixes a crash where a race in overlapping file change processing could occasionally throw "unexpected null".

Fixes #1015

## Root cause
The problem logic is in some instrumentation code introduced in D40346848.

`metro-file-map` batches changes using `setInterval(emitChange, 30)`. The instrumentation attempted to record the time of the first event in a given batch, and then "reset the clock" when the batch is emitted to the consumer.

The problem occurred when an emit interval fell such that a non-empty `emitChange` occurred *during* the async `_processFile` step of a subsequent change. The first emit would reset the "start time" to `null`, and the second `emitChange` would fail at `nullthrows(eventStartTimestamp)`, because `eventStartTimestamp` is only set *before* `_processFile` starts (and only if already `null`).

The (over)writing of `eventStartTimestamp` was flawed, because we don't know at the start of change processing that this event is going to be the start of a batch - this only becomes clear when an event is subsequently enqueued for emit after file processing.

## This diff
This changes the recording of the start time such that we take a timestamp on every change, and record it as the batch start time on the creation of a new batch. We rearrange the various moving parts into a nullable object, so that "first event time" is tightly coupled to the batch it describes, and can never be `null` for a non-empty batch.

## Changelog
```
 * **[Fix]:** Fix "unexpected null" crash when handling a batch of file changes.
```

Reviewed By: huntie

Differential Revision: D49272782

fbshipit-source-id: 8e1a4ebb6876fad982af3aa8a1922c6f14236040
  • Loading branch information
robhogan committed Jan 9, 2024
1 parent 2aa1abf commit 4cc6b08
Showing 1 changed file with 44 additions and 29 deletions.
73 changes: 44 additions & 29 deletions packages/metro-file-map/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -904,33 +904,39 @@ export default class HasteMap extends EventEmitter {
const rootDir = this._options.rootDir;

let changeQueue: Promise<null | void> = Promise.resolve();
let eventsQueue: EventsQueue = [];
let eventStartTimestamp = null;
let nextEmit: ?{
eventsQueue: EventsQueue,
firstEventTimestamp: number,
firstEnqueuedTimestamp: number,
} = null;

const emitChange = () => {
if (eventsQueue.length) {
const hmrPerfLogger = this._options.perfLoggerFactory?.('HMR', {
key: this._getNextChangeID(),
if (nextEmit == null || nextEmit.eventsQueue.length === 0) {
// Nothing to emit
return;
}
const {eventsQueue, firstEventTimestamp, firstEnqueuedTimestamp} =
nextEmit;
const hmrPerfLogger = this._options.perfLoggerFactory?.('HMR', {
key: this._getNextChangeID(),
});
if (hmrPerfLogger != null) {
hmrPerfLogger.start({timestamp: firstEventTimestamp});
hmrPerfLogger.point('waitingForChangeInterval_start', {
timestamp: firstEnqueuedTimestamp,
});
if (hmrPerfLogger != null) {
hmrPerfLogger.start({timestamp: nullthrows(eventStartTimestamp)});
hmrPerfLogger.point('waitingForChangeInterval_start', {
timestamp: nullthrows(eventStartTimestamp),
});
hmrPerfLogger.point('waitingForChangeInterval_end');
hmrPerfLogger.annotate({
int: {eventsQueueLength: eventsQueue.length},
});
hmrPerfLogger.point('fileChange_start');
}
const changeEvent: ChangeEvent = {
logger: hmrPerfLogger,
eventsQueue,
};
this.emit('change', changeEvent);
eventsQueue = [];
eventStartTimestamp = null;
hmrPerfLogger.point('waitingForChangeInterval_end');
hmrPerfLogger.annotate({
int: {eventsQueueLength: eventsQueue.length},
});
hmrPerfLogger.point('fileChange_start');
}
const changeEvent: ChangeEvent = {
logger: hmrPerfLogger,
eventsQueue,
};
this.emit('change', changeEvent);
nextEmit = null;
};

const onChange = (
Expand Down Expand Up @@ -975,15 +981,14 @@ export default class HasteMap extends EventEmitter {
return;
}

if (eventStartTimestamp == null) {
eventStartTimestamp = performance.timeOrigin + performance.now();
}
const onChangeStartTime = performance.timeOrigin + performance.now();

changeQueue = changeQueue
.then(async () => {
// If we get duplicate events for the same file, ignore them.
if (
eventsQueue.find(
nextEmit != null &&
nextEmit.eventsQueue.find(
event =>
event.type === type &&
event.filePath === absoluteFilePath &&
Expand All @@ -1001,11 +1006,21 @@ export default class HasteMap extends EventEmitter {
const linkStats = fileSystem.linkStats(relativeFilePath);

const enqueueEvent = (metadata: ChangeEventMetadata) => {
eventsQueue.push({
const event = {
filePath: absoluteFilePath,
metadata,
type,
});
};
if (nextEmit == null) {
nextEmit = {
eventsQueue: [event],
firstEventTimestamp: onChangeStartTime,
firstEnqueuedTimestamp:
performance.timeOrigin + performance.now(),
};
} else {
nextEmit.eventsQueue.push(event);
}
return null;
};

Expand Down

0 comments on commit 4cc6b08

Please sign in to comment.