dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.2k stars 4.72k forks source link

SerialStream does not support cancellation on Windows #30850

Open krwq opened 5 years ago

krwq commented 5 years ago

Current Windows implementation does not respect cancellation but Unix implementation does.

We should consider doing similar effort for Windows.

cc: @dquist Created per: https://github.com/dotnet/corefx/issues/25154#issuecomment-531217979

Marking as 5.0 to consider. Note that netfx also does not support cancellation there.

dquist commented 5 years ago

most of the time the Read/WriteTimeout should be used for cancellation since this covers most typical scenarios and I believe they work correctly on both platforms

How would that work since that's a SerialPort property and SerialPort doesn't have any async methods? The only way to do an async read is through the BaseStream instance.

If I set a timeout value and call SerialPort.Read, that's a synchronous call and will block my UI thread, forcing me to wrap it in a Task.Run(). Is that correct?

dquist commented 5 years ago

Btw here's an extension method I wrote to wrap the ReadAsync call in a Task.Run. It asynchronously waits on the cancellation token which allows me to use it for any stream instance and guarantee that the cancellation will be respected.

public static async ValueTask<int> ReadAsyncCancellable(this Stream stream, Memory<byte> buffer, CancellationToken cancellationToken = default)
{
    int bytesRead = 0;
    await Task.Run(() =>
    {
        while (bytesRead < buffer.Length)
        {
            if (cancellationToken.IsCancellationRequested)
            {
                throw new OperationCanceledException(cancellationToken);
            }
            var task = stream.ReadAsync(buffer.Slice(bytesRead)).AsTask();
            task.Wait(cancellationToken);
            bytesRead += task.Result;
        }
    });
    return bytesRead;
}
krwq commented 5 years ago

How would that work since that's a SerialPort property and SerialPort doesn't have any async methods? The only way to do an async read is through the BaseStream instance.

https://github.com/dotnet/corefx/blob/master/src/System.IO.Ports/src/System/IO/Ports/SerialStream.Windows.cs#L659

@dquist I'm not sure what Task.Run gives you in that method - I'd expect cancellation to behave mostly the same with or without Task.Run

JeremyKuhne commented 4 years ago

Triage: Reasonable request, needs more investigation.

ygoe commented 1 year ago

Oh, "Future" milestone. You might as well remove that API altogether until then. If the CancellationToken is effectively ignored and ReadAsync cannot be cancelled, there is no async .NET API for the UART hardware. BTW, the "A" stands for "asynchronous". An API for that should only be async and nothing else. So if reading data that is never sent will block your application forever, could you call that usable? The SerialPort data received event has already been found unreliable, don't use that.

I see the workaround with "not waiting for a task any longer" but what does it do with the ongoing Read method call? That'll still be reading into nowhere. After all, if nobody tells it to stop, how should it know about that? I see a memory leak and some data loss at later read calls here, if not greater trouble. Do I have to close the entire serial port every time I decide I want to stop waiting for incoming data for now?

Maybe USB serial adapters should not be used with .NET. Instead, an Ethernet serial adapter might be more suitable for this development platform. TCP streams are a lot more robust. I really didn't want to spend my time building a new Win32/Linux serial port wrapper.

I'm trying to implement a very simple request/response communication protocol for TCP and serial. TCP was easy and works. Serial can't be done for now, given the usual reliability and responsiveness requirements of a public API.

ygoe commented 1 year ago

Considering the untouched open issues for this project, the misleading wishful documentation and the lack of any progress or even life signs, I now officially consider this library unmaintained. I guess that's what happens when a commercial product with a broken design is given to a community of volunteers.

There is a workaround explained here: https://stackoverflow.com/a/54610437 It seems to do the job for now, but this hack (and others) need to be copied to each application now. I'm only waiting for the day when it all falls apart and we can scratch serial port support from .NET.

danmoseley commented 1 year ago

@ygoe Are you interested in offering a change?

ygoe commented 1 year ago

I'd be happy to help. I'll let you know when I've learnt how to use the Win32 serial communication API and corresponding APIs on Linux and other platforms (excl. Mac). Actually, I wanted to use .NET to create other things before that. Needing to create .NET first might make me use another environment altogether.

Usually it's also wiser to let more experienced project members deal with this, but I don't see any this time.

Resources I found, in case somebody wants to start this:

ThaDaVos commented 6 months ago

Why hasn't this be fixed 5 years later... I just ran into the issue of the ReadAsync not following the CancellationToken...