[POC] Move SocketAsyncEngine to System.Threading so different SafeHandle types can use it.

[tmds]

This implements the API proposed in #47631 (comment) and uses it for Sockets, PipeStream, FileStream, RandomAccess and SerialPort.

[jkotas]

(Reposting from offline email thread.)

@tmds Thank you for prototyping this change.

This is performance sensitive change that will require due diligence to avoid regressions. I would like to see it to come with performance improvements enabled by the consolidated design. It is not clear to me whether the proposed API is going to allow delivering the performance improvements.

Have you done any performance measurements yourself?

@mangod9 can help you with triggering performance runs in our performance labs.

@VSadov and @adamsitnik should be able to help you with the implementation to deliver the performance improvements.

I doubt we will be able to merge this change for .NET 11. We are a few weeks away from starting .NET 11 stabilization, and we are already busy investigating and fixing performance issues introduced earlier in .NET 11 cycle.

NAOT binary size check is failing that suggests this change is introducing size regressions.

[tmds]

This is performance sensitive change that will require due diligence to avoid regressions. I would like to see it to come with performance improvements enabled by the consolidated design. It is not clear to me whether the proposed API is going to allow delivering the performance improvements.

For performance, I don't expect there to be an improvement for Sockets.

It has been suggested (in #47631) that there may be opportunities when the implementation becomes part of Threading but I don't have any suggestions what those might be.

I set the bar here at maintaining the performance for Sockets while making improvements to other I/O APIs.
These I/O APIs should get very good performance for implementing cancellable, async operations as we've heavily optimized the Sockets implementation for that.

Have you done any performance measurements yourself?
@mangod9 can help you with triggering performance runs in our performance labs.

I haven't done any performance measurements. The shape of the API is very similar to what Sockets already used so I expect performance to be similar. It would be very interesting to see what the actual numbers are.

I doubt we will be able to merge this change for .NET 11. We are a few weeks away from starting .NET 11 stabilization, and we are already busy investigating and fixing performance issues introduced earlier in .NET 11 cycle.

The PR put this all together to demonstrate and validate the proposed API is usable for use by the various I/O types. For actual implementation, this should be split. I understand it is unlikely, but perhaps some parts might still land for .NET 11.

NAOT binary size check is failing that suggests this change is introducing size regressions.

Thanks for pointing this out. I will take a look what the test is finding.

[mangod9]

@LoopedBard3 is working on getting a private perf run on this PR to check the impact.

[tmds]

I fixed the NAOT size regression and resolved the merge conficts with main.

[jkotas]

You have mentioned that this change is expected with PipeStream trimming. Do you have any numbers for that? For example, does this reduce size of NAOT binary for a simple app that launches a process via a Process API that uses PipeStream underneath?

[tmds]

You have mentioned that this change is expected with PipeStream trimming. Do you have any numbers for that? For example, does this reduce size of NAOT binary for a simple app that launches a process via a Process API that uses PipeStream underneath?

The size of the app reduced by about 5% (95KB). (#47631 (comment))

[LoopedBard3]

@jkotas Did a test run and analysis with an AI skill. There were no obvious swaths of tests regressed. Looking at socket specific tests in the microbenchmark runs (name contains 'System.Net.Sockets', 'NetworkStream', 'Net.Http', 'SslStream', or 'System.Net.Security'), there are fewer regressions than we see just test to test and more improvements. These tests are somewhat noisy so having overall fewer regressions and more improvements is a good sign. The results are stored in our ADX instance, here is a query that provides all of the individual tests differences baseline and PR, and baseline and baseline + 1 build (to help figure out noise): data explorer.

Let me know if there are any other things I can help with or query for 👍.

[tmds]

@LoopedBard3 thanks for running the perf tests!

That the perf results show no regression is what I was hoping for. The intent is to maintain Socket performance while enabling improvements for the other I/O types.

I think the next step should be API review. For this review, I think we should consider the intended usage BCL-internal and consider that this is the API a public version of the SocketAsyncContext which this PR demonstrates to be useful for the other I/O APIs.

Then for implementation, I think we should start with Threading+Sockets. This is mostly a move of code with some minor refactorings where we want to focus on maintaining the performance.

After that, we can move to the other areas, which we can split:

• PipeStream for improved trimming
• FileStream/RandomAccess for cancellable async I/O on non-blocking fds
• SerialPort for improved performance

For the first step, we need to get the proposed API to the "ready-for-review" state. Who can help to get to this? Perhaps someone from the System.Net.Socket area (@dotnet/ncl) because they have experience/background on what is being proposed.

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
  "version": 5,
  "last_dispatched_commit": "947d619d24eaeb59e78d5a0c210a948994c8a605",
  "last_dispatched_base_ref": "main",
  "last_dispatched_base_sha": "75fa507f92d7560a3ec401ad35eee1897125f032",
  "last_reviewed_commit": "947d619d24eaeb59e78d5a0c210a948994c8a605",
  "last_reviewed_base_ref": "main",
  "last_reviewed_base_sha": "75fa507f92d7560a3ec401ad35eee1897125f032",
  "last_recorded_worker_run_id": "29680710816",
  "review_attempt_commit": "",
  "review_attempt_base_ref": "",
  "review_attempt_count": 0,
  "max_review_attempts": 5,
  "review_history_format": "holistic-review-disclosure-v1",
  "review_history": [
    {
      "commit": "947d619d24eaeb59e78d5a0c210a948994c8a605",
      "review_id": 4730525932
    }
  ]
}

[github-actions]

Holistic Review

Motivation: The problem is real and worthwhile — Sockets' Unix SocketAsyncContext/SocketAsyncEngine (epoll/kqueue readiness dispatch) is duplicated conceptually across the I/O stack, and consolidating it into a shared primitive can unlock trimming/size wins (author reports ~5% / ~95KB for a Process-launch NAOT app) and give Pipes/FileStream/RandomAccess/SerialPort the same well-optimized cancellable async I/O. The linked issue #47631 tracks this investigation.

Approach: The PR hoists the Sockets async engine into a new System.Threading.UnixHandleAsyncContext in CoreLib (with a PollThread, Operation work items, and read/write operation queues), renames the native SocketEvent* PAL to HandleEvent*, and rewires Sockets, System.IO.Pipes, FileStream/RandomAccess, and System.IO.Ports onto it. The core queue state machine is a faithful port of the proven Sockets implementation, which is reassuring. However, the consolidated primitive is surfaced as public API with no approved proposal, and the change is explicitly a broad prototype (~6.1k additions across 55 files spanning CoreLib threading, native PAL, and four consumer libraries) that the author and maintainers agree should be split and perf-validated before landing.

Summary: ⚠️ Needs Changes. This is a valuable direction but is not mergeable as-is: (1) it adds unapproved public API surface (UnixHandleAsyncContext and nested types) with only an investigation issue linked — see inline finding; (2) it is a performance-sensitive rewrite of the hot Unix I/O path with, per the discussion, no perf-lab validation yet, and it previously tripped the NAOT size check; (3) it is far too large and cross-cutting to review or land atomically and should be split (native PAL rename, the shared primitive as internal, then per-consumer adoption). The maintainer thread (jkotas) already reached the same conclusion. Recommend treating this as a prototype: get the API shape reviewed/approved (or keep it internal), split into reviewable PRs, and attach perf-lab numbers for Sockets and the newly-onboarded I/O types before merge.

---

Detailed Findings

❌ API — Unapproved new public surface

See the inline comment on System.Threading.ThreadPool.cs. The new System.Threading.UnixHandleAsyncContext public type family has no api-approved issue; #47631 is unlabeled and is an investigation issue, so there is no approved shape to validate against. The contract is also low-level and epoll/kqueue-specific for a general System.Threading API and is only consumed internally today — strong candidate for staying internal pending API review.

⚠️ Performance — hot-path rewrite lacks validation

This replaces the heavily-tuned Sockets Unix readiness engine and extends it to FileStream/Pipes/SerialPort. jkotas flagged this as performance-sensitive and requested lab measurements; the author stated none had been done and only expected parity for Sockets. A change of this nature must come with perf-lab results (Sockets throughput/latency, and the new FileStream/RandomAccess/Pipe async paths) and a resolved NAOT size regression before it can merge. Not verifiable from static review here — a human must gate on the perf runs.

⚠️ Scope — PR is too large and should be split

~6,100 additions across CoreLib threading, the native PAL rename (SocketEvent* → HandleEvent*, entrypoints, browser/wasi shims), and four consumer libraries in a single change. This cannot be reviewed or bisected safely as one unit. Suggested split: (a) native PAL rename as a no-op refactor, (b) introduce the shared primitive as internal and move Sockets onto it (behavior-preserving), (c) onboard each of Pipes / FileStream+RandomAccess / SerialPort as separate PRs with their own tests and perf data. The author acknowledged this in the thread.

✅ Correctness — queue state machine port looks faithful

The OperationQueue state machine (Ready/Waiting/Processing/Stopped, sequence-number reconciliation, inline-vs-threadpool dispatch, sync timeout handling via ExecuteSync) mirrors the existing Sockets implementation closely, including the subtle observedSequenceNumber protocol and StopAndAbort teardown. Registration uses DangerousAddRef/DangerousRelease around PollThread.TryRegister under the write-queue lock, and AbortAndDispose stops the write queue first to establish IsDisposed. No new concurrency defect stood out relative to the proven original, but the cross-consumer reuse (especially SafeFileHandle operation caching via Interlocked.Exchange) deserves focused review per-consumer once split.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 134.7 AIC · ⌖ 10.7 AIC · ⊞ 10K

[github-actions]

❌ Unapproved new public API surface. This introduces a substantial new public type System.Threading.UnixHandleAsyncContext (plus nested Operation, AsyncResult, OnCompletedResult, SyncResult) exposed from System.Private.CoreLib/System.Threading.ThreadPool. The linked issue #47631 is an investigation issue ("Investigate consolidating SocketAsyncEngine into ThreadPool") and does not carry the api-approved label, so there is no approved API shape to validate against. New public API requires an approved proposal before implementation.

Beyond the process gate, the shape itself needs API-review scrutiny: it is a fairly low-level, Unix-readiness-oriented contract (sequence numbers the caller must round-trip, an abstract Operation : IThreadPoolWorkItem the caller subclasses, IsReadReady/IsWriteReady out-param protocols) surfaced as a general System.Threading public API despite being tightly coupled to epoll/kqueue semantics and today only consumed internally. Recommend keeping this internal (it is only used within the BCL/Sockets) pending API review, or splitting the shared implementation from any public surface. The author has also stated this PR is a prototype intended to be split before landing.