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

[CP] Fix race condition in PluginManager #47321

Closed
scheglov opened this issue Sep 29, 2021 · 6 comments
Closed

[CP] Fix race condition in PluginManager #47321

scheglov opened this issue Sep 29, 2021 · 6 comments
Assignees
Labels
cherry-pick-approved Label for approved cherrypick request merge-to-stable type-task

Comments

@scheglov
Copy link
Contributor

commit(s) to merge: 7ee2744

merge instructions: clean merge

What is the issue: A race condition, see flutter/flutter#90868 (comment)

What is the fix: Remove the plugin when it stops, but only when the expected instance stops.

Why cherrypick: This affects users of analyze plugins.

Risk: low

Link to original issue(s): flutter/flutter#90868

/cc @kevmoo @mit-mit @whesse @athomas @vsmenon @devoncarew

@scheglov scheglov added the cherry-pick-review Issue that need cherry pick triage to approve label Sep 29, 2021
@devoncarew
Copy link
Member

This affects users of analyze plugins.

What would trigger this? Does this happen over time to all plugin users? Is it triggered by things that would rebuild the analysis context?

I'm trying to get a sense for the impact here (# of users impacted, what percentage of those users, ...).

@scheglov
Copy link
Contributor Author

This can be triggered by a change to the file system that causes re-creating analysis contexts, such as dart pub get, which updates package_config.json. When this happens, we ask each plugin to stop, then create new analysis contexts, and start (almost always the same) plugins again. And after we started new plugin instances we get this notification that the old instance stopped, so we used to forget about plugin instances.

So, after every dart pub get you get a new plugin instance. Run it 10 times, get 10 instances.

@devoncarew devoncarew added cherry-pick-approved Label for approved cherrypick request and removed cherry-pick-review Issue that need cherry pick triage to approve labels Sep 30, 2021
@devoncarew
Copy link
Member

Approved -

@athomas
Copy link
Member

athomas commented Oct 7, 2021

Assuming this is "merge-to-stable"? The fix should be included in the next beta by default.

@devoncarew
Copy link
Member

Yes, sorry, this is a merge-to-stable item.

@athomas
Copy link
Member

athomas commented Oct 13, 2021

Cherry-picked to stable in 4ac35a7 (2.14.4).

@athomas athomas closed this as completed Oct 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick-approved Label for approved cherrypick request merge-to-stable type-task
Projects
None yet
Development

No branches or pull requests

4 participants