search

dotnet / Nerdbank.Streams

Specialized .NET Streams and pipes for full duplex in-proc communication, web sockets, and multiplexing
https://dotnet.github.io/Nerdbank.Streams/
MIT License
636 stars 61 forks source link

ServiceBroker.DisposeAsync can lead to a deadlock #504

Closed adrianvmsft closed 2 years ago

adrianvmsft commented 2 years ago

This issue was encountered by internal automation - when running 2 tests using ServiceBroker in a row causes a deadlock when each of the tests calls ServiceBroker.DisposeAsync at the end.

1ff00bffb68 <0> Nerdbank.Streams.MultiplexingStream+V1Formatter+<DisposeAsync>d__9 @ 7ff9840086c0
..1ff00bffc78 <1> Nerdbank.Streams.MultiplexingStream+<DisposeAsync>d__55 @ 7ff984006d50
..1ff00bffd88 <0> Microsoft.ServiceHub.Framework.RemoteServiceBroker+<DisposeAsync>d__34 @ 7ff9840018a0
..1ff008ae440 <2> vside.Project.QueryTests.QueryBasicTests+<BasicQueryAsync>d__4 @ 7ff983915020
..1ff008ae400 <0> vside.Project.QueryTests.QueryBasicTests+<BasicQueryTest_NetCore_Async>d__2 @ 7ff984017f20

1ff00b0f390 <0> Nerdbank.Streams.MultiplexingStream+<ReadToFillAsync>d__57 @ 7ff983abdaf0
..1ff00b0f4f0 <1> Nerdbank.Streams.MultiplexingStream+V1Formatter+<ReadFrameAsync>d__13 @ 7ff983abca00
..1ff00ac4c68 <0> Nerdbank.Streams.MultiplexingStream+<ReadStreamAsync>d__60 @ 7ff983abae40

The first async task waits on the work reported by the 2nd async task to be completed first. That never happens, despite the fact the cancellation token that is supposed to cancel is triggered.

The root cause is that the CancellationToken in Stream.ReadAsync is ignored: https://github.com/dotnet/runtime/issues/24093 https://github.com/dotnet/runtime/issues/23638 https://github.com/dotnet/runtime/issues/31390

AArnott commented 2 years ago

There is no general and safe way to force cancellation of I/O that underlies a Stream-derived class as a consumer of Stream. They may or may not honor the CancellationToken. Disposing the Stream can sometimes force a Read call to return 0, but it also risks thread-safety issues to do so because Stream isn't expected to be thread-safe.

What we theoretically could do here is do a runtime type check and if the stream is a named pipe stream, we could wrap it in our own stream that implements the I/O ourselves in a cancelable way by basically copying code from https://github.com/dotnet/runtime/pull/72503 and https://github.com/dotnet/runtime/pull/72612. This seems extremely heavy though. And since the repro for this is only in a VS test, which has already worked around the problem, I'm going to close this as Won't Fix. It will work properly on .NET 7 where the named pipe class now has cancelable I/O.