Avoid SocketException flood in NamedPipeClientStream on Unix#125872
Conversation
When connecting to a non-existent named pipe on Linux, TryConnect creates a Socket and calls Connect on every retry iteration, throwing and catching a SocketException each time. This floods FirstChanceException handlers and wastes CPU on exception allocation. Add a Stat check before the socket path: if the pipe file doesn't exist (ENOENT), return false immediately without allocating a Socket or throwing. Permission errors and all other conditions still reach socket.Connect so they surface their specific exceptions. Fix dotnet#117718 Signed-off-by: haltandcatchwater <angeloregalbutophotography@gmail.com>
|
Tagging subscribers to this area: @dotnet/area-system-io |
|
Note This comment was generated by GitHub Copilot. @EgorBot -linux_amd -osx_arm64 using System.IO.Pipes;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
BenchmarkSwitcher.FromAssembly(typeof(Bench).Assembly).Run(args);
[MemoryDiagnoser]
public class Bench
{
[Benchmark]
public void Connect_ServerAvailable()
{
string pipeName = $"bench_{Guid.NewGuid():N}";
using var server = new NamedPipeServerStream(pipeName, PipeDirection.InOut, 1, PipeTransmissionMode.Byte, PipeOptions.Asynchronous);
var waitTask = server.WaitForConnectionAsync();
using var client = new NamedPipeClientStream(".", pipeName, PipeDirection.InOut);
client.Connect(5000);
waitTask.GetAwaiter().GetResult();
}
[Benchmark]
public void Connect_ServerNotAvailable()
{
using var client = new NamedPipeClientStream(".", $"bench_missing_{Guid.NewGuid():N}", PipeDirection.InOut);
try
{
client.Connect(100);
}
catch (TimeoutException) { }
}
} |
Address review feedback: run the SocketException counting test in an isolated process via RemoteExecutor to prevent other concurrent tests from triggering FirstChanceException and causing flaky results.
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "459bf9a960610e6fd1eb29a3f8056aa262fb52e3",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "45218813655f28bf4f5285ae92f09abd85c02d44",
"last_reviewed_commit": "",
"last_reviewed_base_ref": "",
"last_reviewed_base_sha": "",
"last_recorded_worker_run_id": "",
"review_attempt_commit": "459bf9a960610e6fd1eb29a3f8056aa262fb52e3",
"review_attempt_base_ref": "main",
"review_attempt_count": 1,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": []
} |
There was a problem hiding this comment.
Holistic Review
Motivation: Justified. On Unix, NamedPipeClientStream.Connect() waiting on a non-existent pipe spins in ConnectInternal, and each TryConnect iteration allocates a Socket and throws/catches a SocketException, flooding FirstChanceException handlers and wasting CPU on exception allocation (issue #117718). This is a real, observable cost.
Approach: Sound and minimal. A Interop.Sys.Stat check gates the socket allocation, and it short-circuits only on ENOENT, returning false so the existing polling loop keeps retrying. Because a non-existent socket path already surfaced as a retryable SocketException (mapped to the eventual TimeoutException, as covered by ClientConnect_Throws_Timeout_When_Pipe_Not_Found), returning false here preserves the observable behavior while skipping the exception. Permission (EACCES) and all other Stat failures fall through to socket.Connect, so their specific exceptions are unchanged. The TOCTOU window is benign and correctly documented: if the file appears later, the next iteration picks it up.
Summary: ✅ LGTM. Clean, well-scoped fix with a correctly-isolated regression test. The test uses RemoteExecutor to isolate the process-global FirstChanceException handler and asserts a near-zero SocketException count, which validates the intended behavior. Existing timeout behavior is preserved. No blocking issues found.
Minor, non-blocking observations (no action required):
Statfollows symlinks, so a dangling symlink at the pipe path would now short-circuit onENOENTrather than reachingConnect; this is an extreme edge case with negligible real-world impact and the resulting retry/timeout behavior is equivalent.- The extra
Statsyscall per retry iteration is far cheaper than the previous socket-allocation-plus-exception path, so this is a net performance win.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 68.9 AIC · ⌖ 10.4 AIC · ⊞ 10K
Resolves #117718. Following up on diagnosis by @lindexi.
When
NamedPipeClientStream.Connect()waits for a non-existent pipe on Linux, the internal retry loop creates aSocketand callsConnect()on every iteration. Each attempt throws and catches aSocketException, floodingFirstChanceExceptionhandlers and wasting CPU on exception allocation.This change adds a
Statcheck at the top ofTryConnect: if the socket file doesn't exist (ENOENT), the method returnsfalseimmediately without allocating aSocketor throwing. Permission errors (EACCES) and all other conditions still fall through to the existingsocket.Connectpath, preserving their specific exception behavior.The check introduces a benign TOCTOU window: if the pipe file appears between the
Statcall and the next loop iteration,ConnectInternal's polling loop picks it up on the subsequent retry. This does not reintroduce the exception flood since the file now exists andConnectwill either succeed or fail with a non-retryable error.Changes
NamedPipeClientStream.Unix.cs: AddedInterop.Sys.Stat/ENOENTguard before socket allocation inTryConnectNamedPipeTest.Specific.cs: Added Unix-specific test that verifies connecting to a non-existent pipe does not floodFirstChanceExceptionwithSocketExceptionsTest plan
ClientConnect_PipeNotFound_DoesNotFloodFirstChanceExceptions— connects to a non-existent pipe for 500ms, asserts fewer than 5SocketExceptions viaFirstChanceException(previously hundreds/thousands)ClientConnect_Throws_Timeout_When_Pipe_Not_Found— verifiesTimeoutExceptionstill thrown (unchanged behavior)