Skip to content

[release/9.0-staging] Fix PipeStream leak on Windows when pipe is disposed with a pending operation #116188

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -139,10 +139,7 @@ internal override void DisposeCore(bool disposing)

if (disposing)
{
if (State != PipeState.Closed)
{
_internalTokenSource.Cancel();
}
_internalTokenSource.Cancel();
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,11 @@ internal override void TryToReuse(PipeValueTaskSource source)
{
if (Interlocked.CompareExchange(ref _reusableConnectionValueTaskSource, connectionSource, null) is not null)
{
source._preallocatedOverlapped.Dispose();
source.Dispose();
}
else if (State == PipeState.Closed)
{
Interlocked.Exchange(ref _reusableConnectionValueTaskSource, null)?.Dispose();
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -255,9 +255,25 @@ internal virtual void TryToReuse(PipeValueTaskSource source)
if (source is ReadWriteValueTaskSource readWriteSource)
{
ref ReadWriteValueTaskSource? field = ref readWriteSource._isWrite ? ref _reusableWriteValueTaskSource : ref _reusableReadValueTaskSource;

// Try to return the instance. If there's already something in the field, then there had been concurrent
// operations and someone else already returned their instance, so just dispose of this one (the "pool" has
// a size of 1). If there's not something there and we're successful in storing our instance, the 99% case is
// this is normal operation, there wasn't a concurrent operation, and we just successfully returned the rented
// instance. However, it could also be because the stream has been disposed, in which case the disposal process
// would have nulled out the field, and since this instance wasn't present when disposal happened, it won't have
// been disposed. Further, disposal won't happen again, so just leaving the instance there will result in its
// resources being leaked. Instead, _after_ returning the instance we then check whether the stream is now
// disposed, and if it is, we then try to re-rent the instance and dispose of it. It's fine if the disposal happens
// after we've stored the instance back and before we check the stream's state, as then this code will be racing
// with disposal and only one of the two will succeed in exchanging out the instance.
if (Interlocked.CompareExchange(ref field, readWriteSource, null) is not null)
{
source._preallocatedOverlapped.Dispose();
source.Dispose();
}
else if (State == PipeState.Closed)
{
Interlocked.Exchange(ref field, null)?.Dispose();
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,11 @@ public override Task FlushAsync(CancellationToken cancellationToken)

protected override void Dispose(bool disposing)
{
// Mark the pipe as closed before calling DisposeCore. That way, other threads that might
// be synchronizing on shared resources disposed of in DisposeCore will be guaranteed to
// see the closed state after that synchronization.
_state = PipeState.Closed;

try
{
// Nothing will be done differently based on whether we are
Expand All @@ -159,8 +164,6 @@ protected override void Dispose(bool disposing)
{
base.Dispose(disposing);
}

_state = PipeState.Closed;
}

// ********************** Public Properties *********************** //
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,11 @@ private void TryToReuse(OverlappedValueTaskSource source)

if (Interlocked.CompareExchange(ref _reusableOverlappedValueTaskSource, source, null) is not null)
{
source._preallocatedOverlapped.Dispose();
source.Dispose();
}
else if (IsClosed)
{
Interlocked.Exchange(ref _reusableOverlappedValueTaskSource, null)?.Dispose();
}
}

Expand Down
Loading