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

Use a weak event handler pattern for GlobalOptionService #68062

Merged
merged 3 commits into from
May 4, 2023

Conversation

sharwell
Copy link
Member

@sharwell sharwell commented May 2, 2023

Force the use of a weak event handler pattern for GlobalOptionService. In the event a PreviewWorkspace fails to be released, it will no longer be GC rooted through this event.

Extracted from #68054

@sharwell sharwell requested a review from a team as a code owner May 2, 2023 19:17
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels May 2, 2023
@CyrusNajmabadi
Copy link
Member

Done with pass.

@tmat
Copy link
Member

tmat commented May 2, 2023

Seems odd to have to change all global option change handlers because we have an issue with one of them.

@sharwell
Copy link
Member Author

sharwell commented May 2, 2023

Seems odd to have to change all global option change handlers because we have an issue with one of them.

We never want GlobalOptionService to be the GC root of an item in the graph. A secondary benefit of this change is GC Root commands in the future will automatically de-prioritize the weak reference from GlobalOptionService and better reveal the true owners of objects in a heap.

/// }
/// </code>
/// </remarks>
private readonly ConditionalWeakTable<object, EventHandler<TEventArgs>> _keepAliveTable = new();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new();

How much overhead is there for each CWT instance? Would there be benefit in making this static?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

➡️ It could result in some unexpected semantics if the table overlapped (e.g. removing the event handler from one event might remove the keep-alive status for a different event).

Note that in the current code, only one of these will exist (GlobalOptionService is a singleton).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants