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

Write config file atomically #17774

Merged
merged 1 commit into from Aug 6, 2018

Conversation

Projects
None yet
3 participants
@matthewwithanm
Member

matthewwithanm commented Aug 3, 2018

Description of the Change

We (the Nuclide team) continue to have reports of #17060 from Windows users, even after #17166. The cause isn't certain, however, we do know that the before-quit event isn't reliable on Windows. In any case, atomic writes (write + move) are less error-prone.

I rolled back the fix from #17060 to get back to a state where the issue could be reproduced somewhat reliably, then made the change an was unable to repro. This isn't a guaranteed fix (since the same could be said about #17060), but it seems more robust.

I was initially worried about the watcher not picking up the replace, but I tested that file updates were reflected in UI and that UI updates were mirrored to the file on both MacOS and Windows and we're good.

Alternate Designs

None.

Possible Drawbacks

  • We're doing two operations (write and move) where we used to do one so this is going to be slower.
  • There could be something subtle with the watcher I didn't pick up on when testing.
  • I haven't verified on Linux.

Verification Process

  • For both MacOS and Windows:
    • Update editor.fontSize in my config.cson and see change in Atom.
    • Use ⌘+ to increase the font size and see change in config.cson.
  • Install @Arcanemagus's settings-wiper from #17060 and repeat his instructions in that issue for wiping settings many (~50) times without settings loss.

Applicable Issues

cc @ebluestein

@Arcanemagus

This comment has been minimized.

Show comment
Hide comment
@Arcanemagus

Arcanemagus Aug 3, 2018

Contributor

I was unable to get the config to reset on a build based on 841b01a, so at worst from what I can tell it is the same as what we currently have published.

A few things of interesting note:

settings-wiper is actually designed to write to the config whenever it is deactivated, which should be happening to each instance as every window closes. So theoretically if you are testing with 10 windows the numbers it displays should go up by 10 each time. On the current -nightly builds the number never advances when multiple windows are restarted at the same time. On this branch I had a total of 6 writes to the config file actually get through during my test restarts.

I have noticed a potential downside of this PR: The %TEMP% directory gets spammed with config files. Some are written but never got moved, others the file was created, but has no content (and obviously wasn't moved). It's possible these 0 byte config files would have broken the settings before this PR?
image

Contributor

Arcanemagus commented Aug 3, 2018

I was unable to get the config to reset on a build based on 841b01a, so at worst from what I can tell it is the same as what we currently have published.

A few things of interesting note:

settings-wiper is actually designed to write to the config whenever it is deactivated, which should be happening to each instance as every window closes. So theoretically if you are testing with 10 windows the numbers it displays should go up by 10 each time. On the current -nightly builds the number never advances when multiple windows are restarted at the same time. On this branch I had a total of 6 writes to the config file actually get through during my test restarts.

I have noticed a potential downside of this PR: The %TEMP% directory gets spammed with config files. Some are written but never got moved, others the file was created, but has no content (and obviously wasn't moved). It's possible these 0 byte config files would have broken the settings before this PR?
image

@maxbrunsfeld

⚡️

@maxbrunsfeld maxbrunsfeld merged commit 53218b9 into master Aug 6, 2018

4 checks passed

VSTS: Atom Pull Requests 1.31.0-dev+20180803.2 succeeded
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@maxbrunsfeld maxbrunsfeld deleted the fb-mdt-atomic-config-saves branch Aug 6, 2018

@maxbrunsfeld

This comment has been minimized.

Show comment
Hide comment
@maxbrunsfeld
Contributor

maxbrunsfeld commented Aug 6, 2018

Thanks @matthewwithanm!

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