Fix: Diagnostics Server IPC thread causes 20-40ms shutdown delays on Windows

src/debug/debug-pal/unix/diagnosticsipc.cpp

@@ -15,7 +15,7 @@
 IpcStream::DiagnosticsIpc::DiagnosticsIpc(const int serverSocket, sockaddr_un *const pServerAddress) :
     _serverSocket(serverSocket),
     _pServerAddress(new sockaddr_un),
-    _isUnlinked(false)
+    _isClosed(false)
 {
     _ASSERTE(_pServerAddress != nullptr);
     _ASSERTE(_serverSocket != -1);
@@ -28,15 +28,8 @@ IpcStream::DiagnosticsIpc::DiagnosticsIpc(const int serverSocket, sockaddr_un *c
 
 IpcStream::DiagnosticsIpc::~DiagnosticsIpc()
 {
-    if (_serverSocket != -1)
-    {
-        const int fSuccessClose = ::close(_serverSocket);
-        _ASSERTE(fSuccessClose != -1); // TODO: Add error handling?
-
-        Unlink();
-
-        delete _pServerAddress;
-    }
+    Close();
+    delete _pServerAddress;
 }
 
 IpcStream::DiagnosticsIpc *IpcStream::DiagnosticsIpc::Create(const char *const pIpcName, ErrorCallback callback)
@@ -56,8 +49,10 @@ IpcStream::DiagnosticsIpc *IpcStream::DiagnosticsIpc::Create(const char *const p
         _ASSERTE(!"Failed to create diagnostics IPC socket.");
         return nullptr;
     }
+
     sockaddr_un serverAddress{};
     serverAddress.sun_family = AF_UNIX;
+
     const ProcessDescriptor pd = ProcessDescriptor::FromCurrentProcess();
     PAL_GetTransportName(
         sizeof(serverAddress.sun_path),
@@ -134,6 +129,25 @@ IpcStream *IpcStream::DiagnosticsIpc::Accept(ErrorCallback callback) const
     return new IpcStream(clientSocket);
 }
 
+void IpcStream::DiagnosticsIpc::Close(ErrorCallback callback)
+{
+    if (_isClosed)
+        return;
+    _isClosed = true;
+
+    if (_serverSocket != -1)
+    {
+        if (::close(_serverSocket) == -1)
+        {
+            if (callback != nullptr)
+                callback(strerror(errno), errno);
+            _ASSERTE(!"Failed to close unix domain socket.");
+        }
+
+        Unlink(callback);
+    }
+}
+
 //! This helps remove the socket from the filesystem when the runtime exits.
 //! From: http://man7.org/linux/man-pages/man7/unix.7.html#NOTES
 //!   Binding to a socket with a filename creates a socket in the
@@ -143,16 +157,12 @@ IpcStream *IpcStream::DiagnosticsIpc::Accept(ErrorCallback callback) const
 //!   removed from the filesystem when the last reference to it is closed.
 void IpcStream::DiagnosticsIpc::Unlink(ErrorCallback callback)
 {
-    if (_isUnlinked)
-        return;
-    _isUnlinked = true;
-
     const int fSuccessUnlink = ::unlink(_pServerAddress->sun_path);
     if (fSuccessUnlink == -1)
     {
         if (callback != nullptr)
             callback(strerror(errno), errno);
-        _ASSERTE(fSuccessUnlink == 0);
+        _ASSERTE(!"Failed to unlink server address.");
     }
 }
 

src/debug/debug-pal/win/diagnosticsipc.cpp

@@ -13,6 +13,7 @@ IpcStream::DiagnosticsIpc::DiagnosticsIpc(const char(&namedPipeName)[MaxNamedPip
 
 IpcStream::DiagnosticsIpc::~DiagnosticsIpc()
 {
+    Close();
 }
 
 IpcStream::DiagnosticsIpc *IpcStream::DiagnosticsIpc::Create(const char *const pIpcName, ErrorCallback callback)
@@ -73,7 +74,7 @@ IpcStream *IpcStream::DiagnosticsIpc::Accept(ErrorCallback callback) const
     return new IpcStream(hPipe);
 }
 
-void IpcStream::DiagnosticsIpc::Unlink(ErrorCallback)
+void IpcStream::DiagnosticsIpc::Close(ErrorCallback)
 {
 }
 

src/debug/inc/diagnosticsipc.h

@@ -34,18 +34,21 @@ class IpcStream final
         //! Enables the underlaying IPC implementation to accept connection.
         IpcStream *Accept(ErrorCallback callback = nullptr) const;
 
-        //! Used to unlink the socket so it can be removed from the filesystem
-        //! when the last reference to it is closed.
-        void Unlink(ErrorCallback callback = nullptr);
+        //! Closes an open IPC.
+        void Close(ErrorCallback callback = nullptr);
 
     private:
 
 #ifdef FEATURE_PAL
         const int _serverSocket;
         sockaddr_un *const _pServerAddress;
-        bool _isUnlinked = false;
+        bool _isClosed;
 
         DiagnosticsIpc(const int serverSocket, sockaddr_un *const pServerAddress);
+
+        //! Used to unlink the socket so it can be removed from the filesystem
+        //! when the last reference to it is closed.
+        void Unlink(ErrorCallback callback = nullptr);
 #else
         static const uint32_t MaxNamedPipeNameLength = 256;
         char _pNamedPipeName[MaxNamedPipeNameLength]; // https://docs.microsoft.com/en-us/windows/desktop/api/winbase/nf-winbase-createnamedpipea

src/vm/diagnosticserver.cpp

@@ -20,20 +20,21 @@
 #ifdef FEATURE_PERFTRACING
 
 IpcStream::DiagnosticsIpc *DiagnosticServer::s_pIpc = nullptr;
+Volatile<bool> DiagnosticServer::s_shuttingDown(false);
+HANDLE DiagnosticServer::s_hServerThread = INVALID_HANDLE_VALUE;
 
-static DWORD WINAPI DiagnosticsServerThread(LPVOID lpThreadParameter)
+DWORD WINAPI DiagnosticServer::DiagnosticsServerThread(LPVOID)
 {
     CONTRACTL
     {
         NOTHROW;
         GC_TRIGGERS;
         MODE_PREEMPTIVE;
-        PRECONDITION(lpThreadParameter != nullptr);
+        PRECONDITION(s_pIpc != nullptr);
     }
     CONTRACTL_END;
 
-    auto pIpc = reinterpret_cast<IpcStream::DiagnosticsIpc *>(lpThreadParameter);
-    if (pIpc == nullptr)
+    if (s_pIpc == nullptr)
     {
         STRESS_LOG0(LF_DIAGNOSTICS_PORT, LL_ERROR, "Diagnostics IPC listener was undefined\n");
         return 1;
@@ -45,11 +46,11 @@ static DWORD WINAPI DiagnosticsServerThread(LPVOID lpThreadParameter)
 
     EX_TRY
     {
-        while (true)
+        while (!s_shuttingDown)
         {
             // FIXME: Ideally this would be something like a std::shared_ptr
-            IpcStream *pStream = pIpc->Accept(LoggingCallback);
-            
+            IpcStream *pStream = s_pIpc->Accept(LoggingCallback);
+
             if (pStream == nullptr)
                 continue;
 #ifdef FEATURE_AUTO_TRACE
@@ -140,16 +141,19 @@ bool DiagnosticServer::Initialize()
             auto_trace_launch();
 #endif
             DWORD dwThreadId = 0;
-            HANDLE hThread = ::CreateThread( // TODO: Is it correct to have this "lower" level call here?
+            s_hServerThread = ::CreateThread( // TODO: Is it correct to have this "lower" level call here?
                 nullptr,                     // no security attribute
                 0,                           // default stack size
                 DiagnosticsServerThread,     // thread proc
                 (LPVOID)s_pIpc,              // thread parameter
                 0,                           // not suspended
                 &dwThreadId);                // returns thread ID
 
-            if (hThread == nullptr)
+            if (s_hServerThread == NULL)
             {
+                delete s_pIpc;
+                s_pIpc = nullptr;
+
                 // Failed to create IPC thread.
                 STRESS_LOG1(
                     LF_DIAGNOSTICS_PORT,                                 // facility
@@ -162,10 +166,6 @@ bool DiagnosticServer::Initialize()
 #ifdef FEATURE_AUTO_TRACE
                 auto_trace_wait();
 #endif
-                // FIXME: Maybe hold on to the thread to abort/cleanup at exit?
-                ::CloseHandle(hThread);
-
-                // TODO: Add error handling?
                 fSuccess = true;
             }
         }
@@ -191,6 +191,8 @@ bool DiagnosticServer::Shutdown()
 
     bool fSuccess = false;
 
+    s_shuttingDown = true;
+
     EX_TRY
     {
         if (s_pIpc != nullptr)
@@ -199,11 +201,35 @@ bool DiagnosticServer::Shutdown()
                 STRESS_LOG2(
                     LF_DIAGNOSTICS_PORT,                                  // facility
                     LL_ERROR,                                             // level
-                    "Failed to unlink diagnostic IPC: error (%d): %s.\n", // msg
+                    "Failed to close diagnostic IPC: error (%d): %s.\n",  // msg
                     code,                                                 // data1
                     szMessage);                                           // data2
             };
-            s_pIpc->Unlink(ErrorCallback);
+            s_pIpc->Close(ErrorCallback); // This will break the accept waiting for client connection.
+
+            if (s_hServerThread != NULL)
+            {
+#ifndef FEATURE_PAL
+                if (::CancelSynchronousIo(s_hServerThread) == 0)
+                {
+                    _ASSERTE(!"Failed to mark pending synchronous I/O operations issued by Diagnostics Server Thread as canceled.");
+                }
+#endif // FEATURE_PAL
+
+                // At this point, IO operations on the server thread through the
+                // IPC channel has been closed/cancelled.
+
+                // On non-Windows, this function is blocking on threads that already exit.
+                // ::WaitForSingleObject(s_hServerThread, INFINITE);
+
+                // Close the thread handle (dispose OS resource).
+                ::CloseHandle(s_hServerThread);
+                s_hServerThread = INVALID_HANDLE_VALUE;
+            }
+
+            // If we do not wait for thread to teardown, then we cannot delete this object.
+            // delete s_pIpc;
+            // s_pIpc = nullptr;
         }
         fSuccess = true;
     }

src/vm/diagnosticserver.h

@@ -42,8 +42,13 @@ class DiagnosticServer final
     //! Shutdown the event pipe.
     static bool Shutdown();
 
+    //! Diagnostics server thread.
+    static DWORD WINAPI DiagnosticsServerThread(LPVOID lpThreadParameter);
+
 private:
     static IpcStream::DiagnosticsIpc *s_pIpc;
+    static Volatile<bool> s_shuttingDown;
+    static HANDLE s_hServerThread;
 };
 
 #endif // FEATURE_PERFTRACING