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

Replace pathwatcher w/ bundled watcher to catch created & rename events #15440

Merged
merged 28 commits into from Sep 12, 2017

Conversation

Projects
None yet
4 participants
@kuychaco
Member

kuychaco commented Aug 25, 2017

Fixes #14909

This PR addresses three issues related to config settings data loss.

Issue 1

config file stops being watched

Repro steps (on MacOS):

  1. mv config.cson config.cson-bak
  2. open Atom and open config.cson. Note that it's empty except for some settings set by packages. Add editor.fontSize and set it to something unusual. Note that the font updates immediately after setting it.
  3. mv config.cson-bak config.cson
  4. note that the config.cson file updates, but settings are not loaded. That is, if you changed the editor.fontSize normally you observe it updating on the spot, but making changes to the file after moving it will do nothing
  5. reload the editor and note that config.cson has reverted back to its bare state

Turns out that after step 3 we stop watching the config.cson file. Pathwatcher gives us a delete, and subsequent file changes do not result in more change events so we don't reload the user's config and update the in-memory settings. Reloading Atom flushes the stale settings to disk and overwrites the changes that were made resulting in lost settings.

The fix implemented in this PR involves using @smashwilson's new bundled file watching service. We reload the user's config on created, modified, and renamed events.

The bug outlined above resulted in the config file reverting to a previous version of the file prior to being overwritten. It seems that most of the reports in #14909 indicate that the config file was reset completely, which points to a root cause other than what was outlined above.

The rest of this PR aims to make the code around writing to the config file more airtight. There are only two places where we make calls to writeFileSync:

Issue 2

race condition in loadUserConfig when a config file does not yet exist

Previously, there was room for a race condition. An external process could have created a config file after we checked for its existence and before we wrote to the file, potentially resulting in overwriting the changes made by an external process.

      unless fs.existsSync(@configFilePath)
        CSON.writeFileSync(@configFilePath, {})

Solution: Don't write file if it already exists (depends on atom/season#22)

      CSON.writeFileSync(@configFilePath, {flag: 'x'}, {}) # fails if file exists

Issue 3

race condition in save() we write the blank in-memory settings to disk before the first loadUserSettings() call

Previously, there was the possibility that this happened before the in-memory settings were updated to reflect the user's config on disk (@settings is initialized to {} and updated only after loadUserConfig is called). In this case, writing to the file would clobber all the user's settings. This is the most likely cause of the issues reported in #14909.

Solution: Store of set and unset operations run prior to loading the user's config and run them once the in-memory settings are updated.

TODO:

kuychaco and others added some commits Aug 24, 2017

@smashwilson

This comment has been minimized.

Show comment
Hide comment
@smashwilson

smashwilson Sep 12, 2017

Member

Solution: Store of set and unset operations run prior to loading the user's config and run them once the in-memory settings are updated.

I've made a slight change here. When we store pending operations that happen before the first call to .loadUserSettings(), we create the possibility for odd situations like this:

atom.config.set('foo.bar', 10)
atom.config.get('foo.bar')  // => undefined

Instead, what I've done is store the pending operation in a closure and allow it to proceed (without triggering a requestSave() call). The immediately-applied change allows it to be reflected immediately in get() calls, and the pending operation ensures that the change isn't lost after the first load arrives.

Member

smashwilson commented Sep 12, 2017

Solution: Store of set and unset operations run prior to loading the user's config and run them once the in-memory settings are updated.

I've made a slight change here. When we store pending operations that happen before the first call to .loadUserSettings(), we create the possibility for odd situations like this:

atom.config.set('foo.bar', 10)
atom.config.get('foo.bar')  // => undefined

Instead, what I've done is store the pending operation in a closure and allow it to proceed (without triggering a requestSave() call). The immediately-applied change allows it to be reflected immediately in get() calls, and the pending operation ensures that the change isn't lost after the first load arrives.

smashwilson added some commits Sep 12, 2017

@smashwilson smashwilson merged commit 8632e73 into master Sep 12, 2017

3 checks passed

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

@smashwilson smashwilson deleted the ku-reload-config-when-created branch Sep 12, 2017

smashwilson added a commit that referenced this pull request Sep 12, 2017

Merge pull request #15440 from atom/ku-reload-config-when-created
Replace pathwatcher w/ bundled watcher to catch created & rename events
@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Sep 12, 2017

Contributor

⚡️ Issue 3 mentioned in the body definitely sounds suspicious. Nice work you two!

Contributor

nathansobo commented Sep 12, 2017

⚡️ Issue 3 mentioned in the body definitely sounds suspicious. Nice work you two!

@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE Sep 13, 2017

Member

Very nice work here 🔍 👏

Member

Ben3eeE commented Sep 13, 2017

Very nice work here 🔍 👏

smashwilson added a commit that referenced this pull request Sep 13, 2017

Merge pull request #15440 from atom/ku-reload-config-when-created
Replace pathwatcher w/ bundled watcher to catch created & rename events
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment