-
Notifications
You must be signed in to change notification settings - Fork 9.1k
Labels
Area-SettingsIssues related to settings and customizability, for console or terminalIssues related to settings and customizability, for console or terminalIn-PRThis issue has a related PRThis issue has a related PRIssue-BugIt either shouldn't be doing this or needs an investigation.It either shouldn't be doing this or needs an investigation.Needs-Tag-FixDoesn't match tag requirementsDoesn't match tag requirementsPriority-2A description (P2)A description (P2)Product-TerminalThe new Windows Terminal.The new Windows Terminal.
Milestone
Description
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:
terminal/src/cascadia/TerminalSettingsModel/Profile.cpp
Lines 384 to 392 in 52262b0
| 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).
Reactions are currently unavailable
Metadata
Metadata
Assignees
Labels
Area-SettingsIssues related to settings and customizability, for console or terminalIssues related to settings and customizability, for console or terminalIn-PRThis issue has a related PRThis issue has a related PRIssue-BugIt either shouldn't be doing this or needs an investigation.It either shouldn't be doing this or needs an investigation.Needs-Tag-FixDoesn't match tag requirementsDoesn't match tag requirementsPriority-2A description (P2)A description (P2)Product-TerminalThe new Windows Terminal.The new Windows Terminal.