Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix "unexpected null" handling overlapping file changes #1083

Closed
wants to merge 1 commit into from

Conversation

robhogan
Copy link
Contributor

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

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.

Differential Revision: D49272782

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 14, 2023
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D49272782

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

Fixes facebook#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.
```

Differential Revision: D49272782
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D49272782

@facebook-github-bot
Copy link
Contributor

This pull request has been merged in 75f3927.

robhogan added a commit that referenced this pull request Jan 9, 2024
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
@robhogan robhogan mentioned this pull request Jan 9, 2024
siddarthkay added a commit to status-im/status-mobile that referenced this pull request Jan 12, 2024
### Summary

Metro server often crashes with 
```
status-mobile/node_modules/nullthrows/nullthrows.js:9
  throw error;
  ^

Error: Got unexpected null
```

This commit resolves metro to a commit where this specific issue was fixed.
related PR in metro repo : facebook/metro#1083
robhogan added a commit that referenced this pull request Jan 30, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants