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

Audit our settings model for thread safety issues #18169

Closed
lhecker opened this issue Nov 8, 2024 · 0 comments · Fixed by #18215
Closed

Audit our settings model for thread safety issues #18169

lhecker opened this issue Nov 8, 2024 · 0 comments · Fixed by #18215
Labels
Area-Settings Issues related to settings and customizability, for console or terminal In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Priority-2 A description (P2) Product-Terminal The new Windows Terminal.

Comments

@lhecker
Copy link
Member

lhecker commented Nov 8, 2024

It occurred to me today that we have a couple thread safety issues now. Since a single settings instance is shared across multiple windows (= threads), we must make sure that all getters are thread-safe. An example for such an issue:

winrt::hstring Profile::EvaluatedIcon()
{
// We cache the result here, so we don't search the path for the exe every time.
if (!_evaluatedIcon.has_value())
{
_evaluatedIcon = _evaluateIcon();
}
return *_evaluatedIcon;
}

It should be sufficient to simply find all getter methods that aren't declared as const in the header files. We should fix this not because it's likely to happen, but rather because it's an absolute pain in the ass to debug if it does (= heisenbugs).

@lhecker lhecker added Area-Settings Issues related to settings and customizability, for console or terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Terminal The new Windows Terminal. labels Nov 8, 2024
@lhecker lhecker added this to the Terminal v1.23 milestone Nov 8, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Nov 8, 2024
@lhecker lhecker added Needs-Discussion Something that requires a team discussion before we can proceed and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Nov 8, 2024
@carlos-zamora carlos-zamora removed the Needs-Discussion Something that requires a team discussion before we can proceed label Nov 25, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot added the In-PR This issue has a related PR label Dec 2, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Tag-Fix Doesn't match tag requirements label Dec 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Settings Issues related to settings and customizability, for console or terminal In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Priority-2 A description (P2) Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants