I'm trying really hard to use a SerialPort in C# and found out the API for serial ports in .NET has bugs in it since 2010. I have combined the patches made in this blog post and that answer.
However, I'm still getting an unhandled exception in the GC thread that crashes my program when I try closing my Serial port. Amusingly, the top of the Stack trace is exactly in SerialStream's Finalize() method even if the SerialStream object has already been GC.SuppressFinalize() by the code of the answer I linked.
Here's the exception:
System.ObjectDisposedException: Safe handle has been closed
at System.Runtime.InteropServices.SafeHandle.DangerousAddRef(Boolean& success)
at System.StubHelpers.StubHelpers.SafeHandleAddRef(SafeHandle pHandle, Boolean& success)
at Microsoft.Win32.UnsafeNativeMethods.SetCommMask(SafeFileHandle hFile, Int32 dwEvtMask)
at System.IO.Ports.SerialStream.Dispose(Boolean disposing)
at System.IO.Ports.SerialStream.Finalize()
Here's the logs for the serial port debugging:
2017-20-07 14:29:22, Debug, Working around .NET SerialPort class Dispose bug
2017-20-07 14:29:22, Debug, Waiting for the SerialPort internal EventLoopRunner thread to finish...
2017-20-07 14:29:22, Debug, Wait completed. Now it is safe to continue disposal.
2017-20-07 14:29:22, Debug, Disposing internal serial stream
2017-20-07 14:29:22, Debug, Disposing serial port
Here's my code, a slightly modified version of the 2 links I posted:
public static class SerialPortFixerExt
{
public static void SafeOpen(this SerialPort port)
{
using(new SerialPortFixer(port.PortName))
{
}
port.Open();
}
public static void SafeClose(this SerialPort port, Logger logger)
{
try
{
Stream internalSerialStream = (Stream)port.GetType()
.GetField("internalSerialStream", BindingFlags.Instance | BindingFlags.NonPublic).GetValue(port);
GC.SuppressFinalize(port);
GC.SuppressFinalize(internalSerialStream);
ShutdownEventLoopHandler(internalSerialStream, logger);
try
{
logger.Debug("serial-port-debug", "Disposing internal serial stream");
internalSerialStream.Close();
}
catch(Exception ex)
{
logger.Debug("serial-port-debug", String.Format("Exception in serial stream shutdown of port {0}: ", port.PortName), ex);
}
try
{
logger.Debug("serial-port-debug", "Disposing serial port");
port.Close();
}
catch(Exception ex)
{
logger.Debug("serial-port-debug", String.Format("Exception in port {0} shutdown: ", port.PortName), ex);
}
}
catch(Exception ex)
{
logger.Debug("serial-port-debug", String.Format("Exception in port {0} shutdown: ", port.PortName), ex);
}
}
static void ShutdownEventLoopHandler(Stream internalSerialStream, Logger logger)
{
try
{
logger.Debug("serial-port-debug", "Working around .NET SerialPort class Dispose bug");
FieldInfo eventRunnerField = internalSerialStream.GetType()
.GetField("eventRunner", BindingFlags.NonPublic | BindingFlags.Instance);
if(eventRunnerField == null)
{
logger.Warning("serial-port-debug",
"Unable to find EventLoopRunner field. "
+ "SerialPort workaround failure. Application may crash after "
+ "disposing SerialPort unless .NET 1.1 unhandled exception "
+ "policy is enabled from the application's config file.");
}
else
{
object eventRunner = eventRunnerField.GetValue(internalSerialStream);
Type eventRunnerType = eventRunner.GetType();
FieldInfo endEventLoopFieldInfo = eventRunnerType.GetField(
"endEventLoop", BindingFlags.Instance | BindingFlags.NonPublic);
FieldInfo eventLoopEndedSignalFieldInfo = eventRunnerType.GetField(
"eventLoopEndedSignal", BindingFlags.Instance | BindingFlags.NonPublic);
FieldInfo waitCommEventWaitHandleFieldInfo = eventRunnerType.GetField(
"waitCommEventWaitHandle", BindingFlags.Instance | BindingFlags.NonPublic);
if(endEventLoopFieldInfo == null
|| eventLoopEndedSignalFieldInfo == null
|| waitCommEventWaitHandleFieldInfo == null)
{
logger.Warning("serial-port-debug",
"Unable to find the EventLoopRunner internal wait handle or loop signal fields. "
+ "SerialPort workaround failure. Application may crash after "
+ "disposing SerialPort unless .NET 1.1 unhandled exception "
+ "policy is enabled from the application's config file.");
}
else
{
logger.Debug("serial-port-debug",
"Waiting for the SerialPort internal EventLoopRunner thread to finish...");
var eventLoopEndedWaitHandle =
(WaitHandle)eventLoopEndedSignalFieldInfo.GetValue(eventRunner);
var waitCommEventWaitHandle =
(ManualResetEvent)waitCommEventWaitHandleFieldInfo.GetValue(eventRunner);
endEventLoopFieldInfo.SetValue(eventRunner, true);
// Sometimes the event loop handler resets the wait handle
// before exiting the loop and hangs (in case of USB disconnect)
// In case it takes too long, brute-force it out of its wait by
// setting the handle again.
do
{
waitCommEventWaitHandle.Set();
} while(!eventLoopEndedWaitHandle.WaitOne(2000));
logger.Debug("serial-port-debug", "Wait completed. Now it is safe to continue disposal.");
}
}
}
catch(Exception ex)
{
logger.Warning("serial-port-debug",
"SerialPort workaround failure. Application may crash after "
+ "disposing SerialPort unless .NET 1.1 unhandled exception "
+ "policy is enabled from the application's config file: " +
ex);
}
}
}
public class SerialPortFixer : IDisposable
{
#region IDisposable Members
public void Dispose()
{
if(m_Handle != null)
{
m_Handle.Close();
m_Handle = null;
}
}
#endregion
#region Implementation
private const int DcbFlagAbortOnError = 14;
private const int CommStateRetries = 10;
private SafeFileHandle m_Handle;
internal SerialPortFixer(string portName)
{
const int dwFlagsAndAttributes = 0x40000000;
const int dwAccess = unchecked((int)0xC0000000);
if((portName == null) || !portName.StartsWith("COM", StringComparison.OrdinalIgnoreCase))
{
throw new ArgumentException("Invalid Serial Port", "portName");
}
SafeFileHandle hFile = CreateFile(@"\\.\" + portName, dwAccess, 0, IntPtr.Zero, 3, dwFlagsAndAttributes,
IntPtr.Zero);
if(hFile.IsInvalid)
{
WinIoError();
}
try
{
int fileType = GetFileType(hFile);
if((fileType != 2) && (fileType != 0))
{
throw new ArgumentException("Invalid Serial Port", "portName");
}
m_Handle = hFile;
InitializeDcb();
}
catch
{
hFile.Close();
m_Handle = null;
throw;
}
}
[DllImport("kernel32.dll", CharSet = CharSet.Auto, SetLastError = true)]
private static extern int FormatMessage(int dwFlags, HandleRef lpSource, int dwMessageId, int dwLanguageId,
StringBuilder lpBuffer, int nSize, IntPtr arguments);
[DllImport("kernel32.dll", CharSet = CharSet.Auto, SetLastError = true)]
private static extern bool GetCommState(SafeFileHandle hFile, ref Dcb lpDcb);
[DllImport("kernel32.dll", CharSet = CharSet.Auto, SetLastError = true)]
private static extern bool SetCommState(SafeFileHandle hFile, ref Dcb lpDcb);
[DllImport("kernel32.dll", CharSet = CharSet.Auto, SetLastError = true)]
private static extern bool ClearCommError(SafeFileHandle hFile, ref int lpErrors, ref Comstat lpStat);
[DllImport("kernel32.dll", CharSet = CharSet.Auto, SetLastError = true)]
private static extern SafeFileHandle CreateFile(string lpFileName, int dwDesiredAccess, int dwShareMode,
IntPtr securityAttrs, int dwCreationDisposition,
int dwFlagsAndAttributes, IntPtr hTemplateFile);
[DllImport("kernel32.dll", SetLastError = true)]
private static extern int GetFileType(SafeFileHandle hFile);
private void InitializeDcb()
{
Dcb dcb = new Dcb();
GetCommStateNative(ref dcb);
dcb.Flags &= ~(1u << DcbFlagAbortOnError);
SetCommStateNative(ref dcb);
}
private static string GetMessage(int errorCode)
{
StringBuilder lpBuffer = new StringBuilder(0x200);
if(
FormatMessage(0x3200, new HandleRef(null, IntPtr.Zero), errorCode, 0, lpBuffer, lpBuffer.Capacity,
IntPtr.Zero) != 0)
{
return lpBuffer.ToString();
}
return "Unknown Error";
}
private static int MakeHrFromErrorCode(int errorCode)
{
return (int)(0x80070000 | (uint)errorCode);
}
private static void WinIoError()
{
int errorCode = Marshal.GetLastWin32Error();
throw new IOException(GetMessage(errorCode), MakeHrFromErrorCode(errorCode));
}
private void GetCommStateNative(ref Dcb lpDcb)
{
int commErrors = 0;
Comstat comStat = new Comstat();
for(int i = 0; i < CommStateRetries; i++)
{
if(!ClearCommError(m_Handle, ref commErrors, ref comStat))
{
WinIoError();
}
if(GetCommState(m_Handle, ref lpDcb))
{
break;
}
if(i == CommStateRetries - 1)
{
WinIoError();
}
}
}
private void SetCommStateNative(ref Dcb lpDcb)
{
int commErrors = 0;
Comstat comStat = new Comstat();
for(int i = 0; i < CommStateRetries; i++)
{
if(!ClearCommError(m_Handle, ref commErrors, ref comStat))
{
WinIoError();
}
if(SetCommState(m_Handle, ref lpDcb))
{
break;
}
if(i == CommStateRetries - 1)
{
WinIoError();
}
}
}
#region Nested type: COMSTAT
[StructLayout(LayoutKind.Sequential)]
private struct Comstat
{
public readonly uint Flags;
public readonly uint cbInQue;
public readonly uint cbOutQue;
}
#endregion
#region Nested type: DCB
[StructLayout(LayoutKind.Sequential)]
private struct Dcb
{
public readonly uint DCBlength;
public readonly uint BaudRate;
public uint Flags;
public readonly ushort wReserved;
public readonly ushort XonLim;
public readonly ushort XoffLim;
public readonly byte ByteSize;
public readonly byte Parity;
public readonly byte StopBits;
public readonly byte XonChar;
public readonly byte XoffChar;
public readonly byte ErrorChar;
public readonly byte EofChar;
public readonly byte EvtChar;
public readonly ushort wReserved1;
}
#endregion
#endregion
}
Usage of that code:
public void Connect(ConnectionParameters parameters)
{
if(m_params != null && m_params.Equals(parameters))
return; //already connected here
Close();
m_params = parameters;
try
{
m_comConnection = new SerialPort();
m_comConnection.PortName = m_params.Address;
m_comConnection.BaudRate = m_params.BaudRate;
m_comConnection.ReadTimeout = 6000;
m_comConnection.WriteTimeout = 6000;
m_comConnection.Parity = m_params.Parity;
m_comConnection.StopBits = m_params.StopBits;
m_comConnection.DataBits = m_params.DataBits;
m_comConnection.Handshake = m_params.Handshake;
m_comConnection.SafeOpen();
}
catch(Exception)
{
m_params = null;
m_comConnection = null;
throw;
}
}
public void Close()
{
if(m_params == null)
return;
m_comConnection.SafeClose(new Logger());
m_comConnection = null;
m_params = null;
}
So what am I doing wrong ? My logs couldn't look like that if the GC.SuppressFinalize() wasn't executed. What could make a Finalize() still be executed even after a GC.SuppressFinalize()?
Edit
I'm adding the code I use with my SerialPort between the opening and the closing of the port.
public byte[] ExchangeFrame(byte[] prefix, byte[] suffix, byte[] message, int length = -1)
{
if(m_params == null)
throw new NotConnectedException();
if(length == -1)
length = message.Length;
Write(prefix, 0, prefix.Length);
Write(message, 0, length);
Write(suffix, 0, suffix.Length);
byte[] response = new byte[length];
byte[] prefixBuffer = new byte[prefix.Length];
byte[] suffixBuffer = new byte[suffix.Length];
ReadTillFull(prefixBuffer);
while(!ByteArrayEquals(prefixBuffer, prefix))
{
for(int i = 0; i < prefixBuffer.Length - 1; i++)
prefixBuffer[i] = prefixBuffer[i + 1]; //shift them all back
if(Read(prefixBuffer, prefixBuffer.Length - 1, 1) == 0) //read one more
throw new NotConnectedException("Received no data when reading prefix.");
}
ReadTillFull(response);
ReadTillFull(suffixBuffer);
if(!ByteArrayEquals(suffixBuffer, suffix))
throw new InvalidDataException("Frame received matches prefix but does not match suffix. Response: " + BitConverter.ToString(prefixBuffer) + BitConverter.ToString(response) + BitConverter.ToString(suffixBuffer));
return response;
}
private void ReadTillFull(byte[] buffer, int start = 0, int length = -1)
{
if(length == -1)
length = buffer.Length;
int remaining = length;
while(remaining > 0)
{
int read = Read(buffer, start + length - remaining, remaining);
if(read == 0)
throw new NotConnectedException("Received no data when reading suffix.");
remaining -= read;
}
}
//this method looks dumb but actually, the real code has a switch statement to check if should connect by TCP or Serial
private int Read(byte[] buffer, int start, int length)
{
return m_comConnection.Read(buffer, start, length);
}
private void Write(byte[] buffer, int start, int length)
{
m_comConnection.Write(buffer, start, length);
}
[DllImport("msvcrt.dll", CallingConvention = CallingConvention.Cdecl)]
static extern int memcmp(byte[] b1, byte[] b2, long count);
static bool ByteArrayEquals(byte[] b1, byte[] b2)
{
// Validate buffers are the same length.
// This also ensures that the count does not exceed the length of either buffer.
return b1.Length == b2.Length && memcmp(b1, b2, b1.Length) == 0;
}
Currently, I'm only looking for a healty communication by opening, writing, reading and closing the port. I will have to handle the case where the port is physically unplugged though.(Eventually)
Note that I'm using the ExchangeFrame method multiple times in a row (in a loop). After more testing, I found out using it once doesn't create any error. (Sending a single frame) I guess it's a problem with my code then but I am unsure of what I am doing that could make a SerialPort break when executed multiple times.
On a side note, I have another issue where I'm receiving lots of 0s in the frames I'm reading from times to times. (Didn't notice a pattern) I don't know why but I know its a new bug that appeared with this code. Not part of the question but could be related.
Edit 2
The issue was caused by myself interrupting the SerialPort's with Thread.Interrupt(). I was correctly handling the ThreadInterruptedException but it was breaking SerialPort internally. Here's the code that caused the issue:
public void Stop()
{
m_run = false;
if(m_acquisitor != null)
{
m_acquisitor.Interrupt();
m_acquisitor.Join();
}
m_acquisitor = null;
}
private void Run()
{
while(m_run)
{
long time = DateTime.Now.Ticks, done = -1;
double[] data;
try
{
lock(Connection)
{
if(!m_run)
break; //if closed while waiting for Connection
Connection.Connect(Device.ConnectionParams);
data = Acquire();
}
done = DateTime.Now.Ticks;
Ping = (done - time) / TimeSpan.TicksPerMillisecond;
Samples++;
if(SampleReceived != null)
SampleReceived(this, data);
}
catch(ThreadInterruptedException)
{
continue; //checks if m_run is true, if not quits
}
try
{
if(done == -1)
Thread.Sleep(RETRY_DELAY); //retry delay
else
Thread.Sleep((int)Math.Max(SamplingRate * 1000 - (done - time) / TimeSpan.TicksPerMillisecond, 0));
}
catch(ThreadInterruptedException) {} //continue
}
lock(Connection)
Connection.Close(); //serialport's wrapper
}
Giant thanks to Snoopy who helped me solve the issue in chat.
