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

Race condition during AppLogic::ReloadSettings and all reads of _settings #18176

Open
lhecker opened this issue Nov 11, 2024 · 0 comments · May be fixed by #18215
Open

Race condition during AppLogic::ReloadSettings and all reads of _settings #18176

lhecker opened this issue Nov 11, 2024 · 0 comments · May be fixed by #18215
Labels
Area-Quality Stability, Performance, Etc. In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. Severity-Crash Crashes are real bad news.

Comments

@lhecker
Copy link
Member

lhecker commented Nov 11, 2024

Windows Terminal version

All of them

Windows build number

No response

Other Software

No response

Steps to reproduce

Reduce startup cost and make Windows Terminal launch faster.

Expected Behavior

No response

Actual Behavior

Windows Terminal will randomly crash because now that it starts a lot faster (relatively speaking) the main- and window-thread(s) will call ReloadSettings() concurrently.

Additionally, we swap out the _settings member on the main thread while window threads may currently be reading from it. This is problematic because after optimizations the pointer to _settings may be cached in a register or on the stack and so we may end up reading from a settings instance that has since been deleted.

Sounds like a rare occurrence, but with just the right timing (which I now have), this crashes the app 99% of the time during startup. I suspect it also happens on main, but simply less often.

@lhecker lhecker added Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Nov 11, 2024
@carlos-zamora carlos-zamora added Product-Terminal The new Windows Terminal. Area-Quality Stability, Performance, Etc. Severity-Crash Crashes are real bad news. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Nov 13, 2024
@carlos-zamora carlos-zamora added this to the Terminal v1.23 milestone Nov 13, 2024
@lhecker lhecker linked a pull request Nov 19, 2024 that will close this issue
@microsoft-github-policy-service microsoft-github-policy-service bot added the In-PR This issue has a related PR label Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Quality Stability, Performance, Etc. In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. Severity-Crash Crashes are real bad news.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants