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

[Threading] Fix MEF importing constructors that are not free-threaded #4512

Open
duncanp-lseg opened this issue Jul 11, 2023 · 0 comments
Open
Labels
Infrastructure Project structure, build and release pipeline etc Threading Relates to thread handling - be careful!

Comments

@duncanp-lseg
Copy link
Contributor

duncanp-lseg commented Jul 11, 2023

Description

According to the threading cookbook MEF importing constructors must be free-threaded i.e. be able to run entirely on their callers thread whichever thread that is
=> no thread switching
=> cannot safely call serviceProvider.GetService(...) : we might not be on the UI thread, and if not we can't switch to it.
=> cannot safely call any method on IThreadHandling.

We definitely have older components that break this rule.

In some cases it will be fairly easy to move a serviceProvider.GetService(...) caller out of the constructor.
However, if a component is requesting a service to register for notifications from VS then it will be more difficult to fix e.g. ActiveDocumentTracker (? queue work to finish initialization on idle?)

Affected types

Definitely not ok, but probably straightforward to fix

See #4856.

Definitely not ok - calls GetService: probably straightforward to fix

Addressed here -> #4861. All fixed.

Definitely not ok - tricky to fix

  • ActiveDocumentTracker: calls RunOnUIThread.Run((() => ...) // tricky. Registers for events.
  • ActiveSolutionTracker: tricky. Calls GetService and then registers with the service for VS events
  • FileRenamesEventSource: Calls GetService and then registers with the service for VS events
  • AnalysisIssueSelectionService: Calls GetService and then registers with the service for VS events
  • TaggerProvider (trickier - would benefit from some refactoring first)
  • ActiveSolutionBoundTracker (Might be trickier -> calls configurationProvider.GetConfiguration() and SetBoundSolutionUIContext())

One option that we discussed offline for e.g. IActiveDocumentTracker:

  • add a new interface IActiveDocumentTrackerInitializer with a single Initialize method
  • make the existing IActiveDocumentTracker implement the new interface
  • move the "call VS and register for events" code from the constructor to the new Initialize method
  • make a package MEF-import the new IActiveDocumentTrackerInitializer interface and call the Initialize method. It would need to be a package that loads early in the process, as VS events won't be handled until that package has loaded.

Possibly not ok

Addressed here -> #4856

Ok currently, but could be modified to not make extra calls in the constructor

  • RuleInfoTranslator: calls xamlTranslator = xamlTranslatorFactory.Create();. This looks to be thread-safe currently. However, it would be trivial to delay the call until the translator is actually required.
  • SimpleRuleHelpXamlBuilder: calls ruleHelpXamlTranslator = ruleHelpXamlTranslatorFactory.Create(); // same - looks ok, but would be easy to make the construction lazy.
  • StaticXamlStorage: similar. Calls var ruleHelpXamlTranslator = ruleHelpXamlTranslatorFactory.Create(); // the rest of the setup in the constructor is already lazy.
  • TelemetryManager: probably ok. Need to check the effect of the calls it makes.

Specific issues tracked by other tickets

@duncanp-lseg duncanp-lseg added Infrastructure Project structure, build and release pipeline etc Threading Relates to thread handling - be careful! labels Jul 11, 2023
duncanp-lseg pushed a commit that referenced this issue Sep 4, 2023
- partial. The basic test infrastructure works, but there are dozens of types that cannot be imported
because lots of our MEF constructors fetch services and perform work (see #4512)

Relates to #4825
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Infrastructure Project structure, build and release pipeline etc Threading Relates to thread handling - be careful!
Projects
None yet
Development

No branches or pull requests

1 participant