CLI: Rewrite ForEachAsync to use threadpool, add timeout by dkbennett · Pull Request #40675 · microsoft/WSL

dkbennett · 2026-05-29T23:06:34Z

Summary of the Pull Request

This is an improvement of the ForEachAsync generic method to use the windows thread pool and keep the pool full of workers whenever one completes instead of waiting for every worker to be complete before starting another batch. This also adds a timeout to the method so workers do not execute endlessly and does some refactoring for easier debugging.

PR Checklist

Closes: Link to issue #xxx
Communication: I've discussed this with core contributors already. If work hasn't been agreed, this work might be rejected
Tests: Added/updated if needed and all pass
Localization: All end user facing strings can be localized
Dev docs: Added/updated if needed
Documentation updated: If checked, please file a pull request on our docs repo and link it here: #xxx

Detailed Description of the Pull Request / Additional comments

Rewrites ForEachAsync, refactoring out classes and methods into details instead of lambdas for easier debugging and nicer stack frames.
Uses windows thread pool and wil wrappers consistent with elsewhere in the codebase.
Fills the pool and keeps it full as work completes so longer running threads dont hold up work on other threads and we have full utilization for the duration.
Handles timeouts and cancellation of all workers if one worker times out.
Adds to and updates tests.
Updates container stats usage of this to support the cancellation event.

Validation Steps Performed

Manual running of stats with many containers.
Updated unit tests run quickly and cleanly.

Copilot

Pull request overview

This PR rewrites the WSLC ForEachAsync helper to use the Windows thread pool with bounded concurrency and cooperative cancellation, then updates container stats collection and unit coverage to use the new API.

Changes:

Replaces std::async batch execution with a reusable thread-pool worker implementation.
Adds timeout/cancel-drain parameters and updates container stats to pass a cancellation handle.
Expands unit tests for pool sizing, dispatching beyond the initial pool, timeout, and cancellation behavior.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File	Description
`src/windows/wslc/core/AsyncExecution.h`	Reimplements `ForEachAsync` with thread-pool workers, cancellation, and timeout handling.
`src/windows/wslc/tasks/ContainerTasks.cpp`	Updates stats collection to the new `ForEachAsync` signature and timeout parameters.
`test/windows/wslc/WSLCCLIExecutionUnitTests.cpp`	Updates existing tests and adds coverage for new pool and timeout behaviors.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

OneBlue

I think a generic threadpool that we can use across the codebase has a lot of value, thank you for doing this !

I think we need to design it a bit more defensively though (runway threads will unfortunately cause us a lot of pain, so we should avoid "timing out" on cancellation)

I also recommend checking out src\windows\common\WslCoreMessageQueue.h, which uses a threadpool as well. I think a good target would be to move that class to a generic threadpool, something like:

template <TResult>
struct WorkItem
{
    TResult result;
    std::exception_ptr error;
    void Wait();
    void Cancel();    
};

class ThreadPool
{
    ThreadPool(min-threads, max-threads);

    template <TFunction>
    WorkItem SubmitWork(Function &&Work);
}

Should cover both usecases

OneBlue · 2026-06-01T17:49:55Z

