From b4e32e185b3d76b5464e893fdfd9cf59bed174e0 Mon Sep 17 00:00:00 2001 From: John Salem Date: Wed, 5 Aug 2020 17:38:32 -0700 Subject: [PATCH 01/15] Rebrand Diagnostic Server to Diagnostic Port * allows for multiple ports to be specified * multiple ports can be in a suspend state * resume only works if all suspend ports have resumed --- src/coreclr/src/inc/clrconfigvalues.h | 6 +- src/coreclr/src/vm/diagnosticserver.cpp | 61 ++++----- src/coreclr/src/vm/ipcstreamfactory.cpp | 167 ++++++++++++++++++------ src/coreclr/src/vm/ipcstreamfactory.h | 83 ++++++++++-- 4 files changed, 227 insertions(+), 90 deletions(-) diff --git a/src/coreclr/src/inc/clrconfigvalues.h b/src/coreclr/src/inc/clrconfigvalues.h index 3cd7db89bb7e9e..32d07226003dc7 100644 --- a/src/coreclr/src/inc/clrconfigvalues.h +++ b/src/coreclr/src/inc/clrconfigvalues.h @@ -710,10 +710,10 @@ RETAIL_CONFIG_DWORD_INFO(INTERNAL_EventPipeCircularMB, W("EventPipeCircularMB"), RETAIL_CONFIG_DWORD_INFO(INTERNAL_EventPipeProcNumbers, W("EventPipeProcNumbers"), 0, "Enable/disable capturing processor numbers in EventPipe event headers") // -// Diagnostics Server +// Diagnostics Ports // -RETAIL_CONFIG_STRING_INFO_EX(EXTERNAL_DOTNET_DiagnosticsMonitorAddress, W("DOTNET_DiagnosticsMonitorAddress"), "NamedPipe path without '\\\\.\\pipe\\' on Windows; Full path of Unix Domain Socket on Linux/Unix. Used for Diagnostics Monitoring Agents.", CLRConfig::DontPrependCOMPlus_); -RETAIL_CONFIG_DWORD_INFO_EX(EXTERNAL_DOTNET_DiagnosticsMonitorPauseOnStart, W("DOTNET_DiagnosticsMonitorPauseOnStart"), 1, "If DOTNET_DiagnosticsMonitorAddress is set, this will cause the runtime to pause during startup. Resume using the Diagnostics IPC ResumeStartup command.", CLRConfig::DontPrependCOMPlus_); +RRETAIL_CONFIG_DWORD_INFO_EX(EXTERNAL_DOTNET_DiagnosticPortSuspend, W("DOTNET_DiagnosticPortSuspend"), 1, "This will cause the runtime to pause during startup before major subsystems are started. Resume using the Diagnostics IPC ResumeStartup command.", CLRConfig::DontPrependCOMPlus_); +RETAIL_CONFIG_STRING_INFO_EX(EXTERNAL_DOTNET_DiagnosticPorts, W("DOTNET_DiagnosticPorts"), "A semicolon delimited list of additional Diagnostic Ports, where a Diagnostic Port is a NamedPipe path without '\\\\.\\pipe\\' on Windows or the full path of Unix Domain Socket on Linux/Unix followed by optional tags, e.g., ',listen,nosuspend;,connect'", CLRConfig::DontPrependCOMPlus_); // // LTTng diff --git a/src/coreclr/src/vm/diagnosticserver.cpp b/src/coreclr/src/vm/diagnosticserver.cpp index b04def7957b179..e7e17eea25f57e 100644 --- a/src/coreclr/src/vm/diagnosticserver.cpp +++ b/src/coreclr/src/vm/diagnosticserver.cpp @@ -36,7 +36,7 @@ DWORD WINAPI DiagnosticServer::DiagnosticsServerThread(LPVOID) } CONTRACTL_END; - if (!IpcStreamFactory::HasActiveConnections()) + if (!IpcStreamFactory::HasActivePorts()) { STRESS_LOG0(LF_DIAGNOSTICS_PORT, LL_ERROR, "Diagnostics IPC listener was undefined\n"); return 1; @@ -142,30 +142,18 @@ bool DiagnosticServer::Initialize() szMessage); // data2 }; - NewArrayHolder address = nullptr; - CLRConfigStringHolder wAddress = CLRConfig::GetConfigValue(CLRConfig::EXTERNAL_DOTNET_DiagnosticsMonitorAddress); - int nCharactersWritten = 0; - if (wAddress != nullptr) + // Ports can fail to be configured + bool fAnyErrors = IpcStreamFactory::Configure(ErrorCallback); + if (fAnyErrors) + STRESS_LOG0(LF_DIAGNOSTICS_PORT, LL_WARNING, "At least one Diagnostic Port fails to be configured.\n"); + + if (IpcStreamFactory::AnySuspendedPorts()) { - // By default, opts in to Pause on Start s_ResumeRuntimeStartupEvent = new CLREventStatic(); s_ResumeRuntimeStartupEvent->CreateManualEvent(false); - - nCharactersWritten = WideCharToMultiByte(CP_UTF8, 0, wAddress, -1, NULL, 0, NULL, NULL); - if (nCharactersWritten != 0) - { - address = new char[nCharactersWritten]; - nCharactersWritten = WideCharToMultiByte(CP_UTF8, 0, wAddress, -1, address, nCharactersWritten, NULL, NULL); - assert(nCharactersWritten != 0); - } - - // Create the client mode connection - fSuccess &= IpcStreamFactory::CreateClient(address, ErrorCallback); } - fSuccess &= IpcStreamFactory::CreateServer(nullptr, ErrorCallback); - - if (IpcStreamFactory::HasActiveConnections()) + if (IpcStreamFactory::HasActivePorts()) { #ifdef FEATURE_AUTO_TRACE auto_trace_init(); @@ -182,7 +170,7 @@ bool DiagnosticServer::Initialize() if (hServerThread == NULL) { - IpcStreamFactory::CloseConnections(); + IpcStreamFactory::ClosePorts(); // Failed to create IPC thread. STRESS_LOG1( @@ -227,7 +215,7 @@ bool DiagnosticServer::Shutdown() EX_TRY { - if (IpcStreamFactory::HasActiveConnections()) + if (IpcStreamFactory::HasActivePorts()) { auto ErrorCallback = [](const char *szMessage, uint32_t code) { STRESS_LOG2( @@ -264,22 +252,20 @@ void DiagnosticServer::PauseForDiagnosticsMonitor() } CONTRACTL_END; - CLRConfigStringHolder pDotnetDiagnosticsMonitorAddress = CLRConfig::GetConfigValue(CLRConfig::EXTERNAL_DOTNET_DiagnosticsMonitorAddress); - if (pDotnetDiagnosticsMonitorAddress != nullptr) + if (IpcStreamFactory::AnySuspendedPorts()) { - DWORD dwDotnetDiagnosticsMonitorPauseOnStart = CLRConfig::GetConfigValue(CLRConfig::EXTERNAL_DOTNET_DiagnosticsMonitorPauseOnStart); - if (dwDotnetDiagnosticsMonitorPauseOnStart != 0) + _ASSERTE(s_ResumeRuntimeStartupEvent != nullptr && s_ResumeRuntimeStartupEvent->IsValid()); + STRESS_LOG0(LF_DIAGNOSTICS_PORT, LL_ALWAYS, "The runtime has been configured to pause during startup and is awaiting a Diagnostics IPC ResumeStartup command."); + const DWORD dwFiveSecondWait = s_ResumeRuntimeStartupEvent->Wait(5000, false); + if (dwFiveSecondWait == WAIT_TIMEOUT) { - _ASSERTE(s_ResumeRuntimeStartupEvent != nullptr && s_ResumeRuntimeStartupEvent->IsValid()); - STRESS_LOG0(LF_DIAGNOSTICS_PORT, LL_ALWAYS, "The runtime has been configured to pause during startup and is awaiting a Diagnostics IPC ResumeStartup command."); - const DWORD dwFiveSecondWait = s_ResumeRuntimeStartupEvent->Wait(5000, false); - if (dwFiveSecondWait == WAIT_TIMEOUT) - { - wprintf(W("The runtime has been configured to pause during startup and is awaiting a Diagnostics IPC ResumeStartup command from a server at '%s'.\n"), (LPWSTR)pDotnetDiagnosticsMonitorAddress); - fflush(stdout); - STRESS_LOG0(LF_DIAGNOSTICS_PORT, LL_ALWAYS, "The runtime has been configured to pause during startup and is awaiting a Diagnostics IPC ResumeStartup command and has waited 5 seconds."); - const DWORD dwWait = s_ResumeRuntimeStartupEvent->Wait(INFINITE, false); - } + CLRConfigStringHolder dotnetDiagnosticPortString = CLRConfig::GetConfigValue(CLRConfig::EXTERNAL_DOTNET_DiagnosticPorts); + LPCWSTR dotnetDiagnosticPortPrintable = dotnetDiagnosticPortString == nullptr ? W("") : dotnetDiagnosticPortString; + wprintf(W("The runtime has been configured to pause during startup and is awaiting a Diagnostics IPC ResumeStartup command from a server in the following list:\n")); + wprintf(W("DOTNET_DiagnosticPorts=\"%s\"\n"), dotnetDiagnosticPortPrintable); + fflush(stdout); + STRESS_LOG0(LF_DIAGNOSTICS_PORT, LL_ALWAYS, "The runtime has been configured to pause during startup and is awaiting a Diagnostics IPC ResumeStartup command and has waited 5 seconds."); + const DWORD dwWait = s_ResumeRuntimeStartupEvent->Wait(INFINITE, false); } } // allow wait failures to fall through and the runtime to continue coming up @@ -288,7 +274,8 @@ void DiagnosticServer::PauseForDiagnosticsMonitor() void DiagnosticServer::ResumeRuntimeStartup() { LIMITED_METHOD_CONTRACT; - if (s_ResumeRuntimeStartupEvent != nullptr && s_ResumeRuntimeStartupEvent->IsValid()) + IpcStreamFactory::ResumeCurrentPort(); + if (!IpcStreamFactory::AnySuspendedPorts() && s_ResumeRuntimeStartupEvent != nullptr && s_ResumeRuntimeStartupEvent->IsValid()) s_ResumeRuntimeStartupEvent->Set(); } diff --git a/src/coreclr/src/vm/ipcstreamfactory.cpp b/src/coreclr/src/vm/ipcstreamfactory.cpp index d24f14c8340ebf..df18c7602585a0 100644 --- a/src/coreclr/src/vm/ipcstreamfactory.cpp +++ b/src/coreclr/src/vm/ipcstreamfactory.cpp @@ -7,10 +7,27 @@ #ifdef FEATURE_PERFTRACING -CQuickArrayList IpcStreamFactory::s_rgpConnectionStates = CQuickArrayList(); +CQuickArrayList IpcStreamFactory::s_rgpDiagnosticPorts = CQuickArrayList(); Volatile IpcStreamFactory::s_isShutdown = false; +IpcStreamFactory::DiagnosticPort *IpcStreamFactory::s_currentPort = nullptr; -bool IpcStreamFactory::ClientConnectionState::GetIpcPollHandle(IpcStream::DiagnosticsIpc::IpcPollHandle *pIpcPollHandle, ErrorCallback callback) +CQuickArrayList split(LPSTR string, LPCSTR delimiters) +{ + CQuickArrayList parts; + char *context; + char *portConfig = nullptr; + for (char *cursor = string; ; cursor = nullptr) + { + if ((portConfig = strtok_s(cursor, delimiters, &context)) != nullptr) + parts.Push(portConfig); + else + break; + } + + return parts; +} + +bool IpcStreamFactory::ConnectDiagnosticPort::GetIpcPollHandle(IpcStream::DiagnosticsIpc::IpcPollHandle *pIpcPollHandle, ErrorCallback callback) { if (_pStream == nullptr) { @@ -36,81 +53,135 @@ bool IpcStreamFactory::ClientConnectionState::GetIpcPollHandle(IpcStream::Diagno return true; } -IpcStream *IpcStreamFactory::ClientConnectionState::GetConnectedStream(ErrorCallback callback) +IpcStream *IpcStreamFactory::ConnectDiagnosticPort::GetConnectedStream(ErrorCallback callback) { IpcStream *pStream = _pStream; _pStream = nullptr; return pStream; } -void IpcStreamFactory::ClientConnectionState::Reset(ErrorCallback callback) +void IpcStreamFactory::ConnectDiagnosticPort::Reset(ErrorCallback callback) { delete _pStream; _pStream = nullptr; } -bool IpcStreamFactory::ServerConnectionState::GetIpcPollHandle(IpcStream::DiagnosticsIpc::IpcPollHandle *pIpcPollHandle, ErrorCallback callback) +bool IpcStreamFactory::ListenDiagnosticPort::GetIpcPollHandle(IpcStream::DiagnosticsIpc::IpcPollHandle *pIpcPollHandle, ErrorCallback callback) { *pIpcPollHandle = { _pIpc, nullptr, 0, this }; return true; } -IpcStream *IpcStreamFactory::ServerConnectionState::GetConnectedStream(ErrorCallback callback) +IpcStream *IpcStreamFactory::ListenDiagnosticPort::GetConnectedStream(ErrorCallback callback) { return _pIpc->Accept(callback); } // noop for server -void IpcStreamFactory::ServerConnectionState::Reset(ErrorCallback) +void IpcStreamFactory::ListenDiagnosticPort::Reset(ErrorCallback) { return; } -bool IpcStreamFactory::CreateServer(const char *const pIpcName, ErrorCallback callback) +bool IpcStreamFactory::Configure(ErrorCallback callback) { - IpcStream::DiagnosticsIpc *pIpc = IpcStream::DiagnosticsIpc::Create(pIpcName, IpcStream::DiagnosticsIpc::ConnectionMode::SERVER, callback); - if (pIpc != nullptr) + bool fSuccess = true; + + NewArrayHolder dotnetDiagnosticPorts = nullptr; + CLRConfigStringHolder dotnetDiagnosticPortsW = CLRConfig::GetConfigValue(CLRConfig::EXTERNAL_DOTNET_DiagnosticPorts); + int nCharactersWritten = 0; + if (dotnetDiagnosticPortsW != nullptr) { - if (pIpc->Listen(callback)) + nCharactersWritten = WideCharToMultiByte(CP_UTF8, 0, dotnetDiagnosticPortsW, -1, NULL, 0, NULL, NULL); + if (nCharactersWritten != 0) { - s_rgpConnectionStates.Push(new ServerConnectionState(pIpc)); - return true; + dotnetDiagnosticPorts = new char[nCharactersWritten]; + nCharactersWritten = WideCharToMultiByte(CP_UTF8, 0, dotnetDiagnosticPortsW, -1, dotnetDiagnosticPorts, nCharactersWritten, NULL, NULL); + assert(nCharactersWritten != 0); } - else + + CQuickArrayList portConfigs = split(dotnetDiagnosticPorts, ";"); + while (portConfigs.Size() > 0) { - delete pIpc; - return false; + LPSTR portConfig = portConfigs.Pop(); + STRESS_LOG1(LF_DIAGNOSTICS_PORT, LL_INFO10, "IpcStreamFactory::Configure - Attempted to create Diagnostic Port from \"%s\".\n", portConfig); + CQuickArrayList portConfigParts = split(portConfig, ","); + DiagnosticPortBuilder builder; + + ASSERT(portConfigParts.Size() >= 1); + if (portConfigParts.Size() == 0) + continue; + + builder.WithPath(portConfigParts.Pop()); + while (portConfigParts.Size() > 0) + builder.WithTag(portConfigParts.Pop()); + + const bool fBuildSuccess = BuildAndAddPort(builder, callback); + STRESS_LOG1(LF_DIAGNOSTICS_PORT, LL_INFO10, "IpcStreamFactory::Configure - Diagnostic Port creation succeeded? %d \n", fBuildSuccess); + fSuccess &= fBuildSuccess; } } - else - { - return false; - } + + // create the default listen port + DWORD dotnetDiagnosticPortSuspend = CLRConfig::GetConfigValue(CLRConfig::EXTERNAL_DOTNET_DiagnosticPortSuspend); + DiagnosticPortBuilder defaultListenPortBuilder = DiagnosticPortBuilder{} + .WithPath(nullptr) + .WithSuspendMode(dotnetDiagnosticPortSuspend > 0 ? DiagnosticPortSuspendMode::SUSPEND : DiagnosticPortSuspendMode::NOSUSPEND) + .WithType(DiagnosticPortType::LISTEN); + + + fSuccess &= BuildAndAddPort(defaultListenPortBuilder, callback); + return fSuccess; } -bool IpcStreamFactory::CreateClient(const char *const pIpcName, ErrorCallback callback) +bool IpcStreamFactory::BuildAndAddPort(IpcStreamFactory::DiagnosticPortBuilder builder, ErrorCallback callback) { - IpcStream::DiagnosticsIpc *pIpc = IpcStream::DiagnosticsIpc::Create(pIpcName, IpcStream::DiagnosticsIpc::ConnectionMode::CLIENT, callback); - if (pIpc != nullptr) + if (builder.Type == DiagnosticPortType::LISTEN) { - s_rgpConnectionStates.Push(new ClientConnectionState(pIpc)); - return true; + IpcStream::DiagnosticsIpc *pIpc = IpcStream::DiagnosticsIpc::Create(builder.Path, IpcStream::DiagnosticsIpc::ConnectionMode::SERVER, callback); + if (pIpc != nullptr) + { + if (pIpc->Listen(callback)) + { + s_rgpDiagnosticPorts.Push(new ListenDiagnosticPort(pIpc, builder)); + return true; + } + else + { + delete pIpc; + return false; + } + } + else + { + return false; + } } - else + else if (builder.Type == DiagnosticPortType::CONNECT) { - return false; + IpcStream::DiagnosticsIpc *pIpc = IpcStream::DiagnosticsIpc::Create(builder.Path, IpcStream::DiagnosticsIpc::ConnectionMode::CLIENT, callback); + if (pIpc != nullptr) + { + s_rgpDiagnosticPorts.Push(new ConnectDiagnosticPort(pIpc, builder)); + return true; + } + else + { + return false; + } } + return false; } -bool IpcStreamFactory::HasActiveConnections() +bool IpcStreamFactory::HasActivePorts() { - return !s_isShutdown && s_rgpConnectionStates.Size() > 0; + return !s_isShutdown && s_rgpDiagnosticPorts.Size() > 0; } -void IpcStreamFactory::CloseConnections(ErrorCallback callback) +void IpcStreamFactory::ClosePorts(ErrorCallback callback) { - for (uint32_t i = 0; i < (uint32_t)s_rgpConnectionStates.Size(); i++) - s_rgpConnectionStates[i]->Close(callback); + for (uint32_t i = 0; i < (uint32_t)s_rgpDiagnosticPorts.Size(); i++) + s_rgpDiagnosticPorts[i]->Close(callback); } void IpcStreamFactory::Shutdown(ErrorCallback callback) @@ -118,8 +189,22 @@ void IpcStreamFactory::Shutdown(ErrorCallback callback) if (s_isShutdown) return; s_isShutdown = true; - for (uint32_t i = 0; i < (uint32_t)s_rgpConnectionStates.Size(); i++) - s_rgpConnectionStates[i]->Close(true, callback); + for (uint32_t i = 0; i < (uint32_t)s_rgpDiagnosticPorts.Size(); i++) + s_rgpDiagnosticPorts[i]->Close(true, callback); +} + +bool IpcStreamFactory::AnySuspendedPorts() +{ + bool fAnySuspendedPorts = false; + for (uint32_t i = 0; i < (uint32_t)s_rgpDiagnosticPorts.Size(); i++) + fAnySuspendedPorts |= !(s_rgpDiagnosticPorts[i]->SuspendMode == DiagnosticPortSuspendMode::NOSUSPEND || s_rgpDiagnosticPorts[i]->HasResumedRuntime); + return fAnySuspendedPorts; +} + +void IpcStreamFactory::ResumeCurrentPort() +{ + if (s_currentPort != nullptr) + s_currentPort->HasResumedRuntime = true; } // helper function for getting timeout @@ -149,10 +234,10 @@ IpcStream *IpcStreamFactory::GetNextAvailableStream(ErrorCallback callback) while (pStream == nullptr) { fConnectSuccess = true; - for (uint32_t i = 0; i < (uint32_t)s_rgpConnectionStates.Size(); i++) + for (uint32_t i = 0; i < (uint32_t)s_rgpDiagnosticPorts.Size(); i++) { IpcStream::DiagnosticsIpc::IpcPollHandle pollHandle = {}; - if (s_rgpConnectionStates[i]->GetIpcPollHandle(&pollHandle, callback)) + if (s_rgpDiagnosticPorts[i]->GetIpcPollHandle(&pollHandle, callback)) { rgIpcPollHandles.Push(pollHandle); } @@ -178,13 +263,16 @@ IpcStream *IpcStreamFactory::GetNextAvailableStream(ErrorCallback callback) switch ((IpcStream::DiagnosticsIpc::PollEvents)rgIpcPollHandles[i].revents) { case IpcStream::DiagnosticsIpc::PollEvents::HANGUP: - ((ConnectionState*)(rgIpcPollHandles[i].pUserData))->Reset(callback); + ((DiagnosticPort*)(rgIpcPollHandles[i].pUserData))->Reset(callback); STRESS_LOG2(LF_DIAGNOSTICS_PORT, LL_INFO10, "IpcStreamFactory::GetNextAvailableStream - HUP :: Poll attempt: %d, connection %d hung up.\n", nPollAttempts, i); pollTimeoutMs = s_pollTimeoutMinMs; break; case IpcStream::DiagnosticsIpc::PollEvents::SIGNALED: if (pStream == nullptr) // only use first signaled stream; will get others on subsequent calls - pStream = ((ConnectionState*)(rgIpcPollHandles[i].pUserData))->GetConnectedStream(callback); + { + pStream = ((DiagnosticPort*)(rgIpcPollHandles[i].pUserData))->GetConnectedStream(callback); + s_currentPort = (DiagnosticPort*)(rgIpcPollHandles[i].pUserData); + } STRESS_LOG2(LF_DIAGNOSTICS_PORT, LL_INFO10, "IpcStreamFactory::GetNextAvailableStream - SIG :: Poll attempt: %d, connection %d signalled.\n", nPollAttempts, i); break; case IpcStream::DiagnosticsIpc::PollEvents::ERR: @@ -204,7 +292,10 @@ IpcStream *IpcStreamFactory::GetNextAvailableStream(ErrorCallback callback) if (pStream == nullptr && fSawError) + { + s_currentPort = nullptr; return nullptr; + } // clear the view while (rgIpcPollHandles.Size() > 0) diff --git a/src/coreclr/src/vm/ipcstreamfactory.h b/src/coreclr/src/vm/ipcstreamfactory.h index d65994a1e11dcd..80554b7d523884 100644 --- a/src/coreclr/src/vm/ipcstreamfactory.h +++ b/src/coreclr/src/vm/ipcstreamfactory.h @@ -11,14 +11,67 @@ class IpcStreamFactory { public: - struct ConnectionState + // forward declare + struct DiagnosticPort; + + enum class DiagnosticPortType : uint8_t + { + LISTEN = 0, + CONNECT = 1 + }; + + enum class DiagnosticPortSuspendMode : uint8_t + { + NOSUSPEND = 0, + SUSPEND = 1 + }; + + struct DiagnosticPortBuilder + { + LPSTR Path = nullptr; + DiagnosticPortType Type = DiagnosticPortType::LISTEN; + DiagnosticPortSuspendMode SuspendMode = DiagnosticPortSuspendMode::NOSUSPEND; + + DiagnosticPortBuilder WithPath(LPSTR path) { Path = _strdup(path); return *this; } + DiagnosticPortBuilder WithType(DiagnosticPortType type) { Type = type; return *this; } + DiagnosticPortBuilder WithSuspendMode(DiagnosticPortSuspendMode mode) { SuspendMode = mode; return *this; } + DiagnosticPortBuilder WithTag(LPSTR tag) + { + // check if port type + if (_stricmp(tag, "listen") == 0) + return WithType(DiagnosticPortType::LISTEN); + + if (_stricmp(tag, "connect") == 0) + return WithType(DiagnosticPortType::CONNECT); + + // check if suspendmode tag + if (_stricmp(tag, "nosuspend") == 0) + return WithSuspendMode(DiagnosticPortSuspendMode::NOSUSPEND); + + if (_stricmp(tag, "suspend") == 0) + return WithSuspendMode(DiagnosticPortSuspendMode::SUSPEND); + + // don't mutate if it's not a valid option + STRESS_LOG1(LF_DIAGNOSTICS_PORT, LL_INFO10, "IpcStreamFactory::DiagnosticPortBuilder::WithTag - Unknown tag '%s'.\n", tag); + return *this; + } + }; + + struct DiagnosticPort { public: - ConnectionState(IpcStream::DiagnosticsIpc *pIpc) : + DiagnosticPort(IpcStream::DiagnosticsIpc *pIpc, DiagnosticPortBuilder builder) : + SuspendMode(builder.SuspendMode), _pIpc(pIpc), - _pStream(nullptr) + _pStream(nullptr), + _type(builder.Type) { } + const DiagnosticPortSuspendMode SuspendMode; + + // Will be false until ResumeRuntime command is sent on this connection + bool HasResumedRuntime = false; + // returns a pollable handle and performs any preparation required // e.g., as a side-effect, will connect and advertise on reverse connections virtual bool GetIpcPollHandle(IpcStream::DiagnosticsIpc::IpcPollHandle *pIpcPollHandle, ErrorCallback callback = nullptr) = 0; @@ -42,11 +95,12 @@ class IpcStreamFactory protected: IpcStream::DiagnosticsIpc *_pIpc; IpcStream *_pStream; + DiagnosticPortType _type; }; - struct ClientConnectionState : public ConnectionState + struct ConnectDiagnosticPort : public DiagnosticPort { - ClientConnectionState(IpcStream::DiagnosticsIpc *pIpc) : ConnectionState(pIpc) { } + ConnectDiagnosticPort(IpcStream::DiagnosticsIpc *pIpc, DiagnosticPortBuilder builder) : DiagnosticPort(pIpc, builder) { } // returns a pollable handle and performs any preparation required bool GetIpcPollHandle(IpcStream::DiagnosticsIpc::IpcPollHandle *pIpcPollHandle, ErrorCallback callback = nullptr) override; @@ -58,9 +112,9 @@ class IpcStreamFactory void Reset(ErrorCallback callback = nullptr) override; }; - struct ServerConnectionState : public ConnectionState + struct ListenDiagnosticPort : public DiagnosticPort { - ServerConnectionState(IpcStream::DiagnosticsIpc *pIpc) : ConnectionState(pIpc) { } + ListenDiagnosticPort(IpcStream::DiagnosticsIpc *pIpc, DiagnosticPortBuilder builder) : DiagnosticPort(pIpc, builder) { } // returns a pollable handle and performs any preparation required bool GetIpcPollHandle(IpcStream::DiagnosticsIpc::IpcPollHandle *pIpcPollHandle, ErrorCallback callback = nullptr) override; @@ -72,15 +126,20 @@ class IpcStreamFactory void Reset(ErrorCallback callback = nullptr) override; }; - static bool CreateServer(const char *const pIpcName, ErrorCallback = nullptr); - static bool CreateClient(const char *const pIpcName, ErrorCallback = nullptr); + static bool Configure(ErrorCallback callback = nullptr); static IpcStream *GetNextAvailableStream(ErrorCallback = nullptr); - static bool HasActiveConnections(); - static void CloseConnections(ErrorCallback callback = nullptr); + static void ResumeCurrentPort(); + static bool AnySuspendedPorts(); + static bool HasActivePorts(); + static void ClosePorts(ErrorCallback callback = nullptr); static void Shutdown(ErrorCallback callback = nullptr); private: - static CQuickArrayList s_rgpConnectionStates; + static bool BuildAndAddPort(DiagnosticPortBuilder builder, ErrorCallback callback = nullptr); + static CQuickArrayList s_rgpDiagnosticPorts; static Volatile s_isShutdown; + // set this in GetNextAvailableStream, and then expose a callback that + // allows us to track which connections have sent their ResumeRuntime commands + static DiagnosticPort *s_currentPort; // Polling timeout semantics // If client connection is opted in From 000142d2876d424ad9e7b0e1f67f4fb5ca4d5a69 Mon Sep 17 00:00:00 2001 From: John Salem Date: Thu, 6 Aug 2020 17:10:26 -0700 Subject: [PATCH 02/15] Update tests to use new config switches * small bug fixes in config parsing code --- src/coreclr/src/inc/clrconfigvalues.h | 2 +- src/coreclr/src/vm/ipcstreamfactory.cpp | 13 ++++++++----- src/coreclr/src/vm/ipcstreamfactory.h | 2 +- src/tests/tracing/eventpipe/common/IpcUtils.cs | 4 ++-- .../eventpipe/pauseonstart/pauseonstart.cs | 10 +++++----- src/tests/tracing/eventpipe/reverse/reverse.cs | 18 ++++++------------ .../eventpipe/reverseouter/reverseouter.cs | 3 +-- 7 files changed, 24 insertions(+), 28 deletions(-) diff --git a/src/coreclr/src/inc/clrconfigvalues.h b/src/coreclr/src/inc/clrconfigvalues.h index 32d07226003dc7..f92167f0dfdb5b 100644 --- a/src/coreclr/src/inc/clrconfigvalues.h +++ b/src/coreclr/src/inc/clrconfigvalues.h @@ -712,7 +712,7 @@ RETAIL_CONFIG_DWORD_INFO(INTERNAL_EventPipeProcNumbers, W("EventPipeProcNumbers" // // Diagnostics Ports // -RRETAIL_CONFIG_DWORD_INFO_EX(EXTERNAL_DOTNET_DiagnosticPortSuspend, W("DOTNET_DiagnosticPortSuspend"), 1, "This will cause the runtime to pause during startup before major subsystems are started. Resume using the Diagnostics IPC ResumeStartup command.", CLRConfig::DontPrependCOMPlus_); +RETAIL_CONFIG_DWORD_INFO_EX(EXTERNAL_DOTNET_DiagnosticPortSuspend, W("DOTNET_DiagnosticPortSuspend"), 0, "This will cause the runtime to pause during startup before major subsystems are started. Resume using the Diagnostics IPC ResumeStartup command.", CLRConfig::DontPrependCOMPlus_); RETAIL_CONFIG_STRING_INFO_EX(EXTERNAL_DOTNET_DiagnosticPorts, W("DOTNET_DiagnosticPorts"), "A semicolon delimited list of additional Diagnostic Ports, where a Diagnostic Port is a NamedPipe path without '\\\\.\\pipe\\' on Windows or the full path of Unix Domain Socket on Linux/Unix followed by optional tags, e.g., ',listen,nosuspend;,connect'", CLRConfig::DontPrependCOMPlus_); // diff --git a/src/coreclr/src/vm/ipcstreamfactory.cpp b/src/coreclr/src/vm/ipcstreamfactory.cpp index df18c7602585a0..75b7fb3a0c4117 100644 --- a/src/coreclr/src/vm/ipcstreamfactory.cpp +++ b/src/coreclr/src/vm/ipcstreamfactory.cpp @@ -15,11 +15,11 @@ CQuickArrayList split(LPSTR string, LPCSTR delimiters) { CQuickArrayList parts; char *context; - char *portConfig = nullptr; + char *part = nullptr; for (char *cursor = string; ; cursor = nullptr) { - if ((portConfig = strtok_s(cursor, delimiters, &context)) != nullptr) - parts.Push(portConfig); + if ((part = strtok_s(cursor, delimiters, &context)) != nullptr) + parts.Push(part); else break; } @@ -110,11 +110,14 @@ bool IpcStreamFactory::Configure(ErrorCallback callback) ASSERT(portConfigParts.Size() >= 1); if (portConfigParts.Size() == 0) + { + fSuccess &= false; continue; + } - builder.WithPath(portConfigParts.Pop()); - while (portConfigParts.Size() > 0) + while (portConfigParts.Size() > 1) builder.WithTag(portConfigParts.Pop()); + builder.WithPath(portConfigParts.Pop()); const bool fBuildSuccess = BuildAndAddPort(builder, callback); STRESS_LOG1(LF_DIAGNOSTICS_PORT, LL_INFO10, "IpcStreamFactory::Configure - Diagnostic Port creation succeeded? %d \n", fBuildSuccess); diff --git a/src/coreclr/src/vm/ipcstreamfactory.h b/src/coreclr/src/vm/ipcstreamfactory.h index 80554b7d523884..cfd286beee0bba 100644 --- a/src/coreclr/src/vm/ipcstreamfactory.h +++ b/src/coreclr/src/vm/ipcstreamfactory.h @@ -32,7 +32,7 @@ class IpcStreamFactory DiagnosticPortType Type = DiagnosticPortType::LISTEN; DiagnosticPortSuspendMode SuspendMode = DiagnosticPortSuspendMode::NOSUSPEND; - DiagnosticPortBuilder WithPath(LPSTR path) { Path = _strdup(path); return *this; } + DiagnosticPortBuilder WithPath(LPSTR path) { Path = path != nullptr ? _strdup(path) : nullptr; return *this; } DiagnosticPortBuilder WithType(DiagnosticPortType type) { Type = type; return *this; } DiagnosticPortBuilder WithSuspendMode(DiagnosticPortSuspendMode mode) { SuspendMode = mode; return *this; } DiagnosticPortBuilder WithTag(LPSTR tag) diff --git a/src/tests/tracing/eventpipe/common/IpcUtils.cs b/src/tests/tracing/eventpipe/common/IpcUtils.cs index bf17e1e27e837b..73e817859ad828 100644 --- a/src/tests/tracing/eventpipe/common/IpcUtils.cs +++ b/src/tests/tracing/eventpipe/common/IpcUtils.cs @@ -21,8 +21,8 @@ namespace Tracing.Tests.Common { public static class Utils { - public static readonly string DiagnosticsMonitorAddressEnvKey = "DOTNET_DiagnosticsMonitorAddress"; - public static readonly string DiagnosticsMonitorPauseOnStartEnvKey = "DOTNET_DiagnosticsMonitorPauseOnStart"; + public static readonly string DiagnosticPortsEnvKey = "DOTNET_DiagnosticPorts"; + public static readonly string DiagnosticPortSuspend = "DOTNET_DiagnosticPortSuspend"; public static async Task WaitTillTimeout(Task task, TimeSpan timeout) { diff --git a/src/tests/tracing/eventpipe/pauseonstart/pauseonstart.cs b/src/tests/tracing/eventpipe/pauseonstart/pauseonstart.cs index 4065cd586feef1..e87d367651b06b 100644 --- a/src/tests/tracing/eventpipe/pauseonstart/pauseonstart.cs +++ b/src/tests/tracing/eventpipe/pauseonstart/pauseonstart.cs @@ -28,7 +28,7 @@ public static async Task TEST_RuntimeResumesExecutionWithCommand() var server = new ReverseServer(serverName); Task subprocessTask = Utils.RunSubprocess( currentAssembly: Assembly.GetExecutingAssembly(), - environment: new Dictionary { { Utils.DiagnosticsMonitorAddressEnvKey, serverName } }, + environment: new Dictionary { { Utils.DiagnosticPortsEnvKey, $"{serverName},connect,suspend" } }, duringExecution: async (_) => { Stream stream = await server.AcceptAsync(); @@ -56,7 +56,7 @@ public static async Task TEST_TracesHaveRelevantEvents() using var memoryStream = new MemoryStream(); Task subprocessTask = Utils.RunSubprocess( currentAssembly: Assembly.GetExecutingAssembly(), - environment: new Dictionary { { Utils.DiagnosticsMonitorAddressEnvKey, serverName } }, + environment: new Dictionary { { Utils.DiagnosticPortsEnvKey, $"{serverName},connect,suspend" } }, duringExecution: async (pid) => { Stream stream = await server.AcceptAsync(); @@ -114,7 +114,7 @@ public static async Task TEST_MultipleSessionsCanBeStartedWhilepaused() using var memoryStream3 = new MemoryStream(); Task subprocessTask = Utils.RunSubprocess( currentAssembly: Assembly.GetExecutingAssembly(), - environment: new Dictionary { { Utils.DiagnosticsMonitorAddressEnvKey, serverName } }, + environment: new Dictionary { { Utils.DiagnosticPortsEnvKey, $"{serverName},connect,suspend" } }, duringExecution: async (pid) => { Stream stream = await server.AcceptAsync(); @@ -207,7 +207,7 @@ public static async Task TEST_CanStartAndStopSessionWhilepaused() using var memoryStream3 = new MemoryStream(); Task subprocessTask = Utils.RunSubprocess( currentAssembly: Assembly.GetExecutingAssembly(), - environment: new Dictionary { { Utils.DiagnosticsMonitorAddressEnvKey, serverName } }, + environment: new Dictionary { { Utils.DiagnosticPortsEnvKey, $"{serverName},connect,suspend" } }, duringExecution: async (pid) => { Stream stream = await server.AcceptAsync(); @@ -271,7 +271,7 @@ public static async Task TEST_DisabledCommandsError() using var memoryStream3 = new MemoryStream(); Task subprocessTask = Utils.RunSubprocess( currentAssembly: Assembly.GetExecutingAssembly(), - environment: new Dictionary { { Utils.DiagnosticsMonitorAddressEnvKey, serverName } }, + environment: new Dictionary { { Utils.DiagnosticPortsEnvKey, $"{serverName},connect,suspend" } }, duringExecution: async (pid) => { Stream stream = await server.AcceptAsync(); diff --git a/src/tests/tracing/eventpipe/reverse/reverse.cs b/src/tests/tracing/eventpipe/reverse/reverse.cs index 05436e574d6436..6ce09ea82b81b7 100644 --- a/src/tests/tracing/eventpipe/reverse/reverse.cs +++ b/src/tests/tracing/eventpipe/reverse/reverse.cs @@ -28,8 +28,7 @@ public static async Task TEST_RuntimeIsResilientToServerClosing() currentAssembly: Assembly.GetExecutingAssembly(), environment: new Dictionary { - { Utils.DiagnosticsMonitorAddressEnvKey, serverName }, - { Utils.DiagnosticsMonitorPauseOnStartEnvKey, "0" } + { Utils.DiagnosticPortsEnvKey, $"{serverName},connect,nosuspend" } }, duringExecution: async (_) => { @@ -59,8 +58,7 @@ public static async Task TEST_RuntimeConnectsToExistingServer() currentAssembly: Assembly.GetExecutingAssembly(), environment: new Dictionary { - { Utils.DiagnosticsMonitorAddressEnvKey, serverName }, - { Utils.DiagnosticsMonitorPauseOnStartEnvKey, "0" } + { Utils.DiagnosticPortsEnvKey, $"{serverName},connect,nosuspend" } }, duringExecution: async (_) => { @@ -85,8 +83,7 @@ public static async Task TEST_CanConnectServerAndClientAtSameTime() currentAssembly: Assembly.GetExecutingAssembly(), environment: new Dictionary { - { Utils.DiagnosticsMonitorAddressEnvKey, serverName }, - { Utils.DiagnosticsMonitorPauseOnStartEnvKey, "0" } + { Utils.DiagnosticPortsEnvKey, $"{serverName},connect,nosuspend" } }, duringExecution: async (int pid) => { @@ -139,8 +136,7 @@ public static async Task TEST_ServerWorksIfClientDoesntAccept() currentAssembly: Assembly.GetExecutingAssembly(), environment: new Dictionary { - { Utils.DiagnosticsMonitorAddressEnvKey, serverName }, - { Utils.DiagnosticsMonitorPauseOnStartEnvKey, "0" } + { Utils.DiagnosticPortsEnvKey, $"{serverName},connect,nosuspend" } }, duringExecution: async (int pid) => { @@ -181,8 +177,7 @@ public static async Task TEST_ServerIsResilientToNoBufferAgent() currentAssembly: Assembly.GetExecutingAssembly(), environment: new Dictionary { - { Utils.DiagnosticsMonitorAddressEnvKey, serverName }, - { Utils.DiagnosticsMonitorPauseOnStartEnvKey, "0" } + { Utils.DiagnosticPortsEnvKey, $"{serverName},connect,nosuspend" } }, duringExecution: async (int pid) => { @@ -220,8 +215,7 @@ public static async Task TEST_StandardConnectionStillWorksIfReverseConnect currentAssembly: Assembly.GetExecutingAssembly(), environment: new Dictionary { - { Utils.DiagnosticsMonitorAddressEnvKey, serverName }, - { Utils.DiagnosticsMonitorPauseOnStartEnvKey, "0" } + { Utils.DiagnosticPortsEnvKey, $"{serverName},connect,nosuspend" } }, duringExecution: async (int pid) => { diff --git a/src/tests/tracing/eventpipe/reverseouter/reverseouter.cs b/src/tests/tracing/eventpipe/reverseouter/reverseouter.cs index ff01fbc0f449f1..6d3c0e4657cd84 100644 --- a/src/tests/tracing/eventpipe/reverseouter/reverseouter.cs +++ b/src/tests/tracing/eventpipe/reverseouter/reverseouter.cs @@ -28,8 +28,7 @@ public static async Task TEST_ReverseConnectionCanRecycleWhileTracing() currentAssembly: Assembly.GetExecutingAssembly(), environment: new Dictionary { - { Utils.DiagnosticsMonitorAddressEnvKey, serverName }, - { Utils.DiagnosticsMonitorPauseOnStartEnvKey, "0" } + { Utils.DiagnosticPortsEnvKey, $"{serverName},connect,nosuspend" } }, duringExecution: async (int pid) => { From af9b4cc4e27e37a64e9ca70bf13cc304d549fb52 Mon Sep 17 00:00:00 2001 From: John Salem Date: Thu, 6 Aug 2020 19:30:14 -0700 Subject: [PATCH 03/15] Update diagnosticserver.cpp --- src/coreclr/src/vm/diagnosticserver.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/src/vm/diagnosticserver.cpp b/src/coreclr/src/vm/diagnosticserver.cpp index e7e17eea25f57e..bf2bfdcaa90f85 100644 --- a/src/coreclr/src/vm/diagnosticserver.cpp +++ b/src/coreclr/src/vm/diagnosticserver.cpp @@ -32,7 +32,7 @@ DWORD WINAPI DiagnosticServer::DiagnosticsServerThread(LPVOID) #endif GC_TRIGGERS; MODE_PREEMPTIVE; - PRECONDITION(s_shuttingDown || IpcStreamFactory::HasActiveConnections()); + PRECONDITION(s_shuttingDown || IpcStreamFactory::HasActivePorts()); } CONTRACTL_END; From 5fafe42bea6663dceb6230da17dd7a35cfc8c1eb Mon Sep 17 00:00:00 2001 From: John Salem Date: Fri, 7 Aug 2020 10:04:01 -0700 Subject: [PATCH 04/15] Appease gcc warning --- src/coreclr/src/vm/diagnosticserver.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/src/vm/diagnosticserver.cpp b/src/coreclr/src/vm/diagnosticserver.cpp index e7e17eea25f57e..b3df07360749a2 100644 --- a/src/coreclr/src/vm/diagnosticserver.cpp +++ b/src/coreclr/src/vm/diagnosticserver.cpp @@ -260,7 +260,7 @@ void DiagnosticServer::PauseForDiagnosticsMonitor() if (dwFiveSecondWait == WAIT_TIMEOUT) { CLRConfigStringHolder dotnetDiagnosticPortString = CLRConfig::GetConfigValue(CLRConfig::EXTERNAL_DOTNET_DiagnosticPorts); - LPCWSTR dotnetDiagnosticPortPrintable = dotnetDiagnosticPortString == nullptr ? W("") : dotnetDiagnosticPortString; + LPWSTR dotnetDiagnosticPortPrintable = dotnetDiagnosticPortString == nullptr ? W("") : dotnetDiagnosticPortString; wprintf(W("The runtime has been configured to pause during startup and is awaiting a Diagnostics IPC ResumeStartup command from a server in the following list:\n")); wprintf(W("DOTNET_DiagnosticPorts=\"%s\"\n"), dotnetDiagnosticPortPrintable); fflush(stdout); From b76cf6cd5ca71b93329ae4b3cda492e72f364944 Mon Sep 17 00:00:00 2001 From: John Salem Date: Fri, 7 Aug 2020 11:18:27 -0700 Subject: [PATCH 05/15] Const shenanigans --- src/coreclr/src/vm/diagnosticserver.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/coreclr/src/vm/diagnosticserver.cpp b/src/coreclr/src/vm/diagnosticserver.cpp index c7c780bf1d53e2..5ad6ef9b6d1a0d 100644 --- a/src/coreclr/src/vm/diagnosticserver.cpp +++ b/src/coreclr/src/vm/diagnosticserver.cpp @@ -260,9 +260,8 @@ void DiagnosticServer::PauseForDiagnosticsMonitor() if (dwFiveSecondWait == WAIT_TIMEOUT) { CLRConfigStringHolder dotnetDiagnosticPortString = CLRConfig::GetConfigValue(CLRConfig::EXTERNAL_DOTNET_DiagnosticPorts); - LPWSTR dotnetDiagnosticPortPrintable = dotnetDiagnosticPortString == nullptr ? W("") : dotnetDiagnosticPortString; wprintf(W("The runtime has been configured to pause during startup and is awaiting a Diagnostics IPC ResumeStartup command from a server in the following list:\n")); - wprintf(W("DOTNET_DiagnosticPorts=\"%s\"\n"), dotnetDiagnosticPortPrintable); + wprintf(W("DOTNET_DiagnosticPorts=\"%s\"\n"), dotnetDiagnosticPortString == nullptr ? W("") : dotnetDiagnosticPortString); fflush(stdout); STRESS_LOG0(LF_DIAGNOSTICS_PORT, LL_ALWAYS, "The runtime has been configured to pause during startup and is awaiting a Diagnostics IPC ResumeStartup command and has waited 5 seconds."); const DWORD dwWait = s_ResumeRuntimeStartupEvent->Wait(INFINITE, false); From 51c73771c3928c0cbcbad17285a7130946be38f9 Mon Sep 17 00:00:00 2001 From: John Salem Date: Fri, 7 Aug 2020 12:08:48 -0700 Subject: [PATCH 06/15] Add tests for DiagnosticPort functionality --- .../tracing/eventpipe/common/IpcUtils.cs | 2 + src/tests/tracing/eventpipe/common/Reverse.cs | 1 + .../diagnosticport/diagnosticport.cs | 296 ++++++++++++++++++ .../diagnosticport/diagnosticport.csproj | 17 + 4 files changed, 316 insertions(+) create mode 100644 src/tests/tracing/eventpipe/diagnosticport/diagnosticport.cs create mode 100644 src/tests/tracing/eventpipe/diagnosticport/diagnosticport.csproj diff --git a/src/tests/tracing/eventpipe/common/IpcUtils.cs b/src/tests/tracing/eventpipe/common/IpcUtils.cs index 73e817859ad828..8118ff16b0f26f 100644 --- a/src/tests/tracing/eventpipe/common/IpcUtils.cs +++ b/src/tests/tracing/eventpipe/common/IpcUtils.cs @@ -144,8 +144,10 @@ public static async Task RunSubprocess(Assembly currentAssembly, Dictionar } finally { + Logger.logger.Log($"----------------------------------------"); Logger.logger.Log($"Subprocess stdout: {stdoutSb.ToString()}"); Logger.logger.Log($"Subprocess stderr: {stderrSb.ToString()}"); + Logger.logger.Log($"----------------------------------------"); } diff --git a/src/tests/tracing/eventpipe/common/Reverse.cs b/src/tests/tracing/eventpipe/common/Reverse.cs index 1b99f913d65901..c57c906b441415 100644 --- a/src/tests/tracing/eventpipe/common/Reverse.cs +++ b/src/tests/tracing/eventpipe/common/Reverse.cs @@ -152,6 +152,7 @@ private NamedPipeServerStream GetNewNamedPipeServer() public void Shutdown() { + Logger.logger.Log($"Shutting down Reverse Server at {_serverAddress}"); switch (_server) { case NamedPipeServerStream serverStream: diff --git a/src/tests/tracing/eventpipe/diagnosticport/diagnosticport.cs b/src/tests/tracing/eventpipe/diagnosticport/diagnosticport.cs new file mode 100644 index 00000000000000..319b8cd6db4413 --- /dev/null +++ b/src/tests/tracing/eventpipe/diagnosticport/diagnosticport.cs @@ -0,0 +1,296 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Diagnostics.Tracing; +using System.Diagnostics; +using System.Linq; +using System.Threading.Tasks; +using System.Collections.Generic; +using System.Reflection; +using Microsoft.Diagnostics.Tools.RuntimeClient; +using Tracing.Tests.Common; +using System.Threading; +using System.Text; +using System.IO; +using Microsoft.Diagnostics.Tracing; +using Microsoft.Diagnostics.Tracing.Parsers; + +namespace Tracing.Tests.DiagnosticPortValidation +{ + public class DiagnosticPortValidation + { + private static readonly int s_NumberOfPorts = 4; + public static async Task TEST_MultipleConnectPortsNoSuspend() + { + bool fSuccess = true; + var serverAndNames = new List<(ReverseServer, string)>(); + string dotnetDiagnosticPorts = ""; + for (int i = 0; i < s_NumberOfPorts; i++) + { + string serverName = ReverseServer.MakeServerAddress(); + var server = new ReverseServer(serverName); + Logger.logger.Log($"Server {i} address is '{serverName}'"); + serverAndNames.Add((server, serverName)); + dotnetDiagnosticPorts += $"{serverName},connect,nosuspend;"; + } + Logger.logger.Log($"export DOTNET_DiagnosticPorts={dotnetDiagnosticPorts}"); + var advertisements = new List(); + Object sync = new Object(); + int subprocessId = -1; + Task subprocessTask = Utils.RunSubprocess( + currentAssembly: Assembly.GetExecutingAssembly(), + environment: new Dictionary + { + { Utils.DiagnosticPortsEnvKey, dotnetDiagnosticPorts } + }, + duringExecution: async (int pid) => + { + subprocessId = pid; + var tasks = new List(); + for (int i = 0; i < s_NumberOfPorts; i++) + { + var (server, _) = serverAndNames[i]; + int serverIndex = i; + tasks.Add(Task.Run(async () => + { + Stream stream = await server.AcceptAsync(); + IpcAdvertise advertise = IpcAdvertise.Parse(stream); + lock(sync) + advertisements.Add(advertise); + Logger.logger.Log($"Server {serverIndex} got advertise {advertise.ToString()}"); + })); + } + + await Task.WhenAll(tasks); + } + ); + + fSuccess &= await subprocessTask; + + foreach (var (server, _) in serverAndNames) + server.Shutdown(); + + Guid referenceCookie = advertisements[0].RuntimeInstanceCookie; + foreach (var adv in advertisements) + { + fSuccess &= (int)adv.ProcessId == subprocessId; + fSuccess &= adv.RuntimeInstanceCookie.Equals(referenceCookie); + } + + return fSuccess; + } + + public static async Task TEST_MultipleConnectPortsSuspend() + { + bool fSuccess = true; + var serverAndNames = new List<(ReverseServer, string)>(); + string dotnetDiagnosticPorts = ""; + for (int i = 0; i < s_NumberOfPorts; i++) + { + string serverName = ReverseServer.MakeServerAddress(); + var server = new ReverseServer(serverName); + Logger.logger.Log($"Server {i} address is '{serverName}'"); + serverAndNames.Add((server, serverName)); + dotnetDiagnosticPorts += $"{serverName},connect,suspend;"; + } + Logger.logger.Log($"export DOTNET_DiagnosticPorts={dotnetDiagnosticPorts}"); + + var advertisements = new List(); + Object sync = new Object(); + + int subprocessId = -1; + Task subprocessTask = Utils.RunSubprocess( + currentAssembly: Assembly.GetExecutingAssembly(), + environment: new Dictionary + { + { Utils.DiagnosticPortsEnvKey, dotnetDiagnosticPorts } + }, + duringExecution: async (int pid) => + { + subprocessId = pid; + bool hasResumed = false; + // Create an eventpipe session that will tell us when + // the EEStartupStarted event happens. This will tell us + // the the runtime has been resumed. This should only happen + // AFTER all suspend ports have sent the resume command. + var config = new SessionConfiguration( + circularBufferSizeMB: 1000, + format: EventPipeSerializationFormat.NetTrace, + providers: new List { + new Provider("Microsoft-Windows-DotNETRuntimePrivate", 0x80000000, EventLevel.Verbose) + }); + Logger.logger.Log("Starting EventPipeSession over standard connection"); + using Stream eventStream = EventPipeClient.CollectTracing(pid, config, out var sessionId); + Logger.logger.Log($"Started EventPipeSession over standard connection with session id: 0x{sessionId:x}"); + + Task readerTask = Task.Run(async () => + { + Logger.logger.Log($"Creating EventPipeEventSource"); + using var source = new EventPipeEventSource(eventStream); + var parser = new ClrPrivateTraceEventParser(source); + parser.StartupEEStartupStart += (eventData) => hasResumed = true; + Logger.logger.Log($"Created EventPipeEventSource"); + Logger.logger.Log($"Starting processing"); + await Task.Run(() => source.Process()); + Logger.logger.Log($"Finished processing"); + }); + + for (int i = 0; i < s_NumberOfPorts; i++) + { + fSuccess &= !hasResumed; + Logger.logger.Log($"Runtime is resumed (expects: false): {hasResumed}"); + var (server, _) = serverAndNames[i]; + int serverIndex = i; + Stream stream = await server.AcceptAsync(); + IpcAdvertise advertise = IpcAdvertise.Parse(stream); + lock(sync) + advertisements.Add(advertise); + Logger.logger.Log($"Server {serverIndex} got advertise {advertise.ToString()}"); + + // send resume command on this connection + var message = new IpcMessage(0x04,0x01); + Logger.logger.Log($"Port {serverIndex} sent: {message.ToString()}"); + IpcMessage response = IpcClient.SendMessage(stream, message); + Logger.logger.Log($"Port {serverIndex} received: {response.ToString()}"); + } + + Logger.logger.Log($"Stopping EventPipeSession"); + EventPipeClient.StopTracing(pid, sessionId); + await readerTask; + Logger.logger.Log($"Stopped EventPipeSession"); + + // runtime should have resumed now + fSuccess &= hasResumed; + Logger.logger.Log($"Runtime is resumed (expects: true): {hasResumed}"); + + } + ); + + + fSuccess &= await subprocessTask; + foreach (var (server, _) in serverAndNames) + server.Shutdown(); + + if (advertisements.Count() > 0) + { + Guid referenceCookie = advertisements[0].RuntimeInstanceCookie; + foreach (var adv in advertisements) + { + fSuccess &= (int)adv.ProcessId == subprocessId; + fSuccess &= adv.RuntimeInstanceCookie.Equals(referenceCookie); + } + } + else + { + fSuccess &= false; + } + + return fSuccess; + } + + public static async Task TEST_SuspendDefaultPort() + { + bool fSuccess = true; + + int subprocessId = -1; + Task subprocessTask = Utils.RunSubprocess( + currentAssembly: Assembly.GetExecutingAssembly(), + environment: new Dictionary + { + { Utils.DiagnosticPortSuspend, "1" } + }, + duringExecution: async (int pid) => + { + subprocessId = pid; + bool hasResumed = false; + // Create an eventpipe session that will tell us when + // the EEStartupStarted event happens. This will tell us + // the the runtime has been resumed. This should only happen + // AFTER all suspend ports have sent the resume command. + var config = new SessionConfiguration( + circularBufferSizeMB: 1000, + format: EventPipeSerializationFormat.NetTrace, + providers: new List { + new Provider("Microsoft-Windows-DotNETRuntimePrivate", 0x80000000, EventLevel.Verbose) + }); + Logger.logger.Log("Starting EventPipeSession over standard connection"); + using Stream eventStream = EventPipeClient.CollectTracing(pid, config, out var sessionId); + Logger.logger.Log($"Started EventPipeSession over standard connection with session id: 0x{sessionId:x}"); + + Task readerTask = Task.Run(async () => + { + Logger.logger.Log($"Creating EventPipeEventSource"); + using var source = new EventPipeEventSource(eventStream); + var parser = new ClrPrivateTraceEventParser(source); + parser.StartupEEStartupStart += (eventData) => hasResumed = true; + Logger.logger.Log($"Created EventPipeEventSource"); + Logger.logger.Log($"Starting processing"); + await Task.Run(() => source.Process()); + Logger.logger.Log($"Finished processing"); + }); + + + fSuccess &= !hasResumed; + Logger.logger.Log($"Runtime is resumed (expects: false): {hasResumed}"); + + // send resume command on this connection + var message = new IpcMessage(0x04,0x01); + Logger.logger.Log($"Sent: {message.ToString()}"); + IpcMessage response = IpcClient.SendMessage(ConnectionHelper.GetStandardTransport(pid), message); + Logger.logger.Log($"Received: {response.ToString()}"); + + Logger.logger.Log($"Stopping EventPipeSession"); + EventPipeClient.StopTracing(pid, sessionId); + await readerTask; + Logger.logger.Log($"Stopped EventPipeSession"); + + // runtime should have resumed now + fSuccess &= hasResumed; + Logger.logger.Log($"Runtime is resumed (expects: true): {hasResumed}"); + + } + ); + + + fSuccess &= await subprocessTask; + + return fSuccess; + } + + public static async Task Main(string[] args) + { + if (args.Length >= 1) + { + Console.Out.WriteLine("Subprocess started! Waiting for input..."); + var input = Console.In.ReadLine(); // will block until data is sent across stdin + Console.Out.WriteLine($"Received '{input}'. Exiting..."); + return 0; + } + + bool fSuccess = true; + if (!IpcTraceTest.EnsureCleanEnvironment()) + return -1; + IEnumerable tests = typeof(DiagnosticPortValidation).GetMethods().Where(mi => mi.Name.StartsWith("TEST_")); + foreach (var test in tests) + { + Logger.logger.Log($"::== Running test: {test.Name}"); + bool result = true; + try + { + result = await (Task)test.Invoke(null, new object[] {}); + } + catch (Exception e) + { + result = false; + Logger.logger.Log(e.ToString()); + } + fSuccess &= result; + Logger.logger.Log($"Test passed: {result}"); + Logger.logger.Log($""); + + } + return fSuccess ? 100 : -1; + } + } +} diff --git a/src/tests/tracing/eventpipe/diagnosticport/diagnosticport.csproj b/src/tests/tracing/eventpipe/diagnosticport/diagnosticport.csproj new file mode 100644 index 00000000000000..cec9486b658b50 --- /dev/null +++ b/src/tests/tracing/eventpipe/diagnosticport/diagnosticport.csproj @@ -0,0 +1,17 @@ + + + .NETCoreApp + exe + BuildAndRun + true + 0 + true + true + + true + + + + + + From e3cb293e276146d3a298da5fe608092009a73bc0 Mon Sep 17 00:00:00 2001 From: John Salem Date: Fri, 7 Aug 2020 13:41:56 -0700 Subject: [PATCH 07/15] Attempting to appease GCC --- src/coreclr/src/vm/diagnosticserver.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/coreclr/src/vm/diagnosticserver.cpp b/src/coreclr/src/vm/diagnosticserver.cpp index 5ad6ef9b6d1a0d..10aad1b1d3893b 100644 --- a/src/coreclr/src/vm/diagnosticserver.cpp +++ b/src/coreclr/src/vm/diagnosticserver.cpp @@ -260,8 +260,9 @@ void DiagnosticServer::PauseForDiagnosticsMonitor() if (dwFiveSecondWait == WAIT_TIMEOUT) { CLRConfigStringHolder dotnetDiagnosticPortString = CLRConfig::GetConfigValue(CLRConfig::EXTERNAL_DOTNET_DiagnosticPorts); + WCHAR empty[] = W(""); wprintf(W("The runtime has been configured to pause during startup and is awaiting a Diagnostics IPC ResumeStartup command from a server in the following list:\n")); - wprintf(W("DOTNET_DiagnosticPorts=\"%s\"\n"), dotnetDiagnosticPortString == nullptr ? W("") : dotnetDiagnosticPortString); + wprintf(W("DOTNET_DiagnosticPorts=\"%s\"\n"), dotnetDiagnosticPortString == nullptr ? empty : dotnetDiagnosticPortString.GetValue()); fflush(stdout); STRESS_LOG0(LF_DIAGNOSTICS_PORT, LL_ALWAYS, "The runtime has been configured to pause during startup and is awaiting a Diagnostics IPC ResumeStartup command and has waited 5 seconds."); const DWORD dwWait = s_ResumeRuntimeStartupEvent->Wait(INFINITE, false); From 335f9591f34996e6bb185ee059c7c7445661cc51 Mon Sep 17 00:00:00 2001 From: John Salem Date: Sat, 8 Aug 2020 11:34:02 -0700 Subject: [PATCH 08/15] Defer disable till after streaming starts * detect the case where a disable comes in immediately following a ResumeRuntime. We need to defer the disable to happen AFTER streaming starts since there is locking that happens in both FinishInitialization and Disable --- src/coreclr/src/vm/eventpipe.cpp | 58 ++++++++++++++++++++++++++------ src/coreclr/src/vm/eventpipe.h | 5 ++- 2 files changed, 51 insertions(+), 12 deletions(-) diff --git a/src/coreclr/src/vm/eventpipe.cpp b/src/coreclr/src/vm/eventpipe.cpp index e0b835f40d79b0..51264b1f351c5e 100644 --- a/src/coreclr/src/vm/eventpipe.cpp +++ b/src/coreclr/src/vm/eventpipe.cpp @@ -17,6 +17,7 @@ #include "eventpipesession.h" #include "eventpipejsonfile.h" #include "eventtracebase.h" +#include "ipcstreamfactory.h" #include "sampleprofiler.h" #include "win32threadpool.h" #include "ceemain.h" @@ -38,7 +39,8 @@ Volatile EventPipe::s_allowWrite = 0; unsigned int * EventPipe::s_pProcGroupOffsets = nullptr; #endif Volatile EventPipe::s_numberOfSessions(0); -CQuickArrayList EventPipe::s_rgDeferredEventPipeSessionIds = CQuickArrayList(); +CQuickArrayList EventPipe::s_rgDeferredEnableEventPipeSessionIds = CQuickArrayList(); +CQuickArrayList EventPipe::s_rgDeferredDisableEventPipeSessionIds = CQuickArrayList(); bool EventPipe::s_CanStartThreads = false; // This function is auto-generated from /src/scripts/genEventPipe.py @@ -111,21 +113,34 @@ void EventPipe::FinishInitialize() { STANDARD_VM_CONTRACT; - CrstHolder _crst(GetLock()); + // Enable streaming for any deferred sessions + { + CrstHolder _crst(GetLock()); - s_CanStartThreads = true; + s_CanStartThreads = true; - while (s_rgDeferredEventPipeSessionIds.Size() > 0) - { - EventPipeSessionID id = s_rgDeferredEventPipeSessionIds.Pop(); - if (IsSessionIdInCollection(id)) + while (s_rgDeferredEnableEventPipeSessionIds.Size() > 0) { - EventPipeSession *pSession = reinterpret_cast(id); - pSession->StartStreaming(); + EventPipeSessionID id = s_rgDeferredEnableEventPipeSessionIds.Pop(); + if (IsSessionIdInCollection(id)) + { + EventPipeSession *pSession = reinterpret_cast(id); + pSession->StartStreaming(); + } } + + SampleProfiler::CanStartSampling(); } - SampleProfiler::CanStartSampling(); + // release lock in case someone tried to disable while we held it + // s_rgDeferredDisableEventPipeSessionIds is now safe to access without the + // lock since we've set s_canStartThreads to true inside the lock. Anyone + // who was waiting on that lock will see that state and not mutate the defer list + while (s_rgDeferredDisableEventPipeSessionIds.Size() > 0) + { + EventPipeSessionID id = s_rgDeferredDisableEventPipeSessionIds.Pop(); + DisableHelper(id); + } } // @@ -420,7 +435,7 @@ void EventPipe::StartStreaming(EventPipeSessionID id) } else { - s_rgDeferredEventPipeSessionIds.Push(id); + s_rgDeferredEnableEventPipeSessionIds.Push(id); } } @@ -435,6 +450,27 @@ void EventPipe::Disable(EventPipeSessionID id) } CONTRACTL_END; + // EventPipe::Disable is called synchronously since the diagnostics server is + // single threaded. HOWEVER, if the runtime was suspended in EEStartupHelper, + // then EventPipe::FinishInitialize might not have executed yet. Disabling a session + // needs to either happen before we resume or after initialization. We briefly take the + // lock to check s_CanStartThreads to check whether we've finished initialization. We + // also check whether we are still suspended in which case we can safely disable the session + // without deferral. + { + CrstHolder _crst(GetLock()); + if (!s_CanStartThreads && !IpcStreamFactory::AnySuspendedPorts()) + { + s_rgDeferredDisableEventPipeSessionIds.Push(id); + return; + } + } + + DisableHelper(id); +} + +void EventPipe::DisableHelper(EventPipeSessionID id) +{ if (s_CanStartThreads) SetupThread(); diff --git a/src/coreclr/src/vm/eventpipe.h b/src/coreclr/src/vm/eventpipe.h index c03df4d3ed9fc4..52e7061d0223c5 100644 --- a/src/coreclr/src/vm/eventpipe.h +++ b/src/coreclr/src/vm/eventpipe.h @@ -179,6 +179,8 @@ class EventPipe Thread *pEventThread = nullptr, StackContents *pStack = nullptr); + static void DisableHelper(EventPipeSessionID id); + static void DisableInternal(EventPipeSessionID id, EventPipeProviderCallbackDataQueue* pEventPipeProviderCallbackDataQueue); // Enable the specified EventPipe session. @@ -239,7 +241,8 @@ class EventPipe static bool s_CanStartThreads; - static CQuickArrayList s_rgDeferredEventPipeSessionIds; + static CQuickArrayList s_rgDeferredEnableEventPipeSessionIds; + static CQuickArrayList s_rgDeferredDisableEventPipeSessionIds; //! Bitmask tracking EventPipe active sessions. // in all groups preceding it. For example if there are three groups with sizes: From 693d91a816461c6c3a608c613207e69d64854d12 Mon Sep 17 00:00:00 2001 From: John Salem Date: Tue, 11 Aug 2020 14:41:48 -0700 Subject: [PATCH 09/15] Rename ConnectionMode enum values --- .../debug/debug-pal/unix/diagnosticsipc.cpp | 16 ++++++++-------- .../src/debug/debug-pal/win/diagnosticsipc.cpp | 18 +++++++++--------- src/coreclr/src/debug/inc/diagnosticsipc.h | 12 ++++++------ src/coreclr/src/vm/ipcstreamfactory.cpp | 4 ++-- 4 files changed, 25 insertions(+), 25 deletions(-) diff --git a/src/coreclr/src/debug/debug-pal/unix/diagnosticsipc.cpp b/src/coreclr/src/debug/debug-pal/unix/diagnosticsipc.cpp index 632ac03a44c202..878a30166e702f 100644 --- a/src/coreclr/src/debug/debug-pal/unix/diagnosticsipc.cpp +++ b/src/coreclr/src/debug/debug-pal/unix/diagnosticsipc.cpp @@ -61,8 +61,8 @@ IpcStream::DiagnosticsIpc *IpcStream::DiagnosticsIpc::Create(const char *const p "socket"); } - if (mode == ConnectionMode::CLIENT) - return new IpcStream::DiagnosticsIpc(-1, &serverAddress, ConnectionMode::CLIENT); + if (mode == ConnectionMode::CONNECT) + return new IpcStream::DiagnosticsIpc(-1, &serverAddress, ConnectionMode::CONNECT); #ifdef __APPLE__ mode_t prev_mask = umask(~(S_IRUSR | S_IWUSR)); // This will set the default permission bit to 600 @@ -116,8 +116,8 @@ IpcStream::DiagnosticsIpc *IpcStream::DiagnosticsIpc::Create(const char *const p bool IpcStream::DiagnosticsIpc::Listen(ErrorCallback callback) { - _ASSERTE(mode == ConnectionMode::SERVER); - if (mode != ConnectionMode::SERVER) + _ASSERTE(mode == ConnectionMode::LISTEN); + if (mode != ConnectionMode::LISTEN) { if (callback != nullptr) callback("Cannot call Listen on a client connection", -1); @@ -150,7 +150,7 @@ bool IpcStream::DiagnosticsIpc::Listen(ErrorCallback callback) IpcStream *IpcStream::DiagnosticsIpc::Accept(ErrorCallback callback) { - _ASSERTE(mode == ConnectionMode::SERVER); + _ASSERTE(mode == ConnectionMode::LISTEN); _ASSERTE(_isListening); sockaddr_un from; @@ -168,7 +168,7 @@ IpcStream *IpcStream::DiagnosticsIpc::Accept(ErrorCallback callback) IpcStream *IpcStream::DiagnosticsIpc::Connect(ErrorCallback callback) { - _ASSERTE(mode == ConnectionMode::CLIENT); + _ASSERTE(mode == ConnectionMode::CONNECT); sockaddr_un clientAddress{}; clientAddress.sun_family = AF_UNIX; @@ -194,7 +194,7 @@ IpcStream *IpcStream::DiagnosticsIpc::Connect(ErrorCallback callback) return nullptr; } - return new IpcStream(clientSocket, ConnectionMode::CLIENT); + return new IpcStream(clientSocket, ConnectionMode::CONNECT); } int32_t IpcStream::DiagnosticsIpc::Poll(IpcPollHandle *rgIpcPollHandles, uint32_t nHandles, int32_t timeoutMs, ErrorCallback callback) @@ -208,7 +208,7 @@ int32_t IpcStream::DiagnosticsIpc::Poll(IpcPollHandle *rgIpcPollHandles, uint32_ if (rgIpcPollHandles[i].pIpc != nullptr) { // SERVER - _ASSERTE(rgIpcPollHandles[i].pIpc->mode == ConnectionMode::SERVER); + _ASSERTE(rgIpcPollHandles[i].pIpc->mode == ConnectionMode::LISTEN); fd = rgIpcPollHandles[i].pIpc->_serverSocket; } else diff --git a/src/coreclr/src/debug/debug-pal/win/diagnosticsipc.cpp b/src/coreclr/src/debug/debug-pal/win/diagnosticsipc.cpp index 2b6c38463c1bf2..3d3d3f03f6bf11 100644 --- a/src/coreclr/src/debug/debug-pal/win/diagnosticsipc.cpp +++ b/src/coreclr/src/debug/debug-pal/win/diagnosticsipc.cpp @@ -56,8 +56,8 @@ IpcStream::DiagnosticsIpc *IpcStream::DiagnosticsIpc::Create(const char *const p bool IpcStream::DiagnosticsIpc::Listen(ErrorCallback callback) { - _ASSERTE(mode == ConnectionMode::SERVER); - if (mode != ConnectionMode::SERVER) + _ASSERTE(mode == ConnectionMode::LISTEN); + if (mode != ConnectionMode::LISTEN) { if (callback != nullptr) callback("Cannot call Listen on a client connection", -1); @@ -131,7 +131,7 @@ bool IpcStream::DiagnosticsIpc::Listen(ErrorCallback callback) IpcStream *IpcStream::DiagnosticsIpc::Accept(ErrorCallback callback) { _ASSERTE(_isListening); - _ASSERTE(mode == ConnectionMode::SERVER); + _ASSERTE(mode == ConnectionMode::LISTEN); DWORD dwDummy = 0; bool fSuccess = GetOverlappedResult( @@ -148,7 +148,7 @@ IpcStream *IpcStream::DiagnosticsIpc::Accept(ErrorCallback callback) } // create new IpcStream using handle and reset the Server object so it can listen again - IpcStream *pStream = new IpcStream(_hPipe, ConnectionMode::SERVER); + IpcStream *pStream = new IpcStream(_hPipe, ConnectionMode::LISTEN); // reset the server _hPipe = INVALID_HANDLE_VALUE; @@ -167,8 +167,8 @@ IpcStream *IpcStream::DiagnosticsIpc::Accept(ErrorCallback callback) IpcStream *IpcStream::DiagnosticsIpc::Connect(ErrorCallback callback) { - _ASSERTE(mode == ConnectionMode::CLIENT); - if (mode != ConnectionMode::CLIENT) + _ASSERTE(mode == ConnectionMode::CONNECT); + if (mode != ConnectionMode::CONNECT) { if (callback != nullptr) callback("Cannot call connect on a server connection", 0); @@ -206,7 +206,7 @@ void IpcStream::DiagnosticsIpc::Close(bool isShutdown, ErrorCallback callback) if (_hPipe != INVALID_HANDLE_VALUE) { - if (mode == DiagnosticsIpc::ConnectionMode::SERVER) + if (mode == DiagnosticsIpc::ConnectionMode::LISTEN) { const BOOL fSuccessDisconnectNamedPipe = ::DisconnectNamedPipe(_hPipe); _ASSERTE(fSuccessDisconnectNamedPipe != 0); @@ -248,7 +248,7 @@ void IpcStream::Close(ErrorCallback callback) { Flush(); - if (_mode == DiagnosticsIpc::ConnectionMode::SERVER) + if (_mode == DiagnosticsIpc::ConnectionMode::LISTEN) { const BOOL fSuccessDisconnectNamedPipe = ::DisconnectNamedPipe(_hPipe); _ASSERTE(fSuccessDisconnectNamedPipe != 0); @@ -281,7 +281,7 @@ int32_t IpcStream::DiagnosticsIpc::Poll(IpcPollHandle *rgIpcPollHandles, uint32_ if (rgIpcPollHandles[i].pIpc != nullptr) { // SERVER - _ASSERTE(rgIpcPollHandles[i].pIpc->mode == DiagnosticsIpc::ConnectionMode::SERVER); + _ASSERTE(rgIpcPollHandles[i].pIpc->mode == DiagnosticsIpc::ConnectionMode::LISTEN); pHandles[i] = rgIpcPollHandles[i].pIpc->_oOverlap.hEvent; } else diff --git a/src/coreclr/src/debug/inc/diagnosticsipc.h b/src/coreclr/src/debug/inc/diagnosticsipc.h index 99d670ca6ca552..e273640c191324 100644 --- a/src/coreclr/src/debug/inc/diagnosticsipc.h +++ b/src/coreclr/src/debug/inc/diagnosticsipc.h @@ -29,8 +29,8 @@ class IpcStream final public: enum ConnectionMode { - CLIENT, - SERVER + CONNECT, + LISTEN }; enum class PollEvents : uint8_t @@ -99,7 +99,7 @@ class IpcStream final sockaddr_un *const _pServerAddress; bool _isClosed; - DiagnosticsIpc(const int serverSocket, sockaddr_un *const pServerAddress, ConnectionMode mode = ConnectionMode::SERVER); + DiagnosticsIpc(const int serverSocket, sockaddr_un *const pServerAddress, ConnectionMode mode = ConnectionMode::LISTEN); // Used to unlink the socket so it can be removed from the filesystem // when the last reference to it is closed. @@ -110,7 +110,7 @@ class IpcStream final HANDLE _hPipe = INVALID_HANDLE_VALUE; OVERLAPPED _oOverlap = {}; - DiagnosticsIpc(const char(&namedPipeName)[MaxNamedPipeNameLength], ConnectionMode mode = ConnectionMode::SERVER); + DiagnosticsIpc(const char(&namedPipeName)[MaxNamedPipeNameLength], ConnectionMode mode = ConnectionMode::LISTEN); #endif /* TARGET_UNIX */ bool _isListening; @@ -125,13 +125,13 @@ class IpcStream final private: #ifdef TARGET_UNIX int _clientSocket = -1; - IpcStream(int clientSocket, int serverSocket, DiagnosticsIpc::ConnectionMode mode = DiagnosticsIpc::ConnectionMode::SERVER) + IpcStream(int clientSocket, int serverSocket, DiagnosticsIpc::ConnectionMode mode = DiagnosticsIpc::ConnectionMode::LISTEN) : _clientSocket(clientSocket), _mode(mode) {} #else HANDLE _hPipe = INVALID_HANDLE_VALUE; OVERLAPPED _oOverlap = {}; BOOL _isTestReading = false; // used to check whether we are already doing a 0-byte read to test for data - IpcStream(HANDLE hPipe, DiagnosticsIpc::ConnectionMode mode = DiagnosticsIpc::ConnectionMode::SERVER); + IpcStream(HANDLE hPipe, DiagnosticsIpc::ConnectionMode mode = DiagnosticsIpc::ConnectionMode::LISTEN); #endif /* TARGET_UNIX */ DiagnosticsIpc::ConnectionMode _mode; diff --git a/src/coreclr/src/vm/ipcstreamfactory.cpp b/src/coreclr/src/vm/ipcstreamfactory.cpp index 75b7fb3a0c4117..98d4815594eaff 100644 --- a/src/coreclr/src/vm/ipcstreamfactory.cpp +++ b/src/coreclr/src/vm/ipcstreamfactory.cpp @@ -141,7 +141,7 @@ bool IpcStreamFactory::BuildAndAddPort(IpcStreamFactory::DiagnosticPortBuilder b { if (builder.Type == DiagnosticPortType::LISTEN) { - IpcStream::DiagnosticsIpc *pIpc = IpcStream::DiagnosticsIpc::Create(builder.Path, IpcStream::DiagnosticsIpc::ConnectionMode::SERVER, callback); + IpcStream::DiagnosticsIpc *pIpc = IpcStream::DiagnosticsIpc::Create(builder.Path, IpcStream::DiagnosticsIpc::ConnectionMode::LISTEN, callback); if (pIpc != nullptr) { if (pIpc->Listen(callback)) @@ -162,7 +162,7 @@ bool IpcStreamFactory::BuildAndAddPort(IpcStreamFactory::DiagnosticPortBuilder b } else if (builder.Type == DiagnosticPortType::CONNECT) { - IpcStream::DiagnosticsIpc *pIpc = IpcStream::DiagnosticsIpc::Create(builder.Path, IpcStream::DiagnosticsIpc::ConnectionMode::CLIENT, callback); + IpcStream::DiagnosticsIpc *pIpc = IpcStream::DiagnosticsIpc::Create(builder.Path, IpcStream::DiagnosticsIpc::ConnectionMode::CONNECT, callback); if (pIpc != nullptr) { s_rgpDiagnosticPorts.Push(new ConnectDiagnosticPort(pIpc, builder)); From c3e5b84f7674d14018caa89546b79edc0a58169c Mon Sep 17 00:00:00 2001 From: John Salem Date: Tue, 11 Aug 2020 14:48:57 -0700 Subject: [PATCH 10/15] PR feedback --- src/coreclr/src/vm/diagnosticserver.cpp | 2 +- src/coreclr/src/vm/ipcstreamfactory.cpp | 9 +++------ 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/src/coreclr/src/vm/diagnosticserver.cpp b/src/coreclr/src/vm/diagnosticserver.cpp index 10aad1b1d3893b..b807e74a913a1a 100644 --- a/src/coreclr/src/vm/diagnosticserver.cpp +++ b/src/coreclr/src/vm/diagnosticserver.cpp @@ -145,7 +145,7 @@ bool DiagnosticServer::Initialize() // Ports can fail to be configured bool fAnyErrors = IpcStreamFactory::Configure(ErrorCallback); if (fAnyErrors) - STRESS_LOG0(LF_DIAGNOSTICS_PORT, LL_WARNING, "At least one Diagnostic Port fails to be configured.\n"); + STRESS_LOG0(LF_DIAGNOSTICS_PORT, LL_ERROR, "At least one Diagnostic Port failed to be configured.\n"); if (IpcStreamFactory::AnySuspendedPorts()) { diff --git a/src/coreclr/src/vm/ipcstreamfactory.cpp b/src/coreclr/src/vm/ipcstreamfactory.cpp index 98d4815594eaff..50aa3431d08531 100644 --- a/src/coreclr/src/vm/ipcstreamfactory.cpp +++ b/src/coreclr/src/vm/ipcstreamfactory.cpp @@ -93,12 +93,9 @@ bool IpcStreamFactory::Configure(ErrorCallback callback) if (dotnetDiagnosticPortsW != nullptr) { nCharactersWritten = WideCharToMultiByte(CP_UTF8, 0, dotnetDiagnosticPortsW, -1, NULL, 0, NULL, NULL); - if (nCharactersWritten != 0) - { - dotnetDiagnosticPorts = new char[nCharactersWritten]; - nCharactersWritten = WideCharToMultiByte(CP_UTF8, 0, dotnetDiagnosticPortsW, -1, dotnetDiagnosticPorts, nCharactersWritten, NULL, NULL); - assert(nCharactersWritten != 0); - } + dotnetDiagnosticPorts = new char[nCharactersWritten]; + nCharactersWritten = WideCharToMultiByte(CP_UTF8, 0, dotnetDiagnosticPortsW, -1, dotnetDiagnosticPorts, nCharactersWritten, NULL, NULL); + ASSERT(nCharactersWritten != 0); CQuickArrayList portConfigs = split(dotnetDiagnosticPorts, ";"); while (portConfigs.Size() > 0) From bb70ca762cfdbe6dedaa2f383c2127f6e0e7c3ad Mon Sep 17 00:00:00 2001 From: John Salem Date: Wed, 12 Aug 2020 12:05:08 -0700 Subject: [PATCH 11/15] Fix #40696 and add test to cover that --- src/coreclr/src/vm/diagnosticserver.cpp | 3 ++ src/coreclr/src/vm/diagnosticsprotocol.h | 4 +- .../tracing/eventpipe/common/IpcUtils.cs | 43 +++++++++++++++++++ .../diagnosticport/diagnosticport.cs | 34 +++++++++++++++ 4 files changed, 82 insertions(+), 2 deletions(-) diff --git a/src/coreclr/src/vm/diagnosticserver.cpp b/src/coreclr/src/vm/diagnosticserver.cpp index b807e74a913a1a..30eae789c7e3d3 100644 --- a/src/coreclr/src/vm/diagnosticserver.cpp +++ b/src/coreclr/src/vm/diagnosticserver.cpp @@ -142,6 +142,9 @@ bool DiagnosticServer::Initialize() szMessage); // data2 }; + // Initialize the RuntimeIndentifier before use + DiagnosticsIpc::GetAdvertiseCookie_V1(); + // Ports can fail to be configured bool fAnyErrors = IpcStreamFactory::Configure(ErrorCallback); if (fAnyErrors) diff --git a/src/coreclr/src/vm/diagnosticsprotocol.h b/src/coreclr/src/vm/diagnosticsprotocol.h index a31c698723a0f8..9f0c918caf7368 100644 --- a/src/coreclr/src/vm/diagnosticsprotocol.h +++ b/src/coreclr/src/vm/diagnosticsprotocol.h @@ -143,8 +143,8 @@ namespace DiagnosticsIpc uint64_t *buffer = (uint64_t*)advertiseBuffer; buffer[0] = *(uint64_t*)AdvertiseMagic_V1; - buffer[1] = (((uint64_t)VAL32(cookie.Data1) << 32) | ((uint64_t)VAL16(cookie.Data2) << 16) | VAL16((uint64_t)cookie.Data3)); - buffer[2] = *(uint64_t*)cookie.Data4; + // fills buffer[1] and buffer[2] + memcpy(&buffer[1], &cookie, sizeof(cookie)); buffer[3] = VAL64(pid); // zero out unused field diff --git a/src/tests/tracing/eventpipe/common/IpcUtils.cs b/src/tests/tracing/eventpipe/common/IpcUtils.cs index 8118ff16b0f26f..b845a8c231ac9a 100644 --- a/src/tests/tracing/eventpipe/common/IpcUtils.cs +++ b/src/tests/tracing/eventpipe/common/IpcUtils.cs @@ -293,6 +293,49 @@ override public string ToString() } } + public class ProcessInfo + { + // uint64_t ProcessId; + // GUID RuntimeCookie; + // LPCWSTR CommandLine; + // LPCWSTR OS; + // LPCWSTR Arch; + public UInt64 ProcessId; + public Guid RuntimeCookie; + public string Commandline; + public string OS; + public string Arch; + + public static ProcessInfo TryParse(byte[] buf) + { + var info = new ProcessInfo(); + int start = 0; + int end = 8; /* sizeof(uint64_t) */ + info.ProcessId = BitConverter.ToUInt64(buf[start..end]); + + start = end; + end = start + 16; /* sizeof(guid) */ + info.RuntimeCookie = new Guid(buf[start..end]); + + string ParseString(ref int start, ref int end) + { + start = end; + end = start + 4; /* sizeof(uint32_t) */ + uint nChars = BitConverter.ToUInt32(buf[start..end]); + + start = end; + end = start + ((int)nChars * sizeof(char)); + return System.Text.Encoding.Unicode.GetString(buf[start..end]).TrimEnd('\0'); + } + + info.Commandline = ParseString(ref start, ref end); + info.OS = ParseString(ref start, ref end); + info.Arch = ParseString(ref start, ref end); + + return info; + } + } + public class IpcClient { public static IpcMessage SendMessage(Stream stream, IpcMessage message) diff --git a/src/tests/tracing/eventpipe/diagnosticport/diagnosticport.cs b/src/tests/tracing/eventpipe/diagnosticport/diagnosticport.cs index 319b8cd6db4413..e873b3ea84bc71 100644 --- a/src/tests/tracing/eventpipe/diagnosticport/diagnosticport.cs +++ b/src/tests/tracing/eventpipe/diagnosticport/diagnosticport.cs @@ -258,6 +258,40 @@ public static async Task TEST_SuspendDefaultPort() return fSuccess; } + public static async Task TEST_AdvertiseAndProcessInfoCookiesMatch() + { + bool fSuccess = true; + string serverName = ReverseServer.MakeServerAddress(); + Logger.logger.Log($"Server name is '{serverName}'"); + var server = new ReverseServer(serverName); + using var memoryStream = new MemoryStream(); + Task subprocessTask = Utils.RunSubprocess( + currentAssembly: Assembly.GetExecutingAssembly(), + environment: new Dictionary { { Utils.DiagnosticPortsEnvKey, $"{serverName},connect,nosuspend" } }, + duringExecution: async (pid) => + { + Stream stream = await server.AcceptAsync(); + IpcAdvertise advertise = IpcAdvertise.Parse(stream); + Logger.logger.Log(advertise.ToString()); + + Logger.logger.Log($"Send ProcessInfo Diagnostics IPC Command"); + // send ProcessInfo command (0x04=ProcessCommandSet, 0x00=ProcessInfo commandid) + var message = new IpcMessage(0x04,0x00); + Logger.logger.Log($"Sent: {message.ToString()}"); + IpcMessage response = IpcClient.SendMessage(stream, message); + Logger.logger.Log($"received: {response.ToString()}"); + ProcessInfo info = ProcessInfo.TryParse(response.Payload); + + Utils.Assert(info.RuntimeCookie.Equals(advertise.RuntimeInstanceCookie), $"The runtime cookie reported by ProcessInfo and Advertise must match. ProcessInfo: {info.RuntimeCookie.ToString()}, Advertise: {advertise.RuntimeInstanceCookie.ToString()}"); + Logger.logger.Log($"ProcessInfo and Advertise Cookies are equal"); + } + ); + + fSuccess &= await subprocessTask; + + return fSuccess; + } + public static async Task Main(string[] args) { if (args.Length >= 1) From 2c4f0de95ab69eceec722005cfe26df15d4ca376 Mon Sep 17 00:00:00 2001 From: John Salem Date: Wed, 12 Aug 2020 16:55:38 -0700 Subject: [PATCH 12/15] Fix scope of runtime cookie --- src/coreclr/src/vm/diagnosticserver.cpp | 3 ++- src/coreclr/src/vm/diagnosticsprotocol.h | 10 +++------- .../tracing/eventpipe/diagnosticport/diagnosticport.cs | 1 + 3 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/coreclr/src/vm/diagnosticserver.cpp b/src/coreclr/src/vm/diagnosticserver.cpp index 7847224bebd7e9..f703f1ddcce311 100644 --- a/src/coreclr/src/vm/diagnosticserver.cpp +++ b/src/coreclr/src/vm/diagnosticserver.cpp @@ -22,6 +22,7 @@ Volatile DiagnosticServer::s_shuttingDown(false); CLREventStatic *DiagnosticServer::s_ResumeRuntimeStartupEvent = nullptr; +GUID DiagnosticsIpc::AdvertiseCookie_V1 = GUID_NULL; DWORD WINAPI DiagnosticServer::DiagnosticsServerThread(LPVOID) { @@ -145,7 +146,7 @@ bool DiagnosticServer::Initialize() }; // Initialize the RuntimeIndentifier before use - DiagnosticsIpc::GetAdvertiseCookie_V1(); + CoCreateGuid(&DiagnosticsIpc::AdvertiseCookie_V1); // Ports can fail to be configured bool fAnyErrors = IpcStreamFactory::Configure(ErrorCallback); diff --git a/src/coreclr/src/vm/diagnosticsprotocol.h b/src/coreclr/src/vm/diagnosticsprotocol.h index 9f0c918caf7368..53e7413c04c8fc 100644 --- a/src/coreclr/src/vm/diagnosticsprotocol.h +++ b/src/coreclr/src/vm/diagnosticsprotocol.h @@ -112,7 +112,7 @@ namespace DiagnosticsIpc * * See spec in: dotnet/diagnostics@documentation/design-docs/ipc-spec.md * - * The flow for Advertise is a one-way burst of 24 bytes consisting of + * The flow for Advertise is a one-way burst of 34 bytes consisting of * 8 bytes - "ADVR_V1\0" (ASCII chars + null byte) * 16 bytes - random 128 bit number cookie (little-endian) * 8 bytes - PID (little-endian) @@ -123,15 +123,11 @@ namespace DiagnosticsIpc const uint32_t AdvertiseSize = 34; - static GUID AdvertiseCookie_V1 = GUID_NULL; + // initialized in DiagnosticServer::Initialize during EEStartupHelper + extern GUID AdvertiseCookie_V1; inline GUID GetAdvertiseCookie_V1() { - if (AdvertiseCookie_V1 == GUID_NULL) - { - CoCreateGuid(&AdvertiseCookie_V1); - } - return AdvertiseCookie_V1; } diff --git a/src/tests/tracing/eventpipe/diagnosticport/diagnosticport.cs b/src/tests/tracing/eventpipe/diagnosticport/diagnosticport.cs index e873b3ea84bc71..2f6f0d68f575ac 100644 --- a/src/tests/tracing/eventpipe/diagnosticport/diagnosticport.cs +++ b/src/tests/tracing/eventpipe/diagnosticport/diagnosticport.cs @@ -281,6 +281,7 @@ public static async Task TEST_AdvertiseAndProcessInfoCookiesMatch() IpcMessage response = IpcClient.SendMessage(stream, message); Logger.logger.Log($"received: {response.ToString()}"); ProcessInfo info = ProcessInfo.TryParse(response.Payload); + Logger.logger.Log($"ProcessInfo: {{ id={info.ProcessId}, cookie={info.RuntimeCookie}, cmdline={info.Commandline}, OS={info.OS}, arch={info.Arch} }}"); Utils.Assert(info.RuntimeCookie.Equals(advertise.RuntimeInstanceCookie), $"The runtime cookie reported by ProcessInfo and Advertise must match. ProcessInfo: {info.RuntimeCookie.ToString()}, Advertise: {advertise.RuntimeInstanceCookie.ToString()}"); Logger.logger.Log($"ProcessInfo and Advertise Cookies are equal"); From c5134d0260a1299242ffc04677d3fbb8047f4e0d Mon Sep 17 00:00:00 2001 From: John Salem Date: Wed, 12 Aug 2020 17:05:30 -0700 Subject: [PATCH 13/15] Ignore listen ports for now --- src/coreclr/src/vm/ipcstreamfactory.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/coreclr/src/vm/ipcstreamfactory.cpp b/src/coreclr/src/vm/ipcstreamfactory.cpp index ffb8a149b9f022..735110eeccae8f 100644 --- a/src/coreclr/src/vm/ipcstreamfactory.cpp +++ b/src/coreclr/src/vm/ipcstreamfactory.cpp @@ -124,6 +124,13 @@ bool IpcStreamFactory::Configure(ErrorCallback callback) builder.WithTag(portConfigParts.Pop()); builder.WithPath(portConfigParts.Pop()); + // Ignore listen type (see conversation in https://github.com/dotnet/runtime/pull/40499 for details) + if (builder.Type == DiagnosticPortType::LISTEN) + { + STRESS_LOG0(LF_DIAGNOSTICS_PORT, LL_INFO10, "IpcStreamFactory::Configure - Ignoring LISTEN port configuration \n"); + continue; + } + const bool fBuildSuccess = BuildAndAddPort(builder, callback); STRESS_LOG1(LF_DIAGNOSTICS_PORT, LL_INFO10, "IpcStreamFactory::Configure - Diagnostic Port creation succeeded? %d \n", fBuildSuccess); fSuccess &= fBuildSuccess; From bcf4506c4c8c54e33c27f116bff557047fdeab6d Mon Sep 17 00:00:00 2001 From: John Salem Date: Wed, 12 Aug 2020 17:06:21 -0700 Subject: [PATCH 14/15] default port type is connect --- src/coreclr/src/vm/ipcstreamfactory.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/src/vm/ipcstreamfactory.h b/src/coreclr/src/vm/ipcstreamfactory.h index cfd286beee0bba..7e5e07c0f45784 100644 --- a/src/coreclr/src/vm/ipcstreamfactory.h +++ b/src/coreclr/src/vm/ipcstreamfactory.h @@ -29,7 +29,7 @@ class IpcStreamFactory struct DiagnosticPortBuilder { LPSTR Path = nullptr; - DiagnosticPortType Type = DiagnosticPortType::LISTEN; + DiagnosticPortType Type = DiagnosticPortType::CONNECT; DiagnosticPortSuspendMode SuspendMode = DiagnosticPortSuspendMode::NOSUSPEND; DiagnosticPortBuilder WithPath(LPSTR path) { Path = path != nullptr ? _strdup(path) : nullptr; return *this; } From f97f77a335382b30ef4d7aa169107fa69e5ec823 Mon Sep 17 00:00:00 2001 From: John Salem Date: Thu, 13 Aug 2020 13:42:47 -0700 Subject: [PATCH 15/15] PR feedback * print default port's suspend status if waiting more than 5 sec * remove strdup * add test case for string parsing * update tests to use defaults --- src/coreclr/src/inc/clrconfigvalues.h | 4 +- src/coreclr/src/vm/diagnosticserver.cpp | 4 +- src/coreclr/src/vm/ipcstreamfactory.cpp | 3 +- src/coreclr/src/vm/ipcstreamfactory.h | 2 +- .../tracing/eventpipe/common/IpcUtils.cs | 2 +- .../diagnosticport/diagnosticport.cs | 112 +++++++++++++++--- .../eventpipe/pauseonstart/pauseonstart.cs | 10 +- .../tracing/eventpipe/reverse/reverse.cs | 12 +- .../eventpipe/reverseouter/reverseouter.cs | 2 +- 9 files changed, 117 insertions(+), 34 deletions(-) diff --git a/src/coreclr/src/inc/clrconfigvalues.h b/src/coreclr/src/inc/clrconfigvalues.h index f92167f0dfdb5b..efeac155253c9c 100644 --- a/src/coreclr/src/inc/clrconfigvalues.h +++ b/src/coreclr/src/inc/clrconfigvalues.h @@ -712,8 +712,8 @@ RETAIL_CONFIG_DWORD_INFO(INTERNAL_EventPipeProcNumbers, W("EventPipeProcNumbers" // // Diagnostics Ports // -RETAIL_CONFIG_DWORD_INFO_EX(EXTERNAL_DOTNET_DiagnosticPortSuspend, W("DOTNET_DiagnosticPortSuspend"), 0, "This will cause the runtime to pause during startup before major subsystems are started. Resume using the Diagnostics IPC ResumeStartup command.", CLRConfig::DontPrependCOMPlus_); -RETAIL_CONFIG_STRING_INFO_EX(EXTERNAL_DOTNET_DiagnosticPorts, W("DOTNET_DiagnosticPorts"), "A semicolon delimited list of additional Diagnostic Ports, where a Diagnostic Port is a NamedPipe path without '\\\\.\\pipe\\' on Windows or the full path of Unix Domain Socket on Linux/Unix followed by optional tags, e.g., ',listen,nosuspend;,connect'", CLRConfig::DontPrependCOMPlus_); +RETAIL_CONFIG_DWORD_INFO_EX(EXTERNAL_DOTNET_DefaultDiagnosticPortSuspend, W("DOTNET_DefaultDiagnosticPortSuspend"), 0, "This sets the deafult diagnostic port to suspend causing the runtime to pause during startup before major subsystems are started. Resume using the Diagnostics IPC ResumeStartup command on the default diagnostic port.", CLRConfig::DontPrependCOMPlus_); +RETAIL_CONFIG_STRING_INFO_EX(EXTERNAL_DOTNET_DiagnosticPorts, W("DOTNET_DiagnosticPorts"), "A semicolon delimited list of additional Diagnostic Ports, where a Diagnostic Port is a NamedPipe path without '\\\\.\\pipe\\' on Windows or the full path of Unix Domain Socket on Linux/Unix followed by optional tags, e.g., ',connect,nosuspend;'", CLRConfig::DontPrependCOMPlus_); // // LTTng diff --git a/src/coreclr/src/vm/diagnosticserver.cpp b/src/coreclr/src/vm/diagnosticserver.cpp index f703f1ddcce311..ef026a1f40533a 100644 --- a/src/coreclr/src/vm/diagnosticserver.cpp +++ b/src/coreclr/src/vm/diagnosticserver.cpp @@ -267,8 +267,10 @@ void DiagnosticServer::PauseForDiagnosticsMonitor() { CLRConfigStringHolder dotnetDiagnosticPortString = CLRConfig::GetConfigValue(CLRConfig::EXTERNAL_DOTNET_DiagnosticPorts); WCHAR empty[] = W(""); - wprintf(W("The runtime has been configured to pause during startup and is awaiting a Diagnostics IPC ResumeStartup command from a server in the following list:\n")); + DWORD dotnetDiagnosticPortSuspend = CLRConfig::GetConfigValue(CLRConfig::EXTERNAL_DOTNET_DefaultDiagnosticPortSuspend); + wprintf(W("The runtime has been configured to pause during startup and is awaiting a Diagnostics IPC ResumeStartup command from a Diagnostic Port.\n")); wprintf(W("DOTNET_DiagnosticPorts=\"%s\"\n"), dotnetDiagnosticPortString == nullptr ? empty : dotnetDiagnosticPortString.GetValue()); + wprintf(W("DOTNET_DefaultDiagnosticPortSuspend=%d\n"), dotnetDiagnosticPortSuspend); fflush(stdout); STRESS_LOG0(LF_DIAGNOSTICS_PORT, LL_ALWAYS, "The runtime has been configured to pause during startup and is awaiting a Diagnostics IPC ResumeStartup command and has waited 5 seconds."); const DWORD dwWait = s_ResumeRuntimeStartupEvent->Wait(INFINITE, false); diff --git a/src/coreclr/src/vm/ipcstreamfactory.cpp b/src/coreclr/src/vm/ipcstreamfactory.cpp index 735110eeccae8f..fc880f3e36ae0b 100644 --- a/src/coreclr/src/vm/ipcstreamfactory.cpp +++ b/src/coreclr/src/vm/ipcstreamfactory.cpp @@ -113,7 +113,6 @@ bool IpcStreamFactory::Configure(ErrorCallback callback) CQuickArrayList portConfigParts = split(portConfig, ","); DiagnosticPortBuilder builder; - ASSERT(portConfigParts.Size() >= 1); if (portConfigParts.Size() == 0) { fSuccess &= false; @@ -138,7 +137,7 @@ bool IpcStreamFactory::Configure(ErrorCallback callback) } // create the default listen port - DWORD dotnetDiagnosticPortSuspend = CLRConfig::GetConfigValue(CLRConfig::EXTERNAL_DOTNET_DiagnosticPortSuspend); + DWORD dotnetDiagnosticPortSuspend = CLRConfig::GetConfigValue(CLRConfig::EXTERNAL_DOTNET_DefaultDiagnosticPortSuspend); DiagnosticPortBuilder defaultListenPortBuilder = DiagnosticPortBuilder{} .WithPath(nullptr) .WithSuspendMode(dotnetDiagnosticPortSuspend > 0 ? DiagnosticPortSuspendMode::SUSPEND : DiagnosticPortSuspendMode::NOSUSPEND) diff --git a/src/coreclr/src/vm/ipcstreamfactory.h b/src/coreclr/src/vm/ipcstreamfactory.h index 7e5e07c0f45784..8eec10faafca0a 100644 --- a/src/coreclr/src/vm/ipcstreamfactory.h +++ b/src/coreclr/src/vm/ipcstreamfactory.h @@ -32,7 +32,7 @@ class IpcStreamFactory DiagnosticPortType Type = DiagnosticPortType::CONNECT; DiagnosticPortSuspendMode SuspendMode = DiagnosticPortSuspendMode::NOSUSPEND; - DiagnosticPortBuilder WithPath(LPSTR path) { Path = path != nullptr ? _strdup(path) : nullptr; return *this; } + DiagnosticPortBuilder WithPath(LPSTR path) { Path = path; return *this; } DiagnosticPortBuilder WithType(DiagnosticPortType type) { Type = type; return *this; } DiagnosticPortBuilder WithSuspendMode(DiagnosticPortSuspendMode mode) { SuspendMode = mode; return *this; } DiagnosticPortBuilder WithTag(LPSTR tag) diff --git a/src/tests/tracing/eventpipe/common/IpcUtils.cs b/src/tests/tracing/eventpipe/common/IpcUtils.cs index 223e88232f7ccd..85e8430229292e 100644 --- a/src/tests/tracing/eventpipe/common/IpcUtils.cs +++ b/src/tests/tracing/eventpipe/common/IpcUtils.cs @@ -22,7 +22,7 @@ namespace Tracing.Tests.Common public static class Utils { public static readonly string DiagnosticPortsEnvKey = "DOTNET_DiagnosticPorts"; - public static readonly string DiagnosticPortSuspend = "DOTNET_DiagnosticPortSuspend"; + public static readonly string DiagnosticPortSuspend = "DOTNET_DefaultDiagnosticPortSuspend"; public static async Task WaitTillTimeout(Task task, TimeSpan timeout) { diff --git a/src/tests/tracing/eventpipe/diagnosticport/diagnosticport.cs b/src/tests/tracing/eventpipe/diagnosticport/diagnosticport.cs index 2f6f0d68f575ac..9d3280b37377db 100644 --- a/src/tests/tracing/eventpipe/diagnosticport/diagnosticport.cs +++ b/src/tests/tracing/eventpipe/diagnosticport/diagnosticport.cs @@ -32,7 +32,7 @@ public static async Task TEST_MultipleConnectPortsNoSuspend() var server = new ReverseServer(serverName); Logger.logger.Log($"Server {i} address is '{serverName}'"); serverAndNames.Add((server, serverName)); - dotnetDiagnosticPorts += $"{serverName},connect,nosuspend;"; + dotnetDiagnosticPorts += $"{serverName};"; } Logger.logger.Log($"export DOTNET_DiagnosticPorts={dotnetDiagnosticPorts}"); var advertisements = new List(); @@ -92,7 +92,7 @@ public static async Task TEST_MultipleConnectPortsSuspend() var server = new ReverseServer(serverName); Logger.logger.Log($"Server {i} address is '{serverName}'"); serverAndNames.Add((server, serverName)); - dotnetDiagnosticPorts += $"{serverName},connect,suspend;"; + dotnetDiagnosticPorts += $"{serverName},suspend;"; } Logger.logger.Log($"export DOTNET_DiagnosticPorts={dotnetDiagnosticPorts}"); @@ -109,7 +109,6 @@ public static async Task TEST_MultipleConnectPortsSuspend() duringExecution: async (int pid) => { subprocessId = pid; - bool hasResumed = false; // Create an eventpipe session that will tell us when // the EEStartupStarted event happens. This will tell us // the the runtime has been resumed. This should only happen @@ -124,12 +123,14 @@ public static async Task TEST_MultipleConnectPortsSuspend() using Stream eventStream = EventPipeClient.CollectTracing(pid, config, out var sessionId); Logger.logger.Log($"Started EventPipeSession over standard connection with session id: 0x{sessionId:x}"); + var mre = new ManualResetEvent(false); + Task readerTask = Task.Run(async () => { Logger.logger.Log($"Creating EventPipeEventSource"); using var source = new EventPipeEventSource(eventStream); var parser = new ClrPrivateTraceEventParser(source); - parser.StartupEEStartupStart += (eventData) => hasResumed = true; + parser.StartupEEStartupStart += (eventData) => mre.Set(); Logger.logger.Log($"Created EventPipeEventSource"); Logger.logger.Log($"Starting processing"); await Task.Run(() => source.Process()); @@ -138,8 +139,8 @@ public static async Task TEST_MultipleConnectPortsSuspend() for (int i = 0; i < s_NumberOfPorts; i++) { - fSuccess &= !hasResumed; - Logger.logger.Log($"Runtime is resumed (expects: false): {hasResumed}"); + fSuccess &= !mre.WaitOne(0); + Logger.logger.Log($"Runtime HAS NOT resumed (expects: true): {fSuccess}"); var (server, _) = serverAndNames[i]; int serverIndex = i; Stream stream = await server.AcceptAsync(); @@ -155,14 +156,18 @@ public static async Task TEST_MultipleConnectPortsSuspend() Logger.logger.Log($"Port {serverIndex} received: {response.ToString()}"); } + Logger.logger.Log($"Waiting on EEStartupStarted event"); + mre.WaitOne(); + Logger.logger.Log($"Saw EEStartupStarted Event"); + Logger.logger.Log($"Stopping EventPipeSession"); EventPipeClient.StopTracing(pid, sessionId); await readerTask; Logger.logger.Log($"Stopped EventPipeSession"); // runtime should have resumed now - fSuccess &= hasResumed; - Logger.logger.Log($"Runtime is resumed (expects: true): {hasResumed}"); + fSuccess &= mre.WaitOne(0); + Logger.logger.Log($"Runtime HAS resumed (expects: true): {fSuccess}"); } ); @@ -203,7 +208,6 @@ public static async Task TEST_SuspendDefaultPort() duringExecution: async (int pid) => { subprocessId = pid; - bool hasResumed = false; // Create an eventpipe session that will tell us when // the EEStartupStarted event happens. This will tell us // the the runtime has been resumed. This should only happen @@ -218,12 +222,14 @@ public static async Task TEST_SuspendDefaultPort() using Stream eventStream = EventPipeClient.CollectTracing(pid, config, out var sessionId); Logger.logger.Log($"Started EventPipeSession over standard connection with session id: 0x{sessionId:x}"); + var mre = new ManualResetEvent(false); + Task readerTask = Task.Run(async () => { Logger.logger.Log($"Creating EventPipeEventSource"); using var source = new EventPipeEventSource(eventStream); var parser = new ClrPrivateTraceEventParser(source); - parser.StartupEEStartupStart += (eventData) => hasResumed = true; + parser.StartupEEStartupStart += (eventData) => mre.Set(); Logger.logger.Log($"Created EventPipeEventSource"); Logger.logger.Log($"Starting processing"); await Task.Run(() => source.Process()); @@ -231,8 +237,8 @@ public static async Task TEST_SuspendDefaultPort() }); - fSuccess &= !hasResumed; - Logger.logger.Log($"Runtime is resumed (expects: false): {hasResumed}"); + fSuccess &= !mre.WaitOne(0); + Logger.logger.Log($"Runtime HAS NOT resumed (expects: true): {fSuccess}"); // send resume command on this connection var message = new IpcMessage(0x04,0x01); @@ -240,14 +246,18 @@ public static async Task TEST_SuspendDefaultPort() IpcMessage response = IpcClient.SendMessage(ConnectionHelper.GetStandardTransport(pid), message); Logger.logger.Log($"Received: {response.ToString()}"); + Logger.logger.Log($"Waiting for EEStartupStarted event"); + mre.WaitOne(); + Logger.logger.Log($"Saw EEStartupStarted event!"); + Logger.logger.Log($"Stopping EventPipeSession"); EventPipeClient.StopTracing(pid, sessionId); await readerTask; Logger.logger.Log($"Stopped EventPipeSession"); // runtime should have resumed now - fSuccess &= hasResumed; - Logger.logger.Log($"Runtime is resumed (expects: true): {hasResumed}"); + fSuccess &= mre.WaitOne(0); + Logger.logger.Log($"Runtime HAS resumed (expects: true): {fSuccess}"); } ); @@ -267,7 +277,7 @@ public static async Task TEST_AdvertiseAndProcessInfoCookiesMatch() using var memoryStream = new MemoryStream(); Task subprocessTask = Utils.RunSubprocess( currentAssembly: Assembly.GetExecutingAssembly(), - environment: new Dictionary { { Utils.DiagnosticPortsEnvKey, $"{serverName},connect,nosuspend" } }, + environment: new Dictionary { { Utils.DiagnosticPortsEnvKey, $"{serverName}" } }, duringExecution: async (pid) => { Stream stream = await server.AcceptAsync(); @@ -293,6 +303,78 @@ public static async Task TEST_AdvertiseAndProcessInfoCookiesMatch() return fSuccess; } + public static async Task TEST_ConfigValidation() + { + // load the env var with good and bad configs. Operation of good configs shouldn't be impeded by bad ones. + // This test assumes all good configs have a server at the other end of the specified path. + // Note that while a bad config might not crash the application, it may still degrade the process, e.g., + // a bad configuration that specifies at least a path, will most likely still be built and consume resources polling + // for a server that won't exist. + bool fSuccess = true; + var serverAndNames = new List<(ReverseServer, string)>(); + string dotnetDiagnosticPorts = ""; + dotnetDiagnosticPorts += ";;;;;;"; // empty configs shouldn't cause a crash + dotnetDiagnosticPorts += " ; ; ; ; ; ; ; ; ;"; // whitespace only configs shouldn't cause a crash + dotnetDiagnosticPorts += " , , , , , ;,,,,,;;"; // whitespace configs and empty tags with no path shouldn't cause a crash + dotnetDiagnosticPorts += "connect,connect,connect,nosuspend,nosuspend,nosuspend,,,;"; // path that is the same as a tag name and duplicate tags shouldn't cause a crash + dotnetDiagnosticPorts += "SomeRandomPath,nosuspend,suspend,suspend,suspend,suspend;"; // only the first tag from a pair is respected (this should result in a nosuspend port) + dotnetDiagnosticPorts += "%%bad_Path^* fasdf----##2~~,bad tag$$@#@%_)*)@!#(&%.>, , , , :::;"; // invalid path chars and tag chars won't cause a crash + for (int i = 0; i < s_NumberOfPorts; i++) + { + string serverName = ReverseServer.MakeServerAddress(); + var server = new ReverseServer(serverName); + Logger.logger.Log($"Server {i} address is '{serverName}'"); + serverAndNames.Add((server, serverName)); + dotnetDiagnosticPorts += $"{serverName};"; + dotnetDiagnosticPorts += $"{serverName};"; // duplicating port configs shouldn't cause issues + } + Logger.logger.Log($"export DOTNET_DiagnosticPorts={dotnetDiagnosticPorts}"); + var advertisements = new List(); + Object sync = new Object(); + int subprocessId = -1; + Task subprocessTask = Utils.RunSubprocess( + currentAssembly: Assembly.GetExecutingAssembly(), + environment: new Dictionary + { + { Utils.DiagnosticPortsEnvKey, dotnetDiagnosticPorts } + }, + duringExecution: async (int pid) => + { + subprocessId = pid; + var tasks = new List(); + for (int i = 0; i < s_NumberOfPorts; i++) + { + var (server, _) = serverAndNames[i]; + int serverIndex = i; + tasks.Add(Task.Run(async () => + { + Stream stream = await server.AcceptAsync(); + IpcAdvertise advertise = IpcAdvertise.Parse(stream); + lock(sync) + advertisements.Add(advertise); + Logger.logger.Log($"Server {serverIndex} got advertise {advertise.ToString()}"); + })); + } + + await Task.WhenAll(tasks); + } + ); + + fSuccess &= await subprocessTask; + + foreach (var (server, _) in serverAndNames) + server.Shutdown(); + + Guid referenceCookie = advertisements[0].RuntimeInstanceCookie; + foreach (var adv in advertisements) + { + fSuccess &= (int)adv.ProcessId == subprocessId; + fSuccess &= adv.RuntimeInstanceCookie.Equals(referenceCookie); + } + + return fSuccess; + } + public static async Task Main(string[] args) { if (args.Length >= 1) diff --git a/src/tests/tracing/eventpipe/pauseonstart/pauseonstart.cs b/src/tests/tracing/eventpipe/pauseonstart/pauseonstart.cs index e87d367651b06b..1bd8009be85e5e 100644 --- a/src/tests/tracing/eventpipe/pauseonstart/pauseonstart.cs +++ b/src/tests/tracing/eventpipe/pauseonstart/pauseonstart.cs @@ -28,7 +28,7 @@ public static async Task TEST_RuntimeResumesExecutionWithCommand() var server = new ReverseServer(serverName); Task subprocessTask = Utils.RunSubprocess( currentAssembly: Assembly.GetExecutingAssembly(), - environment: new Dictionary { { Utils.DiagnosticPortsEnvKey, $"{serverName},connect,suspend" } }, + environment: new Dictionary { { Utils.DiagnosticPortsEnvKey, $"{serverName},suspend" } }, duringExecution: async (_) => { Stream stream = await server.AcceptAsync(); @@ -56,7 +56,7 @@ public static async Task TEST_TracesHaveRelevantEvents() using var memoryStream = new MemoryStream(); Task subprocessTask = Utils.RunSubprocess( currentAssembly: Assembly.GetExecutingAssembly(), - environment: new Dictionary { { Utils.DiagnosticPortsEnvKey, $"{serverName},connect,suspend" } }, + environment: new Dictionary { { Utils.DiagnosticPortsEnvKey, $"{serverName},suspend" } }, duringExecution: async (pid) => { Stream stream = await server.AcceptAsync(); @@ -114,7 +114,7 @@ public static async Task TEST_MultipleSessionsCanBeStartedWhilepaused() using var memoryStream3 = new MemoryStream(); Task subprocessTask = Utils.RunSubprocess( currentAssembly: Assembly.GetExecutingAssembly(), - environment: new Dictionary { { Utils.DiagnosticPortsEnvKey, $"{serverName},connect,suspend" } }, + environment: new Dictionary { { Utils.DiagnosticPortsEnvKey, $"{serverName},suspend" } }, duringExecution: async (pid) => { Stream stream = await server.AcceptAsync(); @@ -207,7 +207,7 @@ public static async Task TEST_CanStartAndStopSessionWhilepaused() using var memoryStream3 = new MemoryStream(); Task subprocessTask = Utils.RunSubprocess( currentAssembly: Assembly.GetExecutingAssembly(), - environment: new Dictionary { { Utils.DiagnosticPortsEnvKey, $"{serverName},connect,suspend" } }, + environment: new Dictionary { { Utils.DiagnosticPortsEnvKey, $"{serverName},suspend" } }, duringExecution: async (pid) => { Stream stream = await server.AcceptAsync(); @@ -271,7 +271,7 @@ public static async Task TEST_DisabledCommandsError() using var memoryStream3 = new MemoryStream(); Task subprocessTask = Utils.RunSubprocess( currentAssembly: Assembly.GetExecutingAssembly(), - environment: new Dictionary { { Utils.DiagnosticPortsEnvKey, $"{serverName},connect,suspend" } }, + environment: new Dictionary { { Utils.DiagnosticPortsEnvKey, $"{serverName},suspend" } }, duringExecution: async (pid) => { Stream stream = await server.AcceptAsync(); diff --git a/src/tests/tracing/eventpipe/reverse/reverse.cs b/src/tests/tracing/eventpipe/reverse/reverse.cs index 6ce09ea82b81b7..d26c4ee5d87aaf 100644 --- a/src/tests/tracing/eventpipe/reverse/reverse.cs +++ b/src/tests/tracing/eventpipe/reverse/reverse.cs @@ -28,7 +28,7 @@ public static async Task TEST_RuntimeIsResilientToServerClosing() currentAssembly: Assembly.GetExecutingAssembly(), environment: new Dictionary { - { Utils.DiagnosticPortsEnvKey, $"{serverName},connect,nosuspend" } + { Utils.DiagnosticPortsEnvKey, $"{serverName}" } }, duringExecution: async (_) => { @@ -58,7 +58,7 @@ public static async Task TEST_RuntimeConnectsToExistingServer() currentAssembly: Assembly.GetExecutingAssembly(), environment: new Dictionary { - { Utils.DiagnosticPortsEnvKey, $"{serverName},connect,nosuspend" } + { Utils.DiagnosticPortsEnvKey, $"{serverName}" } }, duringExecution: async (_) => { @@ -83,7 +83,7 @@ public static async Task TEST_CanConnectServerAndClientAtSameTime() currentAssembly: Assembly.GetExecutingAssembly(), environment: new Dictionary { - { Utils.DiagnosticPortsEnvKey, $"{serverName},connect,nosuspend" } + { Utils.DiagnosticPortsEnvKey, $"{serverName}" } }, duringExecution: async (int pid) => { @@ -136,7 +136,7 @@ public static async Task TEST_ServerWorksIfClientDoesntAccept() currentAssembly: Assembly.GetExecutingAssembly(), environment: new Dictionary { - { Utils.DiagnosticPortsEnvKey, $"{serverName},connect,nosuspend" } + { Utils.DiagnosticPortsEnvKey, $"{serverName}" } }, duringExecution: async (int pid) => { @@ -177,7 +177,7 @@ public static async Task TEST_ServerIsResilientToNoBufferAgent() currentAssembly: Assembly.GetExecutingAssembly(), environment: new Dictionary { - { Utils.DiagnosticPortsEnvKey, $"{serverName},connect,nosuspend" } + { Utils.DiagnosticPortsEnvKey, $"{serverName}" } }, duringExecution: async (int pid) => { @@ -215,7 +215,7 @@ public static async Task TEST_StandardConnectionStillWorksIfReverseConnect currentAssembly: Assembly.GetExecutingAssembly(), environment: new Dictionary { - { Utils.DiagnosticPortsEnvKey, $"{serverName},connect,nosuspend" } + { Utils.DiagnosticPortsEnvKey, $"{serverName}" } }, duringExecution: async (int pid) => { diff --git a/src/tests/tracing/eventpipe/reverseouter/reverseouter.cs b/src/tests/tracing/eventpipe/reverseouter/reverseouter.cs index c0be4c21102404..fda0410e11209e 100644 --- a/src/tests/tracing/eventpipe/reverseouter/reverseouter.cs +++ b/src/tests/tracing/eventpipe/reverseouter/reverseouter.cs @@ -28,7 +28,7 @@ public static async Task TEST_ReverseConnectionCanRecycleWhileTracing() currentAssembly: Assembly.GetExecutingAssembly(), environment: new Dictionary { - { Utils.DiagnosticPortsEnvKey, $"{serverName},connect,nosuspend" } + { Utils.DiagnosticPortsEnvKey, $"{serverName}" } }, duringExecution: async (int pid) => {