-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Enable cancellation for anonymous pipes and non-async named pipes on Windows #72503
Conversation
…Windows Although ReadAsync, WriteAsync, and WaitForConnectionAsync on pipes all accept a CancellationToken, that token is only usable on Windows for canceling an in-flight operation when the pipe is using overlapped I/O. If the pipe was created for non-overlapped I/O, as is the case for anonymous pipes and can be the case for named pipes, the token stops being useful for anything other than an up-front cancellation check. This change fixes that by using CancelSynchronousIo to cancel the synchronous I/O performed as part of these async operations, which are implemented as async-over-sync (queueing to the thread pool a work item that performs the synchronous I/O). (The Unix implementation already supports cancellation in these situations.)
Tagging subscribers to this area: @dotnet/area-system-io Issue DetailsAlthough ReadAsync, WriteAsync, and WaitForConnectionAsync on pipes all accept a CancellationToken, that token is only usable on Windows for canceling an in-flight operation when the pipe is using overlapped I/O. If the pipe was created for non-overlapped I/O, as is the case for anonymous pipes and can be the case for named pipes, the token stops being useful for anything other than an up-front cancellation check. This change fixes that by using CancelSynchronousIo to cancel the synchronous I/O performed as part of these async operations, which are implemented as async-over-sync (queueing to the thread pool a work item that performs the synchronous I/O). (The Unix implementation already supports cancellation in these situations.) While this is implemented differently, I drew inspiration from poizan42's experiments in #31390 (comment). Thanks! And even though we're now actually registering for cancellation, performance is actually better even when cancellation is never requested:
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
using System.IO.Pipes;
[MemoryDiagnoser]
public class Program
{
static void Main(string[] args) => BenchmarkSwitcher.FromAssembly(typeof(Program).Assembly).Run(args);
private Stream _server;
private Stream _client;
private byte[] _buffer = new byte[1];
private CancellationTokenSource _cts = new CancellationTokenSource();
[Params(false, true)]
public bool Cancelable { get; set; }
[Params(false, true)]
public bool Named { get; set; }
[GlobalSetup]
public void Setup()
{
if (Named)
{
string name = Guid.NewGuid().ToString("N");
var server = new NamedPipeServerStream(name, PipeDirection.Out);
var client = new NamedPipeClientStream(".", name, PipeDirection.In);
Task.WaitAll(server.WaitForConnectionAsync(), client.ConnectAsync());
_server = server;
_client = client;
}
else
{
var server = new AnonymousPipeServerStream(PipeDirection.Out);
var client = new AnonymousPipeClientStream(PipeDirection.In, server.ClientSafePipeHandle);
_server = server;
_client = client;
}
}
[GlobalCleanup]
public void Cleanup()
{
_server.Dispose();
_client.Dispose();
}
[Benchmark(OperationsPerInvoke = 1000)]
public async Task ReadWriteAsync()
{
CancellationToken ct = Cancelable ? _cts.Token : default;
for (int i = 0; i < 1000; i++)
{
ValueTask<int> read = _client.ReadAsync(_buffer, ct);
await _server.WriteAsync(_buffer, ct);
await read;
}
}
}
|
Do we have this covered in the perf benchmarks? If not maybe we could reuse what you have there. Edit: never mind, it's not a mainline scenario. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a really smart way of solving this problem! Thank you for adding all the comments @stephentoub !
We need to figure out a way to reuse the logic for FileStream
, RandomAccess
and Pipes
. And ideally solve #28585 while we are there ;)
src/libraries/System.IO.Pipes/src/System/IO/Pipes/PipeStream.Windows.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Pipes/src/System/IO/Pipes/PipeStream.Windows.cs
Outdated
Show resolved
Hide resolved
Mostly. The only thing it lacks is passing in a cancellation token. |
@stephentoub I've got a question about how this works. What happens in this sequence of events:
I have fought with this kind of IO cancellation before and was then not able to make it work reliably. Curious to learn how you did that. |
|
Although ReadAsync, WriteAsync, and WaitForConnectionAsync on pipes all accept a CancellationToken, that token is only usable on Windows for canceling an in-flight operation when the pipe is using overlapped I/O. If the pipe was created for non-overlapped I/O, as is the case for anonymous pipes and can be the case for named pipes, the token stops being useful for anything other than an up-front cancellation check.
This change fixes that by using CancelSynchronousIo to cancel the synchronous I/O performed as part of these async operations, which are implemented as async-over-sync (queueing to the thread pool a work item that performs the synchronous I/O).
(The Unix implementation already supports cancellation in these situations.)
Fixes #31390
Closes #63536
While this is implemented differently, I drew inspiration from poizan42's experiments in #31390 (comment). Thanks!
And even though we're now actually registering for cancellation, performance is actually better even when cancellation is never requested: