Avoid holding the instance lock while waiting for OOBE - #41387
Avoid holding the instance lock while waiting for OOBE#41387Sylvain MOLINIER (SylvainM98) wants to merge 2 commits into
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR improves WSL2 OOBE termination behavior by preventing a lock-held wait during OOBE completion and adding a Windows unit test to validate termination behavior during Modern OOBE.
Changes:
- Added a WSL2 unit test that runs a long OOBE command and validates
--terminatecompletes while another process is blocked. - Updated
WslCoreInstance::CreateLxProcessto wait for either OOBE completion or instance destruction without holdingm_lock.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/windows/UnitTests.cpp | Adds a new unit test covering termination behavior while Modern OOBE is in progress. |
| src/windows/service/exe/WslCoreInstance.cpp | Avoids holding m_lock while waiting for OOBE completion; also unblocks when destruction is signaled. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| const DWORD terminationResult = WaitForSingleObject(terminationProcess.get(), terminationTimeout); | ||
|
|
||
| if (terminationResult == WAIT_TIMEOUT) | ||
| { | ||
| VERIFY_ARE_EQUAL(WaitForSingleObject(terminationProcess.get(), cleanupTimeout), WAIT_OBJECT_0); | ||
| } | ||
|
|
||
| VERIFY_ARE_EQUAL(waitingResult, WAIT_TIMEOUT); | ||
| VERIFY_ARE_EQUAL(terminationResult, WAIT_OBJECT_0); |
Blue (OneBlue)
left a comment
Blue (OneBlue)
left a comment
There was a problem hiding this comment.
Thank you for fixing this ! This approach looks good, couple minor comments
| EMIT_USER_WARNING(wsl::shared::Localization::MessageWaitingForOobe(m_configuration.Name.c_str())); | ||
| m_oobeCompleteEvent.wait(); | ||
|
|
||
| const wil::unique_handle oobeCompleteEvent{ |
There was a problem hiding this comment.
nit: Instead of duplicating the handles, we could switch those to be wil::shared_handle, and then just make a copy in this block
| wsl::windows::common::SubProcess terminationProcessBuilder( | ||
| nullptr, LxssGenerateWslCommandLine(L"--terminate " LXSS_DISTRO_NAME_TEST_L).c_str()); | ||
| const auto terminationProcess = terminationProcessBuilder.Start(); | ||
| const DWORD terminationResult = WaitForSingleObject(terminationProcess.get(), terminationTimeout); |
There was a problem hiding this comment.
nit: I would recommend just calling terminationProcessBuilder.Run(terminationTimeout);. This will throw an exception if the process times out, which will fail the test
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
| const auto oobeCompleteEvent = m_oobeCompleteEvent; | ||
| const auto destroyingEvent = m_destroyingEvent; | ||
| const HANDLE waitHandles[] = {oobeCompleteEvent.get(), destroyingEvent.get()}; | ||
| lock.unlock(); | ||
| const DWORD waitResult = WaitForMultipleObjects(RTL_NUMBER_OF(waitHandles), waitHandles, FALSE, INFINITE); |
There was a problem hiding this comment.
Hmm this is true. The easiest solution I can think of would be to only create m_oobeCompleteEvent if it isn't already created
Summary of the Pull Request
Process creation can wait for a distribution's OOBE to complete while holding
WslCoreInstance::m_lock. BecauseStop()requires the same lock before it cansignal
m_destroyingEvent, terminating the distribution can deadlock until theOOBE exits on its own.
This change releases the instance lock while waiting and waits for either OOBE
completion or instance destruction. After reacquiring the lock, it validates
that the instance is still running before continuing.
PR Checklist
Detailed Description of the Pull Request / Additional comments
WslCoreInstance::CreateLxProcess()currently acquiresm_lockbefore checkingwhether OOBE is still running. If it is, the function waits indefinitely on
m_oobeCompleteEventwithout releasing the lock.WslCoreInstance::Stop()acquiresm_lockbefore settingm_destroyingEvent. This creates the following cycle:m_lockand waits for OOBE completion.m_lockinStop().Stop()cannot signalm_destroyingEventwhile process creation holds the lock.The wait now uses duplicated event handles captured while the instance state is
protected, releases
m_lock, and waits for either OOBE completion or instancedestruction. It then reacquires the lock and revalidates
m_initChannelandm_consoleManagerbefore using them.Validation Steps Performed
Added
ModernOOBETermination, a WSL2 regression test that:Static inspection was performed locally. The Windows build, formatting check,
and test execution are left to repository CI because the integration tests
affect machine-wide WSL state.