views:

2149

answers:

2

Good afternoon everybody!

I have this threaded SerialPort wrapper that reads in a line from the serial port. Here is my thread's code.

protected void ReadData()
{
     SerialPort serialPort = null;
     try
     {
         serialPort = SetupSerialPort(_serialPortSettings);
         serialPort.Open();

         string data;
         while (serialPort.IsOpen)
         {
             try
             {

                 data = serialPort.ReadLine();
                 if (data.Length > 0)
                     ReceivedData(serialPort, new ReceivedDataEventArgs(data));

             }
             catch (TimeoutException)
             {
                 //  No action
             }
         }
     }
     catch (ThreadAbortException)
     {
         if (serialPort != null)
             serialPort.Close();
     }
}

when I call myThread.Abort(); I get an exception (with no line or reference to code) "Safe handle has been closed". Can anyone spot what I am doing wrong? Thanks.

By the way, I have a Start() and a Stop() that creates the thread and aborts the thread, respectfully.

+5  A: 

I would suspect that it is because you are using Thread.Abort to end the thread - which is generally frowned upon. The thread behavior when you abort it is not predictable. Because of that, since the serial port is a wrapper over native code, there are native resources - represented by a SafeHandle in .NET - which get disposed of unexpectedly and so you get the Exception.

You can think about what happens with your thread like this:

  • you start your thread
  • you open the serial port (which allocates native resources and uses SafeHandle(s) to hold on to those resources)
  • you start reading from the serial port
  • then at some point (unexpected to your thread) you call Thread.Abort on it
  • most likely the code in your thread is at that point trying to access the serial port (to read data)
  • the thread gets killed and the serial port handle is destroyed implicitly
  • you get an exception thrown from the code inside the ReadLine() function of the serial port because the handle it had is no longer valid

You really should use a different method for aborting the thread so that you get a proper chance to close and dispose of the serial port.

A proper way to close your thread could be implemented using a ManualResetEvent like this:

protected ManualResetEvent threadStop = new New ManualResetEvent(False);

protected void ReadData()
{
     SerialPort serialPort = null;
     try
     {
         serialPort = SetupSerialPort(_serialPortSettings);
         serialPort.Open();

         string data;
         while (serialPort.IsOpen)
         {
             try
             {

                 data = serialPort.ReadLine();
                 if (data.Length > 0)
                     ReceivedData(serialPort, new ReceivedDataEventArgs(data));

             }
             catch (TimeoutException)
             {
                 //  No action
             }

             // WaitOne(0) tests whether the event was set and returns TRUE
             //   if it was set and FALSE otherwise.
             // The 0 tells the manual reset event to only check if it was set
             //   and return immediately, otherwise if the number is greater than
             //   0 it will wait for that many milliseconds for the event to be set
             //   and only then return - effectively blocking your thread for that
             //   period of time
             if(threadStop.WaitOne(0))
                 break;
         }
     }
     catch (Exception exc)
     {
         // you can do something here in case of an exception
         // but a ThreadAbortedException should't be thrown any more if you
         // stop using Thread.Abort and rely on the ManualResetEvent instead
     }finally
     {
         if (serialPort != null)
             serialPort.Close();
     }
}

protected void Stop()
{
    // Set the manual reset event to a "signaled" state --> will cause the
    //   WaitOne function to return TRUE
    threadStop.Set();
}

Of course, when using the events method to stop the thread you have to be careful to include an event state check in all your long running loops or tasks. If you don't your thread may appear not to respond to your setting the event - until it gets out of the long-running loop, or task and gets a chance to "see" that the event has been set.

Miky Dinescu
What would be the proper way to close my thread. The `Stop()`/`Start()` are used for when I reconfigure the port.
Daniel A. White
There are a few ways you could do it and I've given an example using a ManualResetEvent which is a pretty common mechanism..
Miky Dinescu
Sweet. Thank you!
Daniel A. White
You're welcome.. I hope you understand why Thread.Abort is not good practice. If you would like to find out more on the subject - there is a really great book about concurrency in Windows by Joe Duffy: http://www.bluebytesoftware.com/books/winconc/winconc_book_resources.html
Miky Dinescu
A: 

Ran into a similar situation where I attempted to create serial port connections local to a single method.

The idea was to create up to four serialPort objects (For each serial port). Once one of the ports came back with good data I'd know which port my device was connected to. I'd discard all my "trial" objects and create a new connection to the specific com port.

Each time through the method I'd create/discard my serialPort objects. Oops. It so happens that if the GC hadn't run by the time I called the method again it would attempt to create a second serial connection replacing the first connection and they would collide. That would trigger this error.

Solution: make all serial port connection objects global to the class. Ugly Hack: Yes, but it Works

Jeff