Implement SDK IO callbacks#14462
Implement SDK IO callbacks#14462JohnMcPMS wants to merge 8 commits intomicrosoft:feature/wsl-for-appsfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Implements SDK IO callbacks for container/process stdout/stderr by introducing an internal IOCallback runner and wiring it into container start and exec flows.
Changes:
- Add
WslcSetProcessSettingsIOCallbackand route init/exec process output through callbacks. - Introduce
IOCallbackworker thread usingMultiHandleWait; updateMultiHandleWaitcancel to atomic and implement move ops. - Expand process options storage and add SDK tests validating callback behavior and handle exclusivity.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/windows/WslcSdkTests.cpp | Adds unit/integration tests covering IO callback registration, init/exec process delivery, handle exclusion, and large output. |
| src/windows/common/relay.hpp | Makes MultiHandleWait cancel flag atomic; declares custom move operations. |
| src/windows/common/relay.cpp | Implements MultiHandleWait move ctor/assignment to copy cancel state via atomic load. |
| src/windows/WslcSDK/wslcsdk.h | Updates public SDK surface: new callback API name, enum casing, and increased process options size. |
| src/windows/WslcSDK/wslcsdk.def | Exports the renamed callback API symbol. |
| src/windows/WslcSDK/wslcsdk.cpp | Wires IO callbacks into container creation/start and process exec; routes WslcGetProcessIOHandle through IOCallback helper. |
| src/windows/WslcSDK/WslcsdkPrivate.h | Adds internal callback options storage and callback lifetime ownership on container/process impls. |
| src/windows/WslcSDK/IOCallback.h | Introduces internal IO callback thread/handle acquisition abstraction. |
| src/windows/WslcSDK/IOCallback.cpp | Implements IO callback setup, IO loop thread, cancellation, and handle retrieval. |
| src/windows/WslcSDK/CMakeLists.txt | Adds new IOCallback files to the SDK build. |
Comments suppressed due to low confidence (1)
src/windows/WslcSDK/wslcsdk.cpp:1
WslcGetProcessIOHandleno longer validatesioHandle(unlikeWslcSetProcessSettingsIOCallback). Passing an unexpected enum value will be forwarded toIWSLAProcess::GetStdHandle, producing less predictable HRESULTs and making the SDK contract harder to reason about. Add the same explicit validation here (e.g., allow only STDIN/STDOUT/STDERR, or at least match the exact set the service supports) and returnE_INVALIDARGfor out-of-range values.
/*++
There was a problem hiding this comment.
Pull request overview
Implements stdout/stderr IO callbacks for processes created through the WSLC SDK, wiring callback registration from WslcProcessSettings through container start/exec and adding SDK tests to validate the behavior.
Changes:
- Adds
WslcSetProcessSettingsIOCallbackand threads that relay process stdout/stderr into user callbacks (init and exec processes). - Updates relay
MultiHandleWaitcancellation state to be atomic and defines explicit move operations. - Adds/updates Windows SDK tests covering callback registration, init/exec behavior, handle consumption rules, and large output.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| test/windows/WslcSdkTests.cpp | Adds unit/integration tests validating IO callback API behavior and end-to-end delivery for init/exec processes. |
| src/windows/common/relay.hpp | Makes MultiHandleWait::m_cancel atomic and replaces macro-based movability with explicit move declarations. |
| src/windows/common/relay.cpp | Implements MultiHandleWait move ctor/assignment using atomic loads. |
| src/windows/WslcSDK/wslcsdk.h | Updates process options size and introduces WslcProcessIOHandle + WslcSetProcessSettingsIOCallback API surface. |
| src/windows/WslcSDK/wslcsdk.def | Exports the new WslcSetProcessSettingsIOCallback symbol. |
| src/windows/WslcSDK/wslcsdk.cpp | Stores/creates IOCallback worker(s) for init and exec processes; routes WslcGetProcessIOHandle through shared helper. |
| src/windows/WslcSDK/WslcsdkPrivate.h | Adds IO callback option storage to internal process/container impls (shared_ptr lifetime management). |
| src/windows/WslcSDK/IOCallback.h | Declares the internal IOCallback worker wrapper. |
| src/windows/WslcSDK/IOCallback.cpp | Implements handle acquisition and relay thread that invokes registered callbacks. |
| src/windows/WslcSDK/CMakeLists.txt | Adds IOCallback sources to the SDK build. |
|
|
||
| void IOCallback::Cancel() | ||
| { | ||
| m_cancelEvent.SetEvent(); |
There was a problem hiding this comment.
nit: Looks like this is never called separately, so might as well have everything in the destructor
|
|
||
| STDAPI WslcSetProcessSettingsIoCallback( | ||
| _In_ WslcProcessSettings* processSettings, _In_ WslcProcessIoHandle ioHandle, _In_opt_ WslcStdIOCallback stdIOCallback, _In_opt_ PVOID context) | ||
| STDAPI WslcSetProcessSettingsIOCallback( |
There was a problem hiding this comment.
I'm just realizing this now, but we need a way to unregister the IO callbacks and synchronize them.
Something like:
HRESULT WslcClearProcessIOCallbacks( WslcProcessSettings* processSettings, _In_ WslcProcessIOHandle ioHandle);
(Once that method returns, the caller is guaranteed that all pending callbacks are completed and that no future callback can be received).
There was a problem hiding this comment.
The current design is that releasing all of the process/container objects related to the process does that. Container+all retrieved InitProcess instances for init processes, just the one process for any created afterward.
We could additionally have a way to explicitly do it, but I don't think that it is strictly necessary. It would have to take in a WslcProcess handle, and we would need to consider if we wanted a second function to take in a container, removing the need to get the init process just to stop the IO callback.
A reason that this might be needed is to ensure that ALL output has been received (the buffers flushed) before teardown as the current release design cancels any future data flow. If the process is still running that isn't really an issue. If it has stopped, we know that there is an end to the pipe and we should get there in reasonably short order.
I think the caller flow would be like:
process = CreateContainerProcess(withIOCallbacks);
WaitForSingle(GetProcessExitEvent(process));
DisableIOCallbacksAndFlushBuffers(process, timeout); // without this, output could be lost in transit when the release is called
ReleaseProcess(process);
the implementation for (pseudoname) DisableIOCallbacksAndFlushBuffers would be like:
if (process is still running right now) // if you don't want this race, wait for the process exit event
{
IOCancel();
}
if (!WaitForIOWithTimeout(timeout))
{
IOCancel();
}
JoinThread();
There was a problem hiding this comment.
Unfortunately this won't be sufficient because the service doesn't guarantee that all the pending IO has been written the pipe after the exit event is signaled.
For context, two things happen when a process terminates
- A zero byte read is signaled on the pipes
- The process exit event is signaled
But those can happen in either order.
Thinking through this, I think this might be something that we've overlooked when we designed this. The only way I can think of fixing this in a way that's not potentially confusing for the other would be something like:
using WslcOnProcessExitedCallback = Function<void(int, Context)>;
using WslcStdIOCallback = Function<void(WslcProcessIOHandle, Context)>;
struct ProcessIOCallback
{
WslcStdIOCallback OnStdout;
WslcStdIOCallback OnStderr;
WslcOnProcessExitedCallback OnExited;
}
STDAPI WslcSetProcessSettingsIOCallback(
_In_ WslcProcessSettings* processSettings,
_In_ const ProcessIOCallback* Callbacks,
_In_opt_ WslcStdIOCallback stdIOCallback,
_In_opt_ PVOID context);
We could make this work by having IOCallback watch all of:
- The stdout handle
- The stderr handle
- The process exit event
And only call OnExited() once all of those are completed, which would guarantee that all IO has been flushed.
Forcing the user to register all callbacks at once will push them to implement this correctly and only release the process after OnExited is called.
Summary of the Pull Request
Implements IO callbacks for processes created through the SDK.
PR Checklist
Detailed Description of the Pull Request / Additional comments
Implements
WslcSetProcessSettingsIOCallbackand the use of those inputs for both initial and subsequent processes. A new thread is started to run theMultiHandleWaitand the container/process objects hold ashared_ptrreference to it. The caller must keep one of the objects alive for the callback thread to keep working.Open question: I made it an error to specify callbacks and not ATTACH on the container start. Should we just force ATTACH if callbacks are provided?
Also makes
MultiHandleWait's cancel member atomic and implements the move operations using a load. There would always be a race if someone is trying toCancelduring a move (don't do that), but it is more explicit now.Validation Steps Performed
Tests added and run.