+        void CancelAndDrainInFlight() noexcept
+        {
+            context->cancelEvent.SetEvent();
+            ::WaitForMultipleObjects(static_cast<DWORD>(doneHandles.size()), doneHandles.data(), TRUE, cancelDrainMs);


I don't recommend having a "cancel timeout" here. If this timeout ever hits, this will leak threads which will most likely to undefined behavior / crashes

OneBlue · 2026-06-01T17:51:00Z

+        DWORD cancelDrainMs{};
+
+        WorkerPool(size_t workerCount, TWork onWork, std::chrono::milliseconds timeout_, std::chrono::milliseconds cancelDrainTimeout) :
+            context(std::make_shared<TSharedContext>(std::move(onWork))),


Assuming that we don't leak threads, context doesn't need to be a pointer here (could just be a regular class field)

OneBlue · 2026-06-01T17:51:43Z

+        std::vector<HANDLE> doneHandles;
+        std::shared_ptr<TSharedContext> context;
+        std::chrono::milliseconds timeout;
+        DWORD timeoutMs{};


timeoutMs is unused

OneBlue · 2026-06-01T17:53:30Z

+    // On any exception from Launch, Drain, or the user callbacks (onSuccess/onError),
+    // signal cancellation and wait for in-flight workers before rethrowing. This guarantees
+    // no background thread pool callbacks outlive the ForEachAsync call.
+    auto cancelOnError = wil::scope_exit([&pool] { pool.CancelAndDrainInFlight(); });


I recommend doing this in the WorkerPool's destuctor so there's a strong guarantee of no leaked threads

OneBlue · 2026-06-01T17:55:06Z

+            context(std::make_shared<TSharedContext>(std::move(onWork))),
+            timeout(timeout_),
+            timeoutMs(timeout_ == std::chrono::milliseconds::max() ? INFINITE : static_cast<DWORD>(timeout_.count())),
+            cancelDrainMs(static_cast<DWORD>(cancelDrainTimeout.count()))


Given that we have a Launch() method, I'd recommend only creating the workers there. That way this threadpool can dynamically resize itself as needed (which will allow us to reuse it in other places)

OneBlue · 2026-06-01T17:57:09Z

            {
-                onSuccess(*batchResult.result);
+                worker.workerResult.hasError = true;
+                worker.workerResult.error = wil::ResultException{wil::ResultFromCaughtException()};


This catch should cover both catch() cases

OneBlue · 2026-06-01T17:59:27Z

-        TItem item;
+        std::optional<TItem> item;
        std::optional<TResult> result;
        wil::ResultException error{S_OK};


To simplify things, I would recommend storing the error as a std::exception_ptr (null if no error was thrown).

This will have the benefit of allowing us to rethrow non-wil exceptions easily

(and we can get rid of hasError)

OneBlue · 2026-06-01T18:05:18Z

+            // keeping the worker and its SharedContext alive for the full duration of the callback
+            // regardless of WorkerPool lifetime. Re-create the work item each launch so the
+            // context pointer is fresh.
+            auto* ctx = new std::shared_ptr<TSharedWorker>(worker);


Could we merge SharedContext and SharedWorker into one structure that the threadpool owns, and pass a pointer to that structure directly as the context ? This would simplify things a lot, and get rid of the shared_ptr pointer.

If we store them as an std::list, the pointers never get invalidated

OneBlue · 2026-06-01T18:07:49Z

+        NON_MOVABLE(SharedContext);
+
+        TWork onWork;
+        wil::unique_event cancelEvent;


I don't think this cancel event should be in the threadpool structure, since the threadpool logic doesn't actually look at it.

Callers can capture a cancel event in their work callback in they need to

OneBlue · 2026-06-01T18:17:37Z

+        using TSharedContext = SharedContext<TWork>;
+
+        std::vector<std::shared_ptr<TSharedWorker>> workers;
+        std::vector<HANDLE> doneHandles;


If we remove the "timeout on cancel" logic, we can get rid of those

dkbennett · 2026-06-01T19:14:51Z

Converting back to draft to do some rework per @OneBlue comments. There's not huge urgency here so can take a bit of time and ensure it is the best it can be.

Rewrite ForEachAsync to use threadpool, add timeout

22f4a6d

Copilot AI review requested due to automatic review settings May 29, 2026 23:06

Copilot started reviewing on behalf of dkbennett May 29, 2026 23:06 View session

Copilot AI reviewed May 29, 2026

View reviewed changes

Comment thread src/windows/wslc/core/AsyncExecution.h Outdated

Comment thread src/windows/wslc/core/AsyncExecution.h Outdated

Comment thread src/windows/wslc/core/AsyncExecution.h Outdated

Comment thread src/windows/wslc/tasks/ContainerTasks.cpp Outdated

dkbennett added 2 commits May 29, 2026 16:46

Fix shared ptr ownership, address teardown issues

7133a5a

PR feedback adjustments

80061aa

Copilot AI review requested due to automatic review settings May 30, 2026 00:18

Copilot started reviewing on behalf of dkbennett May 30, 2026 00:18 View session

Copilot AI reviewed May 30, 2026

View reviewed changes

Comment thread src/windows/wslc/core/AsyncExecution.h

Comment thread src/windows/wslc/core/AsyncExecution.h

Comment thread src/windows/wslc/core/AsyncExecution.h

Comment thread src/windows/wslc/tasks/ContainerTasks.cpp Outdated

dkbennett added 2 commits May 29, 2026 17:42

PR feedback adjustments

44fbcd1

Fix session capture to by value

2f093fc

Copilot AI review requested due to automatic review settings May 30, 2026 00:46

Copilot started reviewing on behalf of dkbennett May 30, 2026 00:46 View session

Copilot AI reviewed May 30, 2026

View reviewed changes

Comment thread src/windows/wslc/core/AsyncExecution.h Outdated

Comment thread src/windows/wslc/core/AsyncExecution.h Outdated

Comment thread src/windows/wslc/core/AsyncExecution.h

dkbennett added 2 commits May 29, 2026 17:51

PR feedback and fix build break

0be6f83

Add DWORD bounds checking

c578ac9

Copilot AI review requested due to automatic review settings May 30, 2026 00:55

Copilot started reviewing on behalf of dkbennett May 30, 2026 00:55 View session

Copilot AI reviewed May 30, 2026

View reviewed changes

Comment thread src/windows/wslc/core/AsyncExecution.h

Comment thread src/windows/wslc/core/AsyncExecution.h Outdated

Additional fixes

2efe78b

dkbennett marked this pull request as ready for review May 30, 2026 08:12

dkbennett requested a review from a team as a code owner May 30, 2026 08:12

Copilot AI review requested due to automatic review settings May 30, 2026 08:12

Copilot started reviewing on behalf of dkbennett May 30, 2026 08:12 View session

Copilot AI reviewed May 30, 2026

View reviewed changes

Comment thread src/windows/wslc/core/AsyncExecution.h Outdated

Potential fix for pull request finding

993c46d

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings June 1, 2026 16:52

Copilot started reviewing on behalf of dkbennett June 1, 2026 16:53 View session

Copilot AI reviewed Jun 1, 2026

View reviewed changes

OneBlue reviewed Jun 1, 2026

View reviewed changes

dkbennett marked this pull request as draft June 1, 2026 19:14

Conversation

dkbennett commented May 29, 2026

Summary of the Pull Request

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

OneBlue left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkbennett commented Jun 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!