Moved background thread to a task in SerialPort (#19146)

* Moved background thread to a task in SerialPort

* Fixed feedback, removed SafelyWaitForCommEvent and used factory to start the task.

* Made WaitForCommEvent internal so it can be awaited in a task

1 files changed, 7 insertions, 43 deletions

diff --git a/src/System.IO.Ports/src/System/IO/Ports/SerialStream.cs b/src/System.IO.Ports/src/System/IO/Ports/SerialStream.cs
index a5029bfc78..d84a2510de 100644
--- a/src/System.IO.Ports/src/System/IO/Ports/SerialStream.cs
+++ b/src/System.IO.Ports/src/System/IO/Ports/SerialStream.cs
@@ -8,6 +8,7 @@ using System.Diagnostics.CodeAnalysis;
 using System.Globalization;
 using System.Runtime.InteropServices;
 using System.Threading;
+using System.Threading.Tasks;
 
 // Notes about the SerialStream:
 //  * The stream is always opened via the SerialStream constructor.
@@ -52,6 +53,7 @@ namespace System.IO.Ports
         // internal-use members
         internal SafeFileHandle _handle = null;
         internal EventLoopRunner _eventRunner;
+        private Task _waitForComEventTask = null;
 
         private byte[] _tempBuf;                 // used to avoid multiple array allocations in ReadByte()
 
@@ -714,13 +716,8 @@ namespace System.IO.Ports
 
                 // prep. for starting event cycle.
                 _eventRunner = new EventLoopRunner(this);
-
-                // We should consider migrating to a Task here rather than simple background Thread
-                // This would let any exceptions be marshalled back to the owner of the SerialPort
-                // Some discussion in GH issue #17666
-                Thread eventLoopThread = new Thread(_eventRunner.SafelyWaitForCommEvent);
-                eventLoopThread.IsBackground = true;
-                eventLoopThread.Start();
+                _waitForComEventTask = Task.Factory.StartNew(s => ((EventLoopRunner)s).WaitForCommEvent(), _eventRunner, CancellationToken.None,
+                                                                   TaskCreationOptions.DenyChildAttach | TaskCreationOptions.LongRunning, TaskScheduler.Default);
             }
             catch
             {
@@ -745,7 +742,6 @@ namespace System.IO.Ports
             {
                 try
                 {
-
                     _eventRunner.endEventLoop = true;
 
                     Thread.MemoryBarrier();
@@ -793,12 +789,9 @@ namespace System.IO.Ports
                         DiscardOutBuffer();
                     }
 
-                    if (disposing && _eventRunner != null)
+                    if (disposing && _eventRunner != null && _waitForComEventTask != null)
                     {
-                        // now we need to wait for the event loop to tell us it's done.  Without this we could get into a race where the
-                        // event loop kept the port open even after Dispose ended.
-                        _eventRunner.eventLoopEndedSignal.WaitOne();
-                        _eventRunner.eventLoopEndedSignal.Close();
+                        _waitForComEventTask.GetAwaiter().GetResult();
                         _eventRunner.waitCommEventWaitHandle.Close();
                     }
                 }
@@ -1647,7 +1640,6 @@ namespace System.IO.Ports
         internal sealed class EventLoopRunner
         {
             private WeakReference streamWeakReference;
-            internal ManualResetEvent eventLoopEndedSignal = new ManualResetEvent(false);
             internal ManualResetEvent waitCommEventWaitHandle = new ManualResetEvent(false);
             private SafeFileHandle handle = null;
             private bool isAsync;
@@ -1686,36 +1678,9 @@ namespace System.IO.Ports
                 }
             }
 
-            /// <summary>
-            /// Call WaitForCommEvent (which is a thread function for a background thread)
-            /// within an exception handler, so that unhandled exceptions in WaitForCommEvent
-            /// don't cause process termination
-            /// Ultimately it would be good to migrate to a Task here rather than simple background Thread
-            /// This would let any exceptions be marshalled back to the owner of the SerialPort
-            /// Some discussion in GH issue #17666
-            /// </summary>
-            internal void SafelyWaitForCommEvent()
-            {
-                try
-                {
-                    WaitForCommEvent();
-                }
-                catch (ObjectDisposedException)
-                {
-                    // These can happen in some messy tear-down situations (e.g. unexpected USB unplug)
-                    // See GH issue #17661
-                }
-                catch (Exception ex)
-                {
-                    // We don't know of any reason why this should happen, but we still
-                    // don't want process termination
-                    Debug.Fail("Unhandled exception thrown from WaitForCommEvent", ex.ToString());
-                }
-            }
-
             // This is the blocking method that waits for an event to occur.  It wraps the SDK's WaitCommEvent function.
             [SuppressMessage("Microsoft.Interoperability", "CA1404:CallGetLastErrorImmediatelyAfterPInvoke", Justification = "this is debug-only code")]
-            private unsafe void WaitForCommEvent()
+            internal unsafe void WaitForCommEvent()
             {
                 int unused = 0;
                 bool doCleanup = false;
@@ -1808,7 +1773,6 @@ namespace System.IO.Ports
                     endEventLoop = true;
                     Overlapped.Free(intOverlapped);
                 }
-                eventLoopEndedSignal.Set();
             }
 
             private unsafe void FreeNativeOverlappedCallback(uint errorCode, uint numBytes, NativeOverlapped* pOverlapped)