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

Avoid serialization race condition with slow package deactivation #17913

Merged
merged 4 commits into from Aug 27, 2018

Conversation

Projects
None yet
2 participants
@captbaritone
Contributor

captbaritone commented Aug 23, 2018

If any package takes longer than one second (the saveStateDebounceInterval) to deactivate, and the unload was triggered by a key or mouse down, mouse event, you can end up in a situation where
sate is serialized after the packages are deactivated.

The result in a bug where panes, such as the File Tree, will randomly be closed when you reload or reopen Atom. We've seen multiple reports of this type of behavior on the Nuclide team, and I suspect this is the source.

This can be reproduced by creating a package that has an artificially slow deactivate method. With such a package enabled, every reload ends up serializing a state where all panes are closed.

I'm a bit nervous about this exact fix, since we have to track every place where it's possible for prepare-to-unload to be fired, without the window actually closing.

I handled the only instance I saw, but the logic is complex enough, that I'm not 100% confident there are not other instances.

If it did happen that prepare-to-unload was fired and some other logic caused the window to not actually close, we could end up in a state where mousedown/keydown events were no longer causing state to get serialized.

@matthewwithanm @maxbrunsfeld @wbinnssmith

Avoid serialization race condition with slow package deactivation
If any package takes longer than one second (the
`saveStateDebounceInterval`) to deactivate, and the unload was triggered
by a key or mouse down, mouse event, you can end up in a situation where
sate is serialized _after_ the packages are deactivated.

The result in a bug where panes, such as the File Tree, will randomly
be closed when you reload or reopen Atom.

This can be reproduced by creating a package that has an artificially
slow `deactivate` method. With such a package enabled, every reload ends
up serializing a state where all panes are closed.

I'm a bit nervous about this exact fix, since we have to track every
place where it's possible for `prepare-to-unload` to be fired, without
the window actually closing.

I handled the only instance I saw, but the logic is complex enough, that
I'm not 100% confident there are not other instances.

If it did happen that `prepare-to-unload` was fired and some other logic
caused the window to not actually close, we could end up in a state
where mousedown/keydown events were no longer causing state to get
serialized.
@captbaritone

This comment has been minimized.

Show comment
Hide comment
@captbaritone

captbaritone Aug 23, 2018

Contributor

Just talked with @matthewwithanm and he pointed me at #17873 which I think means I can get rid of the unload-aborted event. I'll update the PR.

Further, its seems that since the callback triggered by onRequestUnload is deactivating packages, that we should just assume that its return value will always be respected. So, if we return true from our onRequestUnload callback, we can trust that the window is going to be closed and we don't have to worry about a case where the window continues to exist.

Contributor

captbaritone commented Aug 23, 2018

Just talked with @matthewwithanm and he pointed me at #17873 which I think means I can get rid of the unload-aborted event. I'll update the PR.

Further, its seems that since the callback triggered by onRequestUnload is deactivating packages, that we should just assume that its return value will always be respected. So, if we return true from our onRequestUnload callback, we can trust that the window is going to be closed and we don't have to worry about a case where the window continues to exist.

Remove unload-aborted event
This edge case will be handled by #17873
@maxbrunsfeld

This comment has been minimized.

Show comment
Hide comment
@maxbrunsfeld

maxbrunsfeld Aug 24, 2018

Contributor

This looks good to me.

This can be reproduced by creating a package that has an artificially slow deactivate method. With such a package enabled, every reload ends up serializing a state where all panes are closed.

Have you already done some manual testing to verify that this change fixes that scenario? If so, I'm cool with merging this. It looks like you just need to appease the linter; the actual main process tests passed on CI.

Contributor

maxbrunsfeld commented Aug 24, 2018

This looks good to me.

This can be reproduced by creating a package that has an artificially slow deactivate method. With such a package enabled, every reload ends up serializing a state where all panes are closed.

Have you already done some manual testing to verify that this change fixes that scenario? If so, I'm cool with merging this. It looks like you just need to appease the linter; the actual main process tests passed on CI.

@captbaritone

This comment has been minimized.

Show comment
Hide comment
@captbaritone

captbaritone Aug 27, 2018

Contributor

@maxbrunsfeld My manual testing consisted of creating package which deactivates slowly (see below) and testing with it loaded.

Before this PR change, reloading Atom via a hotkey would cause the file tree to be closed after Atom reloads.

After this PR change, reloading Atom via a hotkey keeps the file tree open after Atom reloads.

'use babel';

export default {
  activate(state) {
    console.log('activated slow-deactivator')
  },

  deactivate() {
    console.log('starting to deactivate slow-deactivator', new Date())
    return new Promise(resolve => {
      setTimeout(() => {
        console.log('done deactivating slow-deactivator', new Date())
        resolve();
      }, 2000);
    });
  }
};
Contributor

captbaritone commented Aug 27, 2018

@maxbrunsfeld My manual testing consisted of creating package which deactivates slowly (see below) and testing with it loaded.

Before this PR change, reloading Atom via a hotkey would cause the file tree to be closed after Atom reloads.

After this PR change, reloading Atom via a hotkey keeps the file tree open after Atom reloads.

'use babel';

export default {
  activate(state) {
    console.log('activated slow-deactivator')
  },

  deactivate() {
    console.log('starting to deactivate slow-deactivator', new Date())
    return new Promise(resolve => {
      setTimeout(() => {
        console.log('done deactivating slow-deactivator', new Date())
        resolve();
      }, 2000);
    });
  }
};

@captbaritone captbaritone changed the title from [RFC] Avoid serialization race condition with slow package deactivation to Avoid serialization race condition with slow package deactivation Aug 27, 2018

@captbaritone

This comment has been minimized.

Show comment
Hide comment
@captbaritone

captbaritone Aug 27, 2018

Contributor

From what I can tell the build failure is unrelated to this change. I think this is ready to review/merge.

Contributor

captbaritone commented Aug 27, 2018

From what I can tell the build failure is unrelated to this change. I think this is ready to review/merge.

@maxbrunsfeld

This comment has been minimized.

Show comment
Hide comment
@maxbrunsfeld

maxbrunsfeld Aug 27, 2018

Contributor

@captbaritone On both mac and windows, a test related to state serialization failed. Can you repro this test failure locally if you run tests enough times?

2018-08-27T16:01:49.4262160Z AtomEnvironment
2018-08-27T16:01:49.4275530Z   saving and loading
2018-08-27T16:01:49.4289160Z     it ignores mousedown/keydown events happening after calling unloadEditorWindow
2018-08-27T16:01:49.4303550Z       Expected spy saveState not to have been called.
2018-08-27T16:01:49.4322510Z         at it (/Users/vsts/agent/2.138.4/work/1/s/spec/atom-environment-spec.js:284:37)
2018-08-27T16:01:49.4352810Z         at jasmine.Spec.global.(anonymous function) (/Users/vsts/agent/2.138.4/work/1/s/spec/async-spec-helpers.js:27:22)
2018-08-27T16:01:49.4369860Z       Expected spy saveState not to have been called.
2018-08-27T16:01:49.4386890Z         at it (/Users/vsts/agent/2.138.4/work/1/s/spec/atom-environment-spec.js:290:37)
2018-08-27T16:01:49.4403310Z         at jasmine.Spec.global.(anonymous function) (/Users/vsts/agent/2.138.4/work/1/s/spec/async-spec-helpers.js:27:22)
Contributor

maxbrunsfeld commented Aug 27, 2018

@captbaritone On both mac and windows, a test related to state serialization failed. Can you repro this test failure locally if you run tests enough times?

2018-08-27T16:01:49.4262160Z AtomEnvironment
2018-08-27T16:01:49.4275530Z   saving and loading
2018-08-27T16:01:49.4289160Z     it ignores mousedown/keydown events happening after calling unloadEditorWindow
2018-08-27T16:01:49.4303550Z       Expected spy saveState not to have been called.
2018-08-27T16:01:49.4322510Z         at it (/Users/vsts/agent/2.138.4/work/1/s/spec/atom-environment-spec.js:284:37)
2018-08-27T16:01:49.4352810Z         at jasmine.Spec.global.(anonymous function) (/Users/vsts/agent/2.138.4/work/1/s/spec/async-spec-helpers.js:27:22)
2018-08-27T16:01:49.4369860Z       Expected spy saveState not to have been called.
2018-08-27T16:01:49.4386890Z         at it (/Users/vsts/agent/2.138.4/work/1/s/spec/atom-environment-spec.js:290:37)
2018-08-27T16:01:49.4403310Z         at jasmine.Spec.global.(anonymous function) (/Users/vsts/agent/2.138.4/work/1/s/spec/async-spec-helpers.js:27:22)
@captbaritone

This comment has been minimized.

Show comment
Hide comment
@captbaritone

captbaritone Aug 27, 2018

Contributor

@maxbrunsfeld Ah yes. I can reproduce. That test needs to be updated. Working on that now.

Contributor

captbaritone commented Aug 27, 2018

@maxbrunsfeld Ah yes. I can reproduce. That test needs to be updated. Working on that now.

@captbaritone

This comment has been minimized.

Show comment
Hide comment
@captbaritone

captbaritone Aug 27, 2018

Contributor

Okay. AppVeyor is now passing.

Contributor

captbaritone commented Aug 27, 2018

Okay. AppVeyor is now passing.

@maxbrunsfeld

This comment has been minimized.

Show comment
Hide comment
@maxbrunsfeld

maxbrunsfeld Aug 27, 2018

Contributor

It looks like the VSTS errors all occurred after the tests passed; there were problems uploading artifacts.

Contributor

maxbrunsfeld commented Aug 27, 2018

It looks like the VSTS errors all occurred after the tests passed; there were problems uploading artifacts.

@maxbrunsfeld maxbrunsfeld merged commit 5c74112 into atom:master Aug 27, 2018

2 of 3 checks passed

VSTS: Atom Pull Requests 20180827.3 failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@maxbrunsfeld

This comment has been minimized.

Show comment
Hide comment
@maxbrunsfeld

maxbrunsfeld Aug 27, 2018

Contributor

Nice job tracking this down and fixing it, @captbaritone. This will ship in Atom 1.32.

Contributor

maxbrunsfeld commented Aug 27, 2018

Nice job tracking this down and fixing it, @captbaritone. This will ship in Atom 1.32.

@captbaritone

This comment has been minimized.

Show comment
Hide comment
@captbaritone

captbaritone Aug 27, 2018

Contributor

Awesome. Thanks for the quick review turnaround! 👏

Contributor

captbaritone commented Aug 27, 2018

Awesome. Thanks for the quick review turnaround! 👏

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