ObjectDisposedException when closing SerialPort in .Net 2.0
Asked Answered
B

3

10

I have a C# windows forms application which communicates with a USB dongle via a COM port. I am using the SerialPort class in .Net 2.0 for communication, and the serial port object is open for the lifetime of the application. The application sends commands to the device and can also receive unsolicited data from the device.

My problem occurs when the form is closed - I get (randomly, unfortunately) an ObjectDisposedException when attempting to close the COM port. Here is the Windows stack trace:

System.ObjectDisposedException was unhandled


Message=Safe handle has been closed
  Source=System
  ObjectName=""
  StackTrace:
       at Microsoft.Win32.UnsafeNativeMethods.SetCommMask(SafeFileHandle hFile, Int32 dwEvtMask)
       at System.IO.Ports.SerialStream.Dispose(Boolean disposing)
       at System.IO.Ports.SerialStream.Finalize()
  InnerException: 

I have found posts from people with similar problems and have tried the workaround [here][1]

[1]: http://zachsaw.blogspot.com/2010/07/net-serialport-woes.html although that is for an IOException and did not stop the problem.

My Close() code is as follows:

        public void Close()
    {
        try
        {
            Console.WriteLine("******ComPort.Close - baseStream.Close*******");
            baseStream.Close();
        }
        catch (Exception ex)
        {
            Console.WriteLine("******ComPort.Close baseStream.Close raised exception: " + ex + "*******");
        }
        try
        {
            _onDataReceived = null;
            Console.WriteLine("******ComPort.Close - _serialPort.Close*******");
            _serialPort.Close();
        }
        catch (Exception ex)
        {
            Console.WriteLine("******ComPort.Close - _serialPort.Close raised exception: " + ex + "*******");
        }            
    }

My logging showed that execution never got beyond attempting to close the SerialPort's BaseStream (this is in the first try block), so I experimented with removing this line but the exception is still thrown periodically - the logging in the second try block appeared then the exception happened. Neither catch block catches the exception.

Any ideas?

UPDATE - adding full class:

    namespace My.Utilities
{
    public interface ISerialPortObserver
    {
        void SerialPortWriteException();
    }

    internal class ComPort : ISerialPort
    {
        private readonly ISerialPortObserver _observer;
        readonly SerialPort _serialPort;

        private DataReceivedDelegate _onDataReceived;
        public event DataReceivedDelegate OnDataReceived
        {
            add { lock (_dataReceivedLocker) { _onDataReceived += value; } }
            remove { lock (_dataReceivedLocker) { _onDataReceived -= value; } }            
        }

        private readonly object _dataReceivedLocker = new object();
        private readonly object _locker = new object();

        internal ComPort()
        {         
            _serialPort = new SerialPort { ReadTimeout = 10, WriteTimeout = 100, DtrEnable = true };
            _serialPort.DataReceived += DataReceived;
        }

        internal ComPort(ISerialPortObserver observer) : this()
        {
            _observer = observer;         
        }

        private void DataReceived(object sender, SerialDataReceivedEventArgs e)
        {
            DataReceivedDelegate temp = null;

            lock (_locker)
            {
                lock (_dataReceivedLocker)
                {
                    temp = _onDataReceived;
                }

                string dataReceived = string.Empty;
                var sp = (SerialPort) sender;

                try
                {
                    dataReceived = sp.ReadExisting();
                }
                catch (Exception ex)
                {
                    Logger.Log(TraceLevel.Error, "ComPort.DataReceived raised exception: " + ex);
                }

                if (null != temp && string.Empty != dataReceived)
                {
                    try
                    {
                        temp(dataReceived, TickProvider.GetTickCount());
                    }
                    catch (Exception ex)
                    {
                        Logger.Log(TraceLevel.Error, "ComPort.DataReceived raised exception calling handler: " + ex);
                    }
                }
            }
        }

        public string Port
        {
            set
            {
                try
                {
                    _serialPort.PortName = value;
                }
                catch (Exception ex)
                {
                    Logger.Log(TraceLevel.Error, "ComPort.Port raised exception: " + ex);
                }
            }
        }

        private System.IO.Stream comPortStream = null;
        public bool Open()
        {
            SetupSerialPortWithWorkaround();
            try
            {
                _serialPort.Open();
                comPortStream = _serialPort.BaseStream;
                return true;
            }
            catch (Exception ex)
            {
                Logger.Log(TraceLevel.Warning, "ComPort.Open raised exception: " + ex);
                return false;
            }
        }

        public bool IsOpen
        {
            get
            {
                SetupSerialPortWithWorkaround();
                try
                {
                    return _serialPort.IsOpen;
                }
                catch(Exception ex)
                {
                    Logger.Log(TraceLevel.Error, "ComPort.IsOpen raised exception: " + ex);
                }

                return false;
            }
        }

        internal virtual void SetupSerialPortWithWorkaround()
        {
            try
            {
                //http://zachsaw.blogspot.com/2010/07/net-serialport-woes.html
                // This class is meant to fix the problem in .Net that is causing the ObjectDisposedException.
                SerialPortFixer.Execute(_serialPort.PortName);
            }
            catch (Exception e)
            {
                Logger.Log(TraceLevel.Info, "Work around for .Net SerialPort object disposed exception failed with : " + e + " Will still attempt open port as normal");
            }
        }

        public void Close()
        {
            try
            {
                comPortStream.Close();
            }
            catch (Exception ex)
            {
                Logger.Log(TraceLevel.Error, "ComPortStream.Close raised exception: " + ex);
            }
            try
            {
                _onDataReceived = null;
                _serialPort.Close();
            }
            catch (Exception ex)
            {
                Logger.Log(TraceLevel.Error, "ComPort.Close raised exception: " + ex);
            }            
        }

        public void WriteData(string aData, DataReceivedDelegate handler)
        {
            try
            {
                OnDataReceived += handler;
                _serialPort.Write(aData + "\r\n");
            }
            catch (Exception ex)
            {
                Logger.Log(TraceLevel.Error, "ComPort.WriteData raised exception: " + ex);                

                if (null != _observer)
                {
                    _observer.SerialPortWriteException();
                }
            }
        }
    }    
}
Bordeaux answered 19/1, 2012 at 14:7 Comment(4)
You appear to be 'leaking' (not closing or disposing) instances of the SerialStream class (since SerialStream.Finalize is called in your stack trace), I would suggest that this is a problem, however to determine what relation this bears to your current problem more information is required.Rossman
Thanks for the response. Which information will help to pinpoint the problem?Bordeaux
The entire class that contains the Close method above will help.Rossman
This bug is pure cancer and MS still didn't fix the issue now in 2017Gelman
P
31

Note: The current findings have only been tested on .NET Framework 4.0 32-bit on Windows 7, please feel free to comment if it works on other versions.

Edit: TL;DR: Here's the crux of the workaround. See below for explanation. Don't forget to use SerialPortFixer when opening the SerialPort as well. ILog is from log4net.

static readonly ILog s_Log = LogManager.GetType("SerialWorkaroundLogger");

static void SafeDisconnect(SerialPort port, Stream internalSerialStream)
{
    GC.SuppressFinalize(port);
    GC.SuppressFinalize(internalSerialStream);

    ShutdownEventLoopHandler(internalSerialStream);

    try
    {
        s_Log.DebugFormat("Disposing internal serial stream");
        internalSerialStream.Close();
    }
    catch (Exception ex)
    {
        s_Log.DebugFormat(
            "Exception in serial stream shutdown of port {0}: {1}", port.PortName, ex);
    }

    try
    {
        s_Log.DebugFormat("Disposing serial port");
        port.Close();
    }
    catch (Exception ex)
    {
        s_Log.DebugFormat("Exception in port {0} shutdown: {1}", port.PortName, ex);
    }
}

static void ShutdownEventLoopHandler(Stream internalSerialStream)
{
    try
    {
        s_Log.DebugFormat("Working around .NET SerialPort class Dispose bug");

        FieldInfo eventRunnerField = internalSerialStream.GetType()
            .GetField("eventRunner", BindingFlags.NonPublic | BindingFlags.Instance);

        if (eventRunnerField == null)
        {
            s_Log.WarnFormat(
                "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)
            {
                s_Log.WarnFormat(
                    "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
            {
                s_Log.DebugFormat(
                    "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));

                s_Log.DebugFormat("Wait completed. Now it is safe to continue disposal.");
            }
        }
    }
    catch (Exception ex)
    {
        s_Log.ErrorFormat(
            "SerialPort workaround failure. Application may crash after "
            + "disposing SerialPort unless .NET 1.1 unhandled exception "
            + "policy is enabled from the application's config file: {0}",
            ex);
    }
}

I've wrestled with this for a couple of days in a recent project.

There are many different bugs (I've seen so far) with the .NET SerialPort class that lead to all of the headaches on the web.

  1. The missing DCB struct flag here: http://zachsaw.blogspot.com/2010/07/net-serialport-woes.html This one is fixed by the SerialPortFixer class, credits go to the author for that one.

  2. When a USB serial device is removed, when closing the SerialPortStream, the eventLoopRunner is asked to stop and SerialPort.IsOpen returns false. Upon disposal this property is checked and closing the internal serial stream is skipped, thus keeping the original handle open indefinitely (until the finalizer runs which leads to the next problem).

    The solution for this one is to manually close the internal serial stream. We can get its reference by SerialPort.BaseStream before the exception has happened or by reflection and getting the "internalSerialStream" field.

  3. When a USB serial device is removed, closing the internal serial stream throws an exception and closes the internal handle without waiting for its eventLoopRunner thread to finish, causing an uncatchable ObjectDisposedException from the background event loop runner thread later on when the stream's finalizer runs (which oddly avoids throwing the exception but still fails to wait for the eventLoopRunner).

    Symptom of that here: https://connect.microsoft.com/VisualStudio/feedback/details/140018/serialport-crashes-after-disconnect-of-usb-com-port

    The solution is to manually ask the event loop runner to stop (via reflection) and waiting for it to finish before closing the internal serial stream.

  4. Since Dispose throws exceptions, the finalizer is not suppressed. This is easily solvable:

    GC.SuppressFinalize(port); GC.SuppressFinalize(port.BaseStream);

Here's a class that wraps a serial port and fixes all these issues: http://pastebin.com/KmKEVzR8

With this workaround class, reverting to .NET 1.1 unhandled exception behavior is unnecessary and it works with excellent stability.

This is my first contribution, so please excuse me if I'm not doing it right. I hope it helps someone.

Panchromatic answered 29/1, 2012 at 21:58 Comment(7)
It looks like this has solved the problem. The only changed I made was to the last while statement: while (!eventLoopEndedWaitHandle.WaitOne(2000, false));. .Net 2.0 does not have the method without the boolean parameter.Bordeaux
Update: on .NET 4.5 apparently the crash seems to have been fixed by MS, but still the handle remains opened too long (probably waiting for finalizer), so this solution is still valid and still works (tested on Win7 and Win8 64-bit .NET 4.5).Panchromatic
I still have the crash on 4.5 but the fix from the answer works perfectly. A shame I can only give one upvote.Ellswerth
@MarkusDeibel I keep trying to break his code (the person who wrote the fix) but I just can't... For some reason it keeps on working! I second your notion that this person deserves more than one upvote.Jehiah
All your links except pastebin are deadGelman
@Snoopy I broke it but didn't mean to... https://mcmap.net/q/1160374/-serialport-39-s-finalize-still-called-after-gc-suppressfinalize/5771029Gelman
@RaifAtef I used this code in my project, but my program is still crashing by ObjectDisposedException. any idea how it can be resolved ?Seow
P
12

Yes, there's a flaw in the SerialPort class that makes this kind of crash possible. SerialPort starts up a thread when you call Open(). That thread watches for events on the port, that's how you get the DataReceived event for example. When you call the BaseStream.Close() or Close() or Dispose() method (they all do the same thing) then SerialPort only asks the thread to exit but doesn't wait for it to exit.

This causes all kinds of problems. One documented one, you're not supposed to Open() a port right after closing it. But the mishap here is when your program exits or garbage collects right after the Close() call. That runs the finalizer and it tries to close the handle as well. It is still open because the worker thread is still using it. A threading race is now possible, this isn't interlocked properly. The kaboom happens when the worker managed to close the handle and exit just before the finalizer thread tries to do the same. The exception is uncatchable because it happens in the finalizer thread, the CLR aborts the program.

Every version of .NET since 2.0 had small changes in the classes to work around SerialPort problems. By far the best thing to do if you're still on .NET 2.0 is to not actually call Close(). It happens automatically anyway, the finalizer takes care of it. Even if that doesn't happen for some reason (hard crash or program abort) then Windows ensures the port is closed.

Ploss answered 19/1, 2012 at 17:26 Comment(2)
Thanks Hans, that's very helpful. I've added the full class incase it helps anyone pinpoint the problem, but I suspect I'm just falling foul of this .net bug. I'll look into not calling Close() and see how I get on.Bordeaux
@Hans: Is there any good way to close a serial port in the event that it's supposed to be made available for use by another program, or handle the possibility of a USB serial adapter being unplugged unexpectedly? The approach I've ended up using is to write a program whose purpose is to open up a serial port and relay data to/from the main program. If it's necessary to close the port or if it dies, that program can die without affecting the main program. That seems sorta icky, though.Waistcloth
P
7

I know this is a quite old, yet current question. I had this problem recently and after looking for a solution, it looks like this issue is finally fixed with .NET Framework 4.7, according to release notes. https://github.com/Microsoft/dotnet/blob/master/releases/net47/dotnet47-changes.md

Fixed an issue in SerialPort where unplugging the device during execution could cause a memory leak in the SerialStream class. [288363]

Pennington answered 27/9, 2017 at 7:32 Comment(2)
There's also something in 4.7.1: SerialPort streams no longer terminate the process when exceptions occur on the background thread. This can happen when removing a USB serial port while in use. This new behavior is controlled by the Switch.System.IO.Ports.DoNotCatchSerialStreamThreadExceptions AppContext switch.Polymorphonuclear
I know this is from 2017 but can someone please confirm if this fix has been applied in .NET 5?Purpura

© 2022 - 2024 — McMap. All rights reserved.