SerialPortDevice can hang when no data is written to COM port.

[SonicScholar]

Here's a simple unit test I wrote that simulates what's happening with the Nmea and SerialPort device classes.

I believe an issue similar to #88 is in the latest NmeaParser 2.2.2 package from Nuget. (FWIW, I didn't see where the 2.2.2 tag/release was here). Also, this may be the same issue reported in #111 but not sure.

If you read this article, it might lead to believe that it's an issue with the serial port class that we wouldn't have any control over (other than working around it in the SerialPortDevice class somehow...) My suspicion from one of the comments is that the SerialPort class' internal stream provides a ReadAsync method (which is used in this NmeaParser library)- The ReadAsync calls BeginRead and EndRead internally. And BeginRead apparently sets the read timeout to infinity before putting it back to the user defined ReadTimeout during the read .

I am on .NET 8 and using the System.IO.Ports 8.0.0 NuGet package

I am game to talk about how a fix/workaround could be implemented for this.

Here's the code:
it's also important to note that COM1 did not have the GPS connected to it. I am playing around with implementing a kind of "COM port crawler" that's built on top of the NmeaParser lib layer. (I get that there are probably more efficient ways to do this.... but still I think this issue could arise for others, especially if the device on the other end is not writing data to the stream)

       /*xUnit test*/ [Fact]
        public async Task Connect_Read_Disconnect_Hangs()
        {
            //arrange
            var portName = "COM1";
            var baudRate = 4800;
            var serialPort = new System.IO.Ports.SerialPort(portName, baudRate);

            var cancellationTokenSource = new CancellationTokenSource();
            var cancellationToken = cancellationTokenSource.Token;

            //act

            //simulate NmeaParser.SerialPortDevice.OpenAsync()
            serialPort.Open();
            var stream = serialPort.BaseStream;

            var readTask = Task.Run(async () =>
            {
                byte[] buffer = new byte[1024];
                int readCount = 0;
                while (!cancellationTokenSource.IsCancellationRequested)
                {
                   /* HANG HAPPENS HERE */
                    _ = await stream.ReadAsync(buffer, 0, buffer.Length).ConfigureAwait(false);
                    if (cancellationToken.IsCancellationRequested)
                        break;

                }
                await Task.Yield();
            });

            await Task.Delay(1000 * 10); // give the Task.Run task time to call ReadAsync
            //simulate NmeaParser.SerialPortDevice.CloseAsync()
            if (cancellationTokenSource != null)
            {
                cancellationTokenSource.Cancel();
                cancellationTokenSource = null;
            }

            if (readTask != null && !readTask.IsCompleted)
            {
                await readTask;
            }

            if (stream != null)
            {
                if(serialPort.IsOpen)
                    serialPort.Close();
            }
            stream = null;

        }

p.s. this library is awesome!!

[SonicScholar]

Also relevant reading. dotnet/runtime#28968

Seems like lots of people have trouble with the SerialStream class (internal to System.IO.SerialPort), and the [sparxeng] article link I posted earlier spells out a bunch of problems with the System.IO.SerialPort class in general.

In the dotnet runtime issue 28968 thread, there is some preference to using background threads with synchronous APIs which ostensibly don't ignore the read timeouts?

I see a couple of approaches for the NmeaParser library:

• use a different implementation of SerialPort (probably not preferable for compatibility considerations, unless another version of SerialPortDevice is added
• In the SerialPort class, wrap the SerialPort's BaseStream in another stream that calls synchronous APIs from a background thread with appropriate timeouts and cancellation token checks.
• Change the implementation of the NmeaDevice to use the synchronous APIs from a background thread. (eesh.... hate to spin up threads for I/O bound work when you don't have to.)

[dotMorten]

Yeah I'm finding that the CancellationToken and the ReadTimeout appear to be completely ignored in the read call. It causes issues like this one, as well as #111
I'm not quite up for wanting to replace the entire serial port reading code, over lobbying to get this fixed in the .NET SDK. The NmeaDevice already takes a custom stream you can pass in instead until such a fix happens.

[SonicScholar]

@dotMorten i ended up creating a copy of your implementation of serial nmea device that uses RJCP's version of SerialPort. I have found that implementation is much more robust than system.io.ports.serialport and it was basically a drop in replacement.

The .NET implementation seems to be pretty notorious and has been for a long time, I doubt they'll fix it anytime in the next decade, if at all.

In your shoes, I don't know that I would take on that dependency for this repo unless you've vetted the RJCP implementation, but the workaround is honestly not the worst.

[dotMorten]

@SonicScholar Cool. You shouldn't actually have to clone this library to do that. You can just create a subclass of NmeaDevice that returns the read stream in the abstract methods you must implement.

[SonicScholar]

I'd say feel free to close this ticket and offer the workaround to copy/paste your serial NMEA device class with one that can take a more robust implementation.

Would it be worth putting in some compiler warning about how there are issues with the .NET version of serial port, and the recommendation would be to extend NmeaDevice if adverse behavior occurs?

[dotMorten]

Another option would be to use the WinRT APIs instead. Just working on exposing that to net6.0-windows (it's already there in the UWP build)

[SonicScholar]

@SonicScholar Cool. You shouldn't actually have to clone this library to do that. You can just create a subclass of NmeaDevice that returns the read stream in the abstract methods you must implement.

Yeah that's exactly what I did, worked great!

[dotMorten]

I've added the WinRT based serial device that handles CloseAsync much better without getting hung.

[dotMorten]

See https://dotmorten.github.io/NmeaParser/api/netwin/NmeaParser.WinRTSerialDevice.html