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

Prevent broken patches when staging or unstaging lines rapidly #1797

Merged
merged 3 commits into from Nov 14, 2018

Conversation

Projects
None yet
3 participants
@smashwilson
Member

smashwilson commented Nov 14, 2018

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • Suggestion: You can use checklists to keep track of progress for the sections on metrics, tests, documentation, and user research.

Description of the Change

To prevent the generation of broken patches, the FilePatchController (soon to be MultiFilePatchController) pauses staging operations while it waits on a Promise that is intended to be resolved when the FilePatch containing the result of the operation arrives as a component prop. However, staging operations were being resumed too soon for two reasons:

  • If a new FilePatch is already being generated before a staging operation is begun -- triggered by an unrelated filesystem event, for example -- and arrives before the staging operation is complete, the "next patch" promise will resolve too early.

I've addressed this somewhat by waiting for the staging operation promise to resolve first before waiting for the "next patch" promise to resolve.

  • Our FilePatch comparison noticed any difference in referential equality between old and new patches. This isn't good enough: it's very possible for the component to be re-rendered with a different FilePatch object that contains an identical patch.

This one I fixed by comparing the actual toString() serialization of the old and new FilePatches to determine when to resolve the "next patch" promise. I am still doing a referential equality check first so we'll stay nice and quick in the common case of a re-render with the same patch, and not doing a check at all in the common case of not having a pending staging operation underway.

Alternate Designs

I thought about introducing a first-class way to sequence the operations performed within a Repository so we could explicitly express our constraint of "wait for the next FilePatch to be generated after the staging operation has completed." We could do this by tracking a counter in the Present state that we increment each time an operation is started, and finding some way to compare its value among two different result objects. It felt pretty invasive to implement once I started looking at it, though, so I took the simpler tack.

Benefits

Rapidly staging and unstaging lines will no longer create bad patches.

Possible Drawbacks

Well, it might cause some merge conflicts in #1767.

Applicable Issues

Fixes #1796.

Metrics

N/A

Tests

I've tested this by manually reproducing the bug in dev mode, making the change, and verifying that it no longer appears.

We already do have unit tests that are intended to capture this sort of situation. It turns out that writing unit tests to catch asynchronous race conditions is tricky. Who knew!

Documentation

N/A

User Experience Research

  • @vanessayuenn should be unable to reproduce the problem when using this branch

smashwilson added some commits Nov 14, 2018

@smashwilson smashwilson requested a review from vanessayuenn Nov 14, 2018

@coveralls

This comment has been minimized.

coveralls commented Nov 14, 2018

Coverage Status

Coverage increased (+0.009%) to 84.921% when pulling caeaa47 on aw/rapid-staging-response into 33b9063 on master.

selectionMode: 'hunk',
selectedRows: new Set(),
};
this.mouseSelectionInProgress = false;
this.stagingOperationInProgress = false;
this.lastPatchString = null;

This comment has been minimized.

@vanessayuenn

vanessayuenn Nov 14, 2018

Contributor

What is the benefit of storing lastPatchString as a variable vs keeping it in the state?

This comment has been minimized.

@smashwilson

smashwilson Nov 14, 2018

Member

The state key was actually unused entirely. There were no references to it; the patch change promise was being resolved with a comparison between this.props and the prevProps given to componentDidUpdate().

As for storing it as a string though - as part of the process of rendering a new patch, we "adopt" the TextBuffer from the previous one, which leaves the buffer on the old patch empty, so every patch was being detected as "different". Pre-rendering the old one to a string lets us compare them even after we've messed around with it.

@vanessayuenn

I did a manual QA, and can confirm that I don't see broken patches anymore as I rapidly stage & unstage. I only have a question re: moving lastFilePatch out of the component state.

@smashwilson smashwilson merged commit b32160b into master Nov 14, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.009%) to 84.921%
Details

@smashwilson smashwilson deleted the aw/rapid-staging-response branch Nov 14, 2018

@vanessayuenn vanessayuenn referenced this pull request Nov 21, 2018

Closed

v0.23.0-0 QA Review #1806

5 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment