Anonymous pipe resource leak when DisposeLocalCopyOfClientHandle is not called

[adamsitnik]

This week I've spent few hours debugging a resource leak that was causing full Microbenchmarks run to fail on my Linux machine: dotnet/performance#2738.

To my surprise, despite the fact that AnonymousPipeServerStream was wrapped with a using block, the resource leak was still happening.

using AnonymousPipeServerStream inputFromBenchmark = new (PipeDirection.In, HandleInheritability.Inheritable);
using AnonymousPipeServerStream acknowledgments = new (PipeDirection.Out, HandleInheritability.Inheritable);

This is how I've learned about the existence of DisposeLocalCopyOfClientHandle method (dotnet/BenchmarkDotNet#2200).

I've taken a quick look to see if we could call it right after the client handle gets exposed, but it's impossible because the call to this method needs to happen after the creation of child process (so an opened handle can be inherited).
As of now, it's impossible. We could in theory call it after first successful read/write but tracking it would be error prone too.

My current best idea for now is to dispose the client handle (stored by the server) when the server instance gets disposed. Only in scenarios, where it was created with HandleInheritability.Inheritable. I can't see any valid scenarios, where users would like to use client pipe handle owned by the server after the server got disposed.

[ghost]

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

---

This week I've spent few hours debugging a resource leak that was causing full Microbenchmarks run to fail on my Linux machine: dotnet/performance#2738.

To my surprise, despite the fact that AnonymousPipeServerStream was wrapped with a using block, the resource leak was still happening.

using AnonymousPipeServerStream inputFromBenchmark = new (PipeDirection.In, HandleInheritability.Inheritable);
using AnonymousPipeServerStream acknowledgments = new (PipeDirection.Out, HandleInheritability.Inheritable);

This is how I've learned about the existence of DisposeLocalCopyOfClientHandle method (dotnet/BenchmarkDotNet#2200).

I've taken a quick look to see if we could call it right after the client handle gets exposed, but it's impossible because the call to this method needs to happen after the creation of child process (so an opened handle can be inherited).
As of now, it's impossible.

My current best idea for now is to dispose the client handle (stored by the server) when the server instance gets disposed. Only in scenarios, where it was created with HandleInheritability.Inheritable. I can't see any valid scenarios, where users would like to use client pipe handle owned by the server after the server got disposed.

Author:	adamsitnik
Assignees:	-
Labels:	area-System.IO
Milestone:	8.0.0

[stephentoub]

Won't this be a breaking change if anonymous pipes are being used intraprocess? Let's say one thread is writing to the server stream, finishes writing, and disposes of the stream. Another thread is reading from the client stream. Today won't the client simply get EOF and their read will return 0, whereas now they could get an ObjectDisposedException? Or if they do get an exception today in this or related scenarios, won't it be a different type of exception?

[stephentoub]

Oh, wait, you're still checking !_clientHandleExposed. Nevermind.

[stephentoub]

Oh, no, you're not, if it was created as inheritable. My original question still stands :)

[adamsitnik]

Won't this be a breaking change if anonymous pipes are being used intraprocess?

It will, but only in case someone created anonymous pipe as inheritable (the intent of this is to perform outproc communication), but then uses it for inproc. I know the fix is not perfect, but IMO it's a lesser evil (I expect that way more people are going to avoid resource leak).

[adamsitnik]

@stephentoub Could we include this breaking change in .NET 8 Preview 1 and eventually revert it if it causes a lot of trouble (I really doubt that)?

[stephentoub]

I know the fix is not perfect, but IMO it's a lesser evil (I expect that way more people are going to avoid resource leak).

Outside of a benchmarking suite that's specifically testing creating tons of these, how impactful is that really? We're talking about one SafeHandle being left for finalization if someone doesn't follow the right pattern. Are these apps you're concerned about creating massive numbers of anonymous server/client pipe streams?

[adamsitnik]

Outside of a benchmarking suite that's specifically testing creating tons of these

In this case it's not about benchmarking the pipes, BDN uses them internally for the host-benchmark process communication. So we create two pipes per every benchmark.

We're talking about one SafeHandle being left for finalization if someone doesn't follow the right pattern.

The problem is that it's never left for finalization:

runtime/src/libraries/System.IO.Pipes/src/System/IO/Pipes/AnonymousPipeServerStream.cs

Lines 86 to 89 in b3de41e

	public string GetClientHandleAsString()
	{
	_clientHandleExposed = true;
	GC.SuppressFinalize(_clientHandle);

So basically, if the user creates an inheritable handle, uses it in anyway, but does not call DisposeLocalCopyOfClientHandle the handle is not closed until the app exits.

[stephentoub]

The problem is that it's never left for finalization

If GetClientHandleAsString is used. I was talking about a scenario where it's not used.

Maybe a further mitigation is to split _clientHandleExposed. Have an additional _clientHandleExposedAsString, where GetClientHandleAsString sets both, and only do your proposed change if _clientHandleExposedAsString has been set.

[jozkee]

Is it possible to call GC.ReRegisterForFinalize(_clientHandle), rather than _clientHandle.Dispose() to avoid the described breaking change?

[adamsitnik]

Maybe a further mitigation is to split _clientHandleExposed. Have an additional _clientHandleExposedAsString, where GetClientHandleAsString sets both, and only do your proposed change if _clientHandleExposedAsString has been set.

Done, PTAL.

Is it possible to call GC.ReRegisterForFinalize(_clientHandle), rather than _clientHandle.Dispose() to avoid the described breaking change?

This would be also a breaking change and it would be non-deterministic:

AnonymousPipeServerStream server = new (PipeDirection.Out, HandleInheritability.Inheritable);
SafePipeHandle clientPipeHandle = new (nint.Parse(server.GetClientHandleAsString()), ownsHandle: true);
server.Dispose();
server = null;
// Even if we re-register client handle owned by the server for finalization,
// it's not rooted by any object and going to be closed when the finalizer is executed.
// It might "work for a while" and then stop when the GC cleanups the memory.
Use(clientPipeHandle);

[ghost]

Added needs-breaking-change-doc-created label because this PR has the breaking-change label.

When you commit this breaking change:

1. Create and link to this PR and the issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.
2. Ask a committer to mail the .NET Breaking Change Notification DL.

Tagging @dotnet/compat for awareness of the breaking change.

[adamsitnik]

Breaking change created: dotnet/docs#32713