From f6c1d8ab05adaec16a0e217c12625a1f6b0458f3 Mon Sep 17 00:00:00 2001 From: Bram Oosterhuis Date: Tue, 12 May 2026 21:34:17 +0200 Subject: [PATCH 01/26] Implement StateMachine for plugin lifecycle management - Introduced a StateMachine class to manage the lifecycle states of plugins, including DEACTIVATED, PRECONDITION, ACTIVATED, HIBERNATED, and UNAVAILABLE. - Defined stable and transient states with clear guarantees for callbacks during state transitions. - Added detailed documentation for state transitions, callback guarantees, reentrancy, and crash handling. - Refactored existing plugin state handling to utilize the new StateMachine, improving clarity and maintainability. - Updated the Service class to integrate the StateMachine and handle state change notifications. - Removed redundant locks and streamlined the Evaluate method to leverage the StateMachine's re-evaluation capabilities. --- Source/Thunder/PluginServer.cpp | 872 ++++++++++++++++++-------------- Source/Thunder/PluginServer.h | 444 ++++++++++++++-- 2 files changed, 894 insertions(+), 422 deletions(-) diff --git a/Source/Thunder/PluginServer.cpp b/Source/Thunder/PluginServer.cpp index b86da1bbd..6180f0e63 100644 --- a/Source/Thunder/PluginServer.cpp +++ b/Source/Thunder/PluginServer.cpp @@ -294,18 +294,9 @@ namespace PluginHost { asIUnknown == false ? result = static_cast(this) : result = static_cast(this); } else { - _queryInterfaceLock.Lock(); - if ((State() == IShell::state::ACTIVATED) || (State() == IShell::state::DEACTIVATION)) { //needed as we only want to send plugin state notifications when the plugin is active or deactivating which is not guaranteed by the lock itself as it comes from the same thread handling the activation and deactivation - if (id == PluginHost::IDispatcher::ID) { - if (_jsonrpc != nullptr) { - _jsonrpc->AddRef(); - asIUnknown == false ? result = _jsonrpc : result = static_cast(_jsonrpc); - } - } else if (_handler != nullptr) { - result = _handler->QueryInterface(id, asIUnknown); - } - } - _queryInterfaceLock.Unlock(); + // Route through the state machine — only ACTIVATED and states that + // explicitly override QueryInterface will return a non-null result. + result = _stateMachine.QueryInterface(id, asIUnknown); } return (result); @@ -338,485 +329,625 @@ namespace PluginHost { // Methods to stop/start/update the service. Core::hresult Server::Service::Activate(const PluginHost::IShell::reason why) /* override */ { - Core::hresult result = Core::ERROR_NONE; + return _stateMachine.Activate(why); + } - Lock(); + uint32_t Server::Service::Resume(const reason why) /* override */ { + uint32_t result = Core::ERROR_NONE; + // Read state snapshot and release Lock() before calling Activate(). + // Holding Lock() across Activate() inverts the lock order: + // Resume: Lock() → _transitionLock (via Activate) + // Transitions: _transitionLock → Lock() + // That inversion is a deadlock. The snapshot is acceptable — Resume() + // was already operating on a snapshot since state could change between + // the check and the Activate() call anyway. + Lock(); IShell::state currentState(State()); + Unlock(); if (currentState == IShell::state::ACTIVATION) { - Unlock(); result = Core::ERROR_INPROGRESS; - } - else if ((currentState == IShell::state::UNAVAILABLE) || (currentState == IShell::state::DEACTIVATION) || (currentState == IShell::state::DESTROYED) ) { - Unlock(); + } else if ((currentState == IShell::state::DEACTIVATION) || (currentState == IShell::state::DESTROYED) || (currentState == IShell::state::HIBERNATED)) { result = Core::ERROR_ILLEGAL_STATE; - } else if (currentState == IShell::state::HIBERNATED) { - result = Wakeup(3000); + } else if (currentState == IShell::state::DEACTIVATED) { + result = Activate(why); + Lock(); + currentState = State(); Unlock(); - } else if ((currentState == IShell::state::DEACTIVATED) || (currentState == IShell::state::PRECONDITION)) { - - _reason = why; - - _queryInterfaceLock.Lock(); + } - // Load the interfaces, If we did not load them yet... - if (_handler == nullptr) { - AcquireInterfaces(); + if (currentState == IShell::ACTIVATED) { + Lock(); + // See if we need can and should RESUME. + if (_stateControl == nullptr) { + result = Core::ERROR_BAD_REQUEST; } + else { + // We have a StateControl interface, so at least start resuming, if not already resumed :-) + if (_stateControl->State() == PluginHost::IStateControl::SUSPENDED) { + result = _stateControl->Request(PluginHost::IStateControl::RESUME); + } + } + Unlock(); + } - const string callSign(PluginHost::Service::Configuration().Callsign.Value()); - const string className(PluginHost::Service::Configuration().ClassName.Value()); - - if (_handler == nullptr) { - SYSLOG(Logging::Startup, (_T("Loading of plugin [%s]:[%s], failed. Error [%s]"), className.c_str(), callSign.c_str(), ErrorMessage().c_str())); - result = Core::ERROR_UNAVAILABLE; - _reason = reason::INSTANTIATION_FAILED; - State(DEACTIVATED); - - _queryInterfaceLock.Unlock(); - - Unlock(); - - // See if the preconditions have been met.. - } else if (_precondition.IsMet() == false) { - SYSLOG(Logging::Startup, (_T("Activation of plugin [%s]:[%s], postponed, preconditions have not been met, yet."), className.c_str(), callSign.c_str())); - result = Core::ERROR_PENDING_CONDITIONS; - State(PRECONDITION); + return (result); + } - _queryInterfaceLock.Unlock(); + Core::hresult Server::Service::Deactivate(const reason why) /* override */ + { + return _stateMachine.Deactivate(why); + } - if (Thunder::Messaging::LocalLifetimeType::IsEnabled() == true) { + uint32_t Server::Service::Suspend(const reason why) { - string feedback; - uint8_t index = 1; - uint32_t delta(_precondition.Delta(_administrator.SubSystemInfo().Value())); + uint32_t result = Core::ERROR_NONE; - while (delta != 0) { - if ((delta & 0x01) != 0) { - if (feedback.empty() == false) { - feedback += ','; - } + if (StartMode() == PluginHost::IShell::startmode::DEACTIVATED) { + // We need to shutdown completely + result = Deactivate(why); + } + else { + Lock(); - PluginHost::ISubSystem::subsystem element(static_cast(index)); - feedback += string(Core::EnumerateType(element).Data()); - } + IShell::state currentState(State()); - delta = (delta >> 1); - index++; + if (currentState == IShell::state::DEACTIVATION) { + result = Core::ERROR_INPROGRESS; + } else if ((currentState == IShell::state::ACTIVATION) || (currentState == IShell::state::DESTROYED) || (currentState == IShell::state::HIBERNATED)) { + result = Core::ERROR_ILLEGAL_STATE; + } else if ((currentState == IShell::state::ACTIVATED) || (currentState == IShell::state::PRECONDITION)) { + // See if we need can and should SUSPEND. + if (_stateControl == nullptr) { + result = Core::ERROR_BAD_REQUEST; + } + else { + // We have a StateControl interface, so at least start suspending, if not already suspended :-) + if (_stateControl->State() == PluginHost::IStateControl::RESUMED) { + result = _stateControl->Request(PluginHost::IStateControl::SUSPEND); } - - TRACE(Activity, (_T("Delta preconditions: %s"), feedback.c_str())); } + } - Unlock(); + Unlock(); + } - } else { + return (result); + } - // Before we dive into the "new" initialize lets see if this has a pending OOP running, if so forcefully kill it now, no time to wait ! - if (_lastId != 0) { - _administrator.Destroy(_lastId); - _lastId = 0; - } + Core::hresult Server::Service::Unavailable(const reason why) /* override */ { + return _stateMachine.Unavailable(why); + } - TRACE(Activity, (_T("Activation plugin [%s]:[%s]"), className.c_str(), callSign.c_str())); + Core::hresult Server::Service::Hibernate(const uint32_t timeout VARIABLE_IS_NOT_USED) /* override */ { + return _stateMachine.Hibernate(timeout); + } - _administrator.Initialize(callSign, this); + // ------------------------------------------------------------------------- + // StateMachine — static state instance definitions + // ------------------------------------------------------------------------- - State(ACTIVATION); + Server::Service::StateMachine::DeactivatedState Server::Service::StateMachine::_stateDeactivated; + Server::Service::StateMachine::PreconditionState Server::Service::StateMachine::_statePrecondition; + Server::Service::StateMachine::ActivationState Server::Service::StateMachine::_stateActivation; + Server::Service::StateMachine::ActivatedState Server::Service::StateMachine::_stateActivated; + Server::Service::StateMachine::DeactivationState Server::Service::StateMachine::_stateDeactivation; + Server::Service::StateMachine::HibernatedState Server::Service::StateMachine::_stateHibernated; + Server::Service::StateMachine::UnavailableState Server::Service::StateMachine::_stateUnavailable; - Unlock(); + // ------------------------------------------------------------------------- + // DeactivatedState + // ------------------------------------------------------------------------- - REPORT_DURATION_WARNING( { ErrorMessage(_handler->Initialize(this)); }, WarningReporting::TooLongPluginState, WarningReporting::TooLongPluginState::StateChange::ACTIVATION, callSign.c_str()); + Core::hresult Server::Service::StateMachine::DeactivatedState::Activate(StateMachine& sm, const reason why) + { + const string callSign(sm._parent.PluginHost::Service::Configuration().Callsign.Value()); - if (HasError() == true) { - result = Core::ERROR_GENERAL; + sm._parent.Lock(); + sm._parent._reason = why; - SYSLOG(Logging::Startup, (_T("Activation of plugin [%s]:[%s], failed. Error [%s]"), className.c_str(), callSign.c_str(), ErrorMessage().c_str())); + Core::hresult result = sm._parent.LoadPlugin(); - _reason = reason::INITIALIZATION_FAILED; + if (result == Core::ERROR_UNAVAILABLE) { + sm.SetState(_stateDeactivated); + sm._parent.Unlock(); + return result; + } + if (result == Core::ERROR_PENDING_CONDITIONS) { + sm.SetState(_statePrecondition); + sm._parent.Unlock(); + return result; + } - if( _administrator.Configuration().LegacyInitialize() == false ) { - REPORT_DURATION_WARNING({ _handler->Deinitialize(this); }, WarningReporting::TooLongPluginState, WarningReporting::TooLongPluginState::StateChange::DEACTIVATION, callSign.c_str()); - } + sm._parent._administrator.Initialize(callSign, &sm._parent); + sm.SetState(_stateActivation); + sm._parent.Unlock(); - Lock(); - ReleaseInterfaces(); - State(DEACTIVATED); - Unlock(); + result = sm._parent.InitializePlugin(); - _queryInterfaceLock.Unlock(); + if (result != Core::ERROR_NONE) { + sm._parent.Lock(); + sm._parent._reason = IShell::reason::INITIALIZATION_FAILED; + sm._parent.UnloadPlugin(); + sm.SetState(_stateDeactivated); + sm._parent.Unlock(); + sm._callback(IShell::DEACTIVATED); + return result; + } - _administrator.Deinitialized(callSign, this); + sm._parent.Attach(); - } else { + sm._parent.Lock(); + sm.SetState(_stateActivated); + sm._parent.Unlock(); - const Core::EnumerateType textReason(why); - const string webUI(PluginHost::Service::Configuration().WebUI.Value()); - if ((PluginHost::Service::Configuration().WebUI.IsSet()) || (webUI.empty() == false)) { - EnableWebServer(webUI, EMPTY_STRING); - } + sm._callback(IShell::ACTIVATED); + return Core::ERROR_NONE; + } - if (_jsonrpc != nullptr) { - PluginHost::IShell::IConnectionServer::INotification* sink = nullptr; - _jsonrpc->Attach(sink, this); - if (sink != nullptr) { - Register(sink); - sink->Release(); - } - } + Core::hresult Server::Service::StateMachine::DeactivatedState::Unavailable(StateMachine& sm, const reason why) + { + if (sm._parent.AllowedUnavailable() == false) { + return Core::ERROR_NOT_SUPPORTED; + } - if (_external.Connector().empty() == false) { - uint32_t result = _external.Open(0); - if ((result != Core::ERROR_NONE) && (result != Core::ERROR_INPROGRESS)) { - TRACE(Trace::Error, (_T("Could not open the external connector for %s"), Callsign().c_str())); - } - } + const string callSign(sm._parent.PluginHost::Service::Configuration().Callsign.Value()); + const string className(sm._parent.PluginHost::Service::Configuration().ClassName.Value()); - SYSLOG(Logging::Startup, (_T("Activated plugin [%s]:[%s]"), className.c_str(), callSign.c_str())); - Lock(); - State(ACTIVATED); + sm._parent.Lock(); + sm._parent._reason = why; + SYSLOG(Logging::Shutdown, (_T("Unavailable plugin [%s]:[%s]"), className.c_str(), callSign.c_str())); + TRACE(Activity, (Core::Format(_T("Unavailable plugin [%s]:[%s]"), className.c_str(), callSign.c_str()))); + sm.SetState(_stateUnavailable); + sm._parent.Unlock(); - _queryInterfaceLock.Unlock(); + sm._callback(IShell::UNAVAILABLE); + return Core::ERROR_NONE; + } - _administrator.Activated(callSign, this); + // ------------------------------------------------------------------------- + // PreconditionState + // ------------------------------------------------------------------------- - _stateControl = _handler->QueryInterface(); - if (_stateControl != nullptr) { - _stateControl->Register(&_composit); + Core::hresult Server::Service::StateMachine::PreconditionState::Deactivate(StateMachine& sm, const reason why) + { + sm._parent.Lock(); + sm._parent._reason = why; + const IShell::state finalState(why == IShell::CONDITIONS ? IShell::PRECONDITION : IShell::DEACTIVATED); + sm.SetState(why == IShell::CONDITIONS ? static_cast(_statePrecondition) : static_cast(_stateDeactivated)); + sm._parent.Unlock(); - if (Resumed() == true) { - _stateControl->Request(PluginHost::IStateControl::RESUME); - } - } + sm._parent.UnloadPlugin(); - Unlock(); + sm._callback(finalState); + return Core::ERROR_NONE; + } - #ifdef THUNDER_RESTFULL_API - Notify(EMPTY_STRING, string(_T("{\"state\":\"activated\",\"reason\":\"")) + textReason.Data() + _T("\"}")); - #endif - Notify(_T("statechange"), string(_T("{\"state\":\"activated\",\"reason\":\"")) + textReason.Data() + _T("\"}")); - } - } + void Server::Service::StateMachine::PreconditionState::Reevaluate(StateMachine& sm) + { + sm._parent.Lock(); + const uint32_t subsystems(sm._parent._administrator.SubSystemInfo().Value()); + + if ((sm._parent._precondition.Evaluate(subsystems) == true) && (sm._parent._precondition.IsMet() == true)) { + sm._parent.Unlock(); + sm._Activate(sm._parent._reason); } else { - Unlock(); + sm._parent.Unlock(); } - - return (result); } - uint32_t Server::Service::Resume(const reason why) /* override */ { - uint32_t result = Core::ERROR_NONE; - Lock(); + // ------------------------------------------------------------------------- + // ActivationState — only INITIALIZATION_FAILED deactivation is legal + // ------------------------------------------------------------------------- - IShell::state currentState(State()); - - if (currentState == IShell::state::ACTIVATION) { - result = Core::ERROR_INPROGRESS; - } else if ((currentState == IShell::state::DEACTIVATION) || (currentState == IShell::state::DESTROYED) || (currentState == IShell::state::HIBERNATED)) { - result = Core::ERROR_ILLEGAL_STATE; - } else if (currentState == IShell::state::DEACTIVATED) { - result = Activate(why); - currentState = State(); - } - - if (currentState == IShell::ACTIVATED) { - // See if we need can and should RESUME. - if (_stateControl == nullptr) { - result = Core::ERROR_BAD_REQUEST; - } - else { - // We have a StateControl interface, so at least start resuming, if not already resumed :-) - if (_stateControl->State() == PluginHost::IStateControl::SUSPENDED) { - result = _stateControl->Request(PluginHost::IStateControl::RESUME); - } - } + Core::hresult Server::Service::StateMachine::ActivationState::Deactivate(StateMachine& sm, const reason why) + { + if (why != IShell::reason::INITIALIZATION_FAILED) { + return Core::ERROR_ILLEGAL_STATE; } - Unlock(); + const string callSign(sm._parent.PluginHost::Service::Configuration().Callsign.Value()); - return (result); - } - - Core::hresult Server::Service::Deactivate(const reason why) /* override */ - { - Core::hresult result = Core::ERROR_NONE; + sm._parent.Lock(); + sm._parent._reason = why; + sm.SetState(_stateDeactivation); + sm._parent.Unlock(); - Lock(); + sm.Evaluate(); // temporarily releases _transitionLock — safe, DEACTIVATION is set + Server::PostMortem(sm._parent, why, sm._parent._connection); + sm._parent.Detach(); - IShell::state currentState(State()); + sm._parent.DeinitializePlugin(); + sm._parent.Lock(); - if (currentState == IShell::state::DEACTIVATION) { - result = Core::ERROR_INPROGRESS; - Unlock(); - } - else if ( ((currentState == IShell::state::ACTIVATION) && (why != IShell::reason::INITIALIZATION_FAILED)) || (currentState == IShell::state::DESTROYED)) { - result = Core::ERROR_ILLEGAL_STATE; - Unlock(); - } - else if ( ((currentState == IShell::state::ACTIVATION) && (why == IShell::reason::INITIALIZATION_FAILED)) || (currentState == IShell::state::UNAVAILABLE) || (currentState == IShell::state::ACTIVATED) || (currentState == IShell::state::PRECONDITION) || (currentState == IShell::state::HIBERNATED) ) { - const Core::EnumerateType textReason(why); + sm.SetState(_stateDeactivated); + sm._parent.UnloadPlugin(); + sm._parent.Unlock(); - const string className(PluginHost::Service::Configuration().ClassName.Value()); - const string callSign(PluginHost::Service::Configuration().Callsign.Value()); + sm._callback(IShell::DEACTIVATED); + return Core::ERROR_NONE; + } - _reason = why; + // ------------------------------------------------------------------------- + // ActivatedState + // ------------------------------------------------------------------------- - if(currentState == IShell::state::HIBERNATED) - { - uint32_t wakeupResult = Wakeup(3000); - if(wakeupResult != Core::ERROR_NONE) - { - //Force Activated state - State(ACTIVATED); - } - currentState = ACTIVATED; - } + Core::hresult Server::Service::StateMachine::ActivatedState::Deactivate(StateMachine& sm, const reason why) + { + const string callSign(sm._parent.PluginHost::Service::Configuration().Callsign.Value()); - if ( (currentState == IShell::ACTIVATION) || (currentState == IShell::ACTIVATED)) { - ASSERT(_handler != nullptr); + sm._parent.Lock(); + sm._parent._reason = why; - State(DEACTIVATION); + SystemInfo& systeminfo = sm._parent._administrator.SubSystemInfo(); + for (const PluginHost::ISubSystem::subsystem sys : sm._parent.SubSystemControl()) { + systeminfo.Unset(sys); + } - SystemInfo& systeminfo = _administrator.SubSystemInfo(); + // Set transient state before Evaluate() so RecursiveNotification sees + // DEACTIVATION and short-circuits — no-op in DeactivationState::Reevaluate. + sm.SetState(_stateDeactivation); + sm._parent.Unlock(); - // On behalf of the plugin stop all subsystems it was controlling. - for (const PluginHost::ISubSystem::subsystem sys : SubSystemControl()) { - systeminfo.Unset(sys); - } + // Temporarily releases _transitionLock — safe because DEACTIVATION is set. + sm.Evaluate(); - Unlock(); + // Fire Deactivated() while _handler is still alive — subscribers may + // safely call QueryInterface during this notification. + sm._parent._administrator.Deactivated(callSign, &sm._parent); - // Reevaluate status. In case some plugins were dependant on the subsystems - // that have been just disabled, deactivate them too, recursively. - _administrator.Evaluate(); + Server::PostMortem(sm._parent, why, sm._parent._connection); + sm._parent.Detach(); - // And finally start tearing down this plugin... + sm._parent.DeinitializePlugin(); + sm._parent.Lock(); - if (_stateControl != nullptr) { - _stateControl->Unregister(&_composit); - _stateControl->Release(); - _stateControl = nullptr; - } + const IShell::state finalState(why == IShell::CONDITIONS ? IShell::PRECONDITION : IShell::DEACTIVATED); + sm.SetState(why == IShell::CONDITIONS ? static_cast(_statePrecondition) : static_cast(_stateDeactivated)); + sm._parent.UnloadPlugin(); + sm._parent.Unlock(); - if (currentState == IShell::ACTIVATED) { - TRACE(Activity, (_T("Deactivating plugin [%s]:[%s]"), className.c_str(), callSign.c_str())); - _administrator.Deactivated(callSign, this); - } - - // We might require PostMortem analyses if the reason is not really clear. Call the PostMortum installed so it can generate - // required logs/OS information before we start to kill it. - Server::PostMortem(*this, why, _connection); + sm._callback(finalState); + return Core::ERROR_NONE; + } - // If we enabled the webserver, we should also disable it. - if ((PluginHost::Service::Configuration().WebUI.IsSet()) || (PluginHost::Service::Configuration().WebUI.Value().empty() == false)) { - DisableWebServer(); - } + Core::hresult Server::Service::StateMachine::ActivatedState::Hibernate(StateMachine& sm, const uint32_t timeout VARIABLE_IS_NOT_USED) + { + if (sm._parent.AllowedHibernate() == false) { + return Core::ERROR_NOT_SUPPORTED; + } - _queryInterfaceLock.Lock(); + sm._parent.Lock(); - REPORT_DURATION_WARNING( { _handler->Deinitialize(this); }, WarningReporting::TooLongPluginState, WarningReporting::TooLongPluginState::StateChange::DEACTIVATION, callSign.c_str()); + if (sm._parent._connection == nullptr) { + sm._parent.Unlock(); + return Core::ERROR_INPROC; + } - Lock(); + RPC::IMonitorableProcess* local = sm._parent._connection->QueryInterface(); - if (currentState != IShell::state::ACTIVATION) { - SYSLOG(Logging::Shutdown, (_T("Deactivated plugin [%s]:[%s]"), className.c_str(), callSign.c_str())); + if (local == nullptr) { + sm._parent.Unlock(); + return Core::ERROR_BAD_REQUEST; + } -#ifdef THUNDER_RESTFULL_API - Notify(EMPTY_STRING, string(_T("{\"state\":\"deactivated\",\"reason\":\"")) + textReason.Data() + _T("\"}")); -#endif - Notify(_T("statechange"), string(_T("{\"state\":\"deactivated\",\"reason\":\"")) + textReason.Data() + _T("\"}")); - } + // Set HIBERNATED under lock as the in-progress guard. + // Note: this is set before the blocking HibernateProcess() call — the process + // is still running at this point. A HIBERNATING transient state would be + // more accurate but requires an IShell::state enum change (tracked as debt). + sm.SetState(_stateHibernated); - if (_external.Connector().empty() == false) { - _external.Close(0); - } +#ifdef HIBERNATE_SUPPORT_ENABLED + pid_t parentPID = local->ParentPID(); + local->Release(); + sm._parent.Unlock(); + + TRACE(Activity, (_T("Hibernation of plugin [%s] process [%u]"), sm._parent.Callsign().c_str(), parentPID)); + Core::hresult result = HibernateProcess(timeout, parentPID, sm._parent._administrator.Configuration().HibernateLocator().c_str(), _T(""), &sm._parent._hibernateStorage); + + sm._parent.Lock(); + // Note: in the original design this checked whether a concurrent Wakeup() or + // Deactivate() had changed state during HibernateProcess(). Under the new design + // _transitionLock is held throughout — no concurrent transition can run — so this + // check can never fire. Retained as a defensive guard only. + if (sm._parent.State() != IShell::HIBERNATED) { + SYSLOG(Logging::Startup, (_T("Hibernation aborted of plugin [%s] process [%u]"), sm._parent.Callsign().c_str(), parentPID)); + sm._parent.Unlock(); + return Core::ERROR_ABORTED; + } + sm._parent.Unlock(); - if (_jsonrpc != nullptr) { - PluginHost::IShell::IConnectionServer::INotification* sink = nullptr; + if (result == HIBERNATE_ERROR_NONE) { + result = sm._parent.HibernateChildren(parentPID, timeout); + } - _jsonrpc->Detach(sink); + if (result != Core::ERROR_NONE && result != Core::ERROR_ABORTED) { + // Rollback — try to wake the parent process to recover from failed hibernation. + TRACE(Activity, (_T("Wakeup plugin [%s] process [%u] on Hibernate error [%d]"), sm._parent.Callsign().c_str(), parentPID, result)); + WakeupProcess(timeout, parentPID, sm._parent._administrator.Configuration().HibernateLocator().c_str(), _T(""), &sm._parent._hibernateStorage); + } - if (sink != nullptr) { - Unregister(sink); - sink->Release(); - } - } + sm._parent.Lock(); +#else + local->Release(); + Core::hresult result = Core::ERROR_NONE; +#endif + // coverity[DEADCODE] — on the non-HIBERNATE_ENABLED path result is always + // ERROR_NONE here, making the else-if appear unreachable to Coverity. + // Both branches are reachable when HIBERNATE_SUPPORT_ENABLED is defined. + if (result == Core::ERROR_NONE) { + if (sm._parent.State() == IShell::state::HIBERNATED) { + SYSLOG(Logging::Startup, ("Hibernated plugin [%s]:[%s]", sm._parent.ClassName().c_str(), sm._parent.Callsign().c_str())); + sm._parent.Unlock(); + sm._callback(IShell::HIBERNATED); + } else { + // Wakeup occurred right after hibernation finished. + SYSLOG(Logging::Startup, ("Hibernation aborted of plugin [%s]:[%s]", sm._parent.ClassName().c_str(), sm._parent.Callsign().c_str())); + sm._parent.Unlock(); + result = Core::ERROR_ABORTED; } + } else if (sm._parent.State() == IShell::state::HIBERNATED) { + // Hibernation failed — roll back state to ACTIVATED. + sm.SetState(_stateActivated); + SYSLOG(Logging::Startup, (_T("Hibernation error [%d] of [%s]:[%s]"), result, sm._parent.ClassName().c_str(), sm._parent.Callsign().c_str())); + sm._parent.Unlock(); + sm._callback(IShell::ACTIVATED); + } else { + sm._parent.Unlock(); + } - State(why == CONDITIONS ? PRECONDITION : DEACTIVATED); - - // We have no need for his module anymore.. - ReleaseInterfaces(); - Unlock(); + return result; + } - if ((currentState == IShell::ACTIVATION) || (currentState == IShell::ACTIVATED)) { - _queryInterfaceLock.Unlock(); - } + void Server::Service::StateMachine::ActivatedState::Reevaluate(StateMachine& sm) + { + sm._parent.Lock(); + const uint32_t subsystems(sm._parent._administrator.SubSystemInfo().Value()); - _administrator.Deinitialized(callSign, this); + if ((sm._parent._termination.Evaluate(subsystems) == true) && (sm._parent._termination.IsMet() == false)) { + sm._parent.Unlock(); + sm._Deactivate(IShell::CONDITIONS); } else { - Unlock(); + sm._parent.Unlock(); } - - return (result); } - uint32_t Server::Service::Suspend(const reason why) { + void* Server::Service::StateMachine::ActivatedState::QueryInterface(StateMachine& sm, const uint32_t id, const bool asIUnknown) + { + // Take a ref-counted local copy of _handler under _pluginHandling. + // This keeps _handler alive across the lock boundary even if + // UnloadPlugin() runs concurrently on another thread. + sm._parent._pluginHandling.Lock(); + + if (id == PluginHost::IDispatcher::ID) { + // Fast path: return the cached _jsonrpc pointer directly rather than + // routing through _handler->QueryInterface(). This is intentional. + // _jsonrpc is set once in AcquireInterfaces() via + // _handler->QueryInterface() and is the same pointer the + // plugin would return. Bypassing the plugin's own QueryInterface avoids + // a potential lock inversion if the plugin's QI acquires internal locks. + // Assumption: plugins do not conditionally expose IDispatcher at runtime. + // If a plugin gates IDispatcher on runtime state, this fast path will + // return the cached pointer regardless — revisit if that pattern emerges. + IDispatcher* jsonrpc = sm._parent._jsonrpc; + if (jsonrpc != nullptr) { + jsonrpc->AddRef(); + } + sm._parent._pluginHandling.Unlock(); + if (jsonrpc != nullptr) { + return asIUnknown == false ? static_cast(jsonrpc) : static_cast(static_cast(jsonrpc)); + } + return nullptr; + } - uint32_t result = Core::ERROR_NONE; + IPlugin* handler = sm._parent._handler; + if (handler != nullptr) { + handler->AddRef(); + } + sm._parent._pluginHandling.Unlock(); - if (StartMode() == PluginHost::IShell::startmode::DEACTIVATED) { - // We need to shutdown completely - result = Deactivate(why); + if (handler == nullptr) { + return nullptr; } - else { - Lock(); - IShell::state currentState(State()); + void* result = handler->QueryInterface(id, asIUnknown); + handler->Release(); + return result; + } - if (currentState == IShell::state::DEACTIVATION) { - result = Core::ERROR_INPROGRESS; - } else if ((currentState == IShell::state::ACTIVATION) || (currentState == IShell::state::DESTROYED) || (currentState == IShell::state::HIBERNATED)) { - result = Core::ERROR_ILLEGAL_STATE; - } else if ((currentState == IShell::state::ACTIVATED) || (currentState == IShell::state::PRECONDITION)) { - // See if we need can and should SUSPEND. - if (_stateControl == nullptr) { - result = Core::ERROR_BAD_REQUEST; - } - else { - // We have a StateControl interface, so at least start suspending, if not already suspended :-) - if (_stateControl->State() == PluginHost::IStateControl::RESUMED) { - result = _stateControl->Request(PluginHost::IStateControl::SUSPEND); - } - } - } + // ------------------------------------------------------------------------- + // HibernatedState + // ------------------------------------------------------------------------- - Unlock(); + Core::hresult Server::Service::StateMachine::HibernatedState::Activate(StateMachine& sm, const reason why VARIABLE_IS_NOT_USED) + { + const Core::hresult result = sm._parent.Wakeup(3000); + if (result == Core::ERROR_NONE) { + sm.SetState(_stateActivated); + sm._callback(IShell::ACTIVATED); } + return result; + } - return (result); + Core::hresult Server::Service::StateMachine::HibernatedState::Deactivate(StateMachine& sm, const reason why) + { + // Always update _current before dispatching regardless of Wakeup result. + // If _current is not updated and Wakeup succeeds, _Deactivate dispatches + // back to HibernatedState::Deactivate → infinite recursion → stack overflow. + sm._parent.Wakeup(3000); // best-effort, errors are non-fatal here + sm.SetState(_stateActivated); // _current must be updated before _Deactivate + return sm._Deactivate(why); } - Core::hresult Server::Service::Unavailable(const reason why) /* override */ { - Core::hresult result = Core::ERROR_NONE; + // ------------------------------------------------------------------------- + // UnavailableState + // ------------------------------------------------------------------------- - if (AllowedUnavailable() == true) { + Core::hresult Server::Service::StateMachine::UnavailableState::Deactivate(StateMachine& sm, const reason why) + { + sm._parent.Lock(); + sm._parent._reason = why; + sm.SetState(_stateDeactivated); + sm._parent.Unlock(); + sm._callback(IShell::DEACTIVATED); + return Core::ERROR_NONE; + } - Lock(); + // ------------------------------------------------------------------------- + // Work methods — called by StateMachine, one concern each + // ------------------------------------------------------------------------- - IShell::state currentState(State()); + Core::hresult Server::Service::LoadPlugin() + { + ASSERT(_stateMachine.IsTransitionThread()); + // Pre: Lock() held by caller (state transition in progress) + const string callSign(PluginHost::Service::Configuration().Callsign.Value()); + const string className(PluginHost::Service::Configuration().ClassName.Value()); - if ((currentState == IShell::state::DEACTIVATION) || (currentState == IShell::state::ACTIVATION) || (currentState == IShell::state::DESTROYED) || (currentState == IShell::state::ACTIVATED) || (currentState == IShell::state::PRECONDITION) || (currentState == IShell::state::HIBERNATED)) { - result = Core::ERROR_ILLEGAL_STATE; - Unlock(); - } else if (currentState == IShell::state::DEACTIVATED) { + if (_handler == nullptr) { + AcquireInterfaces(); + } + + if (_handler == nullptr) { + SYSLOG(Logging::Startup, (_T("Loading of plugin [%s]:[%s], failed. Error [%s]"), className.c_str(), callSign.c_str(), ErrorMessage().c_str())); + _reason = reason::INSTANTIATION_FAILED; + return Core::ERROR_UNAVAILABLE; + } + + if (_precondition.IsMet() == false) { + SYSLOG(Logging::Startup, (_T("Activation of plugin [%s]:[%s], postponed, preconditions have not been met, yet."), className.c_str(), callSign.c_str())); + + if (Thunder::Messaging::LocalLifetimeType::IsEnabled() == true) { + string feedback; + uint8_t index = 1; + uint32_t delta(_precondition.Delta(_administrator.SubSystemInfo().Value())); - const Core::EnumerateType textReason(why); + while (delta != 0) { + if ((delta & 0x01) != 0) { + if (feedback.empty() == false) { + feedback += ','; + } + PluginHost::ISubSystem::subsystem element(static_cast(index)); + feedback += string(Core::EnumerateType(element).Data()); + } + delta = (delta >> 1); + index++; + } + TRACE(Activity, (_T("Delta preconditions: %s"), feedback.c_str())); + } - const string className(PluginHost::Service::Configuration().ClassName.Value()); - const string callSign(PluginHost::Service::Configuration().Callsign.Value()); + return Core::ERROR_PENDING_CONDITIONS; + } - _reason = why; + if (_lastId != 0) { + _administrator.Destroy(_lastId); + _lastId = 0; + } - SYSLOG(Logging::Shutdown, (_T("Unavailable plugin [%s]:[%s]"), className.c_str(), callSign.c_str())); + TRACE(Activity, (_T("Activation plugin [%s]:[%s]"), className.c_str(), callSign.c_str())); + return Core::ERROR_NONE; + } - TRACE(Activity, (Core::Format(_T("Unavailable plugin [%s]:[%s]"), className.c_str(), callSign.c_str()))); + Core::hresult Server::Service::InitializePlugin() + { + ASSERT(_stateMachine.IsTransitionThread()); + // Pre: no service Lock() — blocking call + const string callSign(PluginHost::Service::Configuration().Callsign.Value()); + const string className(PluginHost::Service::Configuration().ClassName.Value()); - State(UNAVAILABLE); - _administrator.Unavailable(callSign, this); + REPORT_DURATION_WARNING({ ErrorMessage(_handler->Initialize(this)); }, WarningReporting::TooLongPluginState, WarningReporting::TooLongPluginState::StateChange::ACTIVATION, callSign.c_str()); - Unlock(); + if (HasError() == true) { + SYSLOG(Logging::Startup, (_T("Activation of plugin [%s]:[%s], failed. Error [%s]"), className.c_str(), callSign.c_str(), ErrorMessage().c_str())); -#ifdef THUNDER_RESTFULL_API - Notify(EMPTY_STRING, string(_T("{\"state\":\"unavailable\",\"reason\":\"")) + textReason.Data() + _T("\"}")); -#endif - Notify(_T("statechange"), string(_T("{\"state\":\"unavailable\",\"reason\":\"")) + textReason.Data() + _T("\"}")); - } else { - Unlock(); + if (_administrator.Configuration().LegacyInitialize() == false) { + REPORT_DURATION_WARNING({ _handler->Deinitialize(this); }, WarningReporting::TooLongPluginState, WarningReporting::TooLongPluginState::StateChange::DEACTIVATION, callSign.c_str()); } - } else { - result = Core::ERROR_NOT_SUPPORTED; + + return Core::ERROR_GENERAL; } - return (result); + SYSLOG(Logging::Startup, (_T("Activated plugin [%s]:[%s]"), className.c_str(), callSign.c_str())); + return Core::ERROR_NONE; } - Core::hresult Server::Service::Hibernate(const uint32_t timeout VARIABLE_IS_NOT_USED) /* override */ { - Core::hresult result = Core::ERROR_NONE; + void Server::Service::DeinitializePlugin() + { + ASSERT(_stateMachine.IsTransitionThread()); + // Pre: no service Lock() — blocking call + const string callSign(PluginHost::Service::Configuration().Callsign.Value()); + REPORT_DURATION_WARNING({ _handler->Deinitialize(this); }, WarningReporting::TooLongPluginState, WarningReporting::TooLongPluginState::StateChange::DEACTIVATION, callSign.c_str()); + SYSLOG(Logging::Shutdown, (_T("Deactivated plugin [%s]:[%s]"), ClassName().c_str(), callSign.c_str())); + } - if (AllowedHibernate() == true) { + void Server::Service::UnloadPlugin() + { + ASSERT(_stateMachine.IsTransitionThread()); + // Pre: Lock() held by caller + ReleaseInterfaces(); + } - Lock(); + void Server::Service::Attach() + { + ASSERT(_stateMachine.IsTransitionThread()); + // Pre: no locks held + const string webUI(PluginHost::Service::Configuration().WebUI.Value()); + if ((PluginHost::Service::Configuration().WebUI.IsSet()) || (webUI.empty() == false)) { + EnableWebServer(webUI, EMPTY_STRING); + } - IShell::state currentState(State()); + if (_jsonrpc != nullptr) { + PluginHost::IShell::IConnectionServer::INotification* sink = nullptr; + _jsonrpc->Attach(sink, this); + if (sink != nullptr) { + Register(sink); + sink->Release(); + } + } - if (currentState != IShell::state::ACTIVATED) { - result = Core::ERROR_ILLEGAL_STATE; - } else if (_connection == nullptr) { - result = Core::ERROR_INPROC; - } else { - // Oke we have an Connection so there is something to Hibernate.. - RPC::IMonitorableProcess* local = _connection->QueryInterface(); + if (_external.Connector().empty() == false) { + uint32_t result = _external.Open(0); + if ((result != Core::ERROR_NONE) && (result != Core::ERROR_INPROGRESS)) { + TRACE(Trace::Error, (_T("Could not open the external connector for %s"), Callsign().c_str())); + } + } - if (local == nullptr) { - result = Core::ERROR_BAD_REQUEST; - } else { - State(IShell::HIBERNATED); -#ifdef HIBERNATE_SUPPORT_ENABLED - pid_t parentPID = local->ParentPID(); - local->Release(); - Unlock(); + _stateControl = _handler->QueryInterface(); + if (_stateControl != nullptr) { + _stateControl->Register(&_composit); + if (Resumed() == true) { + _stateControl->Request(PluginHost::IStateControl::RESUME); + } + } + } - TRACE(Activity, (_T("Hibernation of plugin [%s] process [%u]"), Callsign().c_str(), parentPID)); - result = HibernateProcess(timeout, parentPID, _administrator.Configuration().HibernateLocator().c_str(), _T(""), &_hibernateStorage); - Lock(); - if (State() != IShell::HIBERNATED) { - SYSLOG(Logging::Startup, (_T("Hibernation aborted of plugin [%s] process [%u]"), Callsign().c_str(), parentPID)); - result = Core::ERROR_ABORTED; - } - Unlock(); + void Server::Service::Detach() + { + ASSERT(_stateMachine.IsTransitionThread()); + // Pre: no locks held — reverse of Attach() + if (_stateControl != nullptr) { + _stateControl->Unregister(&_composit); + _stateControl->Release(); + _stateControl = nullptr; + } - if (result == HIBERNATE_ERROR_NONE) { - result = HibernateChildren(parentPID, timeout); - } + if ((PluginHost::Service::Configuration().WebUI.IsSet()) || (PluginHost::Service::Configuration().WebUI.Value().empty() == false)) { + DisableWebServer(); + } - if (result != Core::ERROR_NONE && result != Core::ERROR_ABORTED) { - // try to wakeup Parent process to revert Hibernation and recover - TRACE(Activity, (_T("Wakeup plugin [%s] process [%u] on Hibernate error [%d]"), Callsign().c_str(), parentPID, result)); - WakeupProcess(timeout, parentPID, _administrator.Configuration().HibernateLocator().c_str(), _T(""), &_hibernateStorage); - } + if (_external.Connector().empty() == false) { + _external.Close(0); + } - Lock(); -#else - local->Release(); - result = Core::ERROR_NONE; -#endif - // coverity[DEADCODE] - On the non-HIBERNATE_ENABLED path result is always - // ERROR_NONE here, making the else-if appear unreachable to Coverity. - // Both branches are reachable when HIBERNATE_ENABLED is defined. - if (result == Core::ERROR_NONE) { - if (State() == IShell::state::HIBERNATED) { - SYSLOG(Logging::Startup, ("Hibernated plugin [%s]:[%s]", ClassName().c_str(), Callsign().c_str())); - } else { - // wakeup occured right after hibernation finished - SYSLOG(Logging::Startup, ("Hibernation aborted of plugin [%s]:[%s]", ClassName().c_str(), Callsign().c_str())); - result = Core::ERROR_ABORTED; - } - } else if (State() == IShell::state::HIBERNATED) { - State(IShell::ACTIVATED); - SYSLOG(Logging::Startup, (_T("Hibernation error [%d] of [%s]:[%s]"), result, ClassName().c_str(), Callsign().c_str())); - } - } + if (_jsonrpc != nullptr) { + PluginHost::IShell::IConnectionServer::INotification* sink = nullptr; + _jsonrpc->Detach(sink); + if (sink != nullptr) { + Unregister(sink); + sink->Release(); } - Unlock(); - } else { - - result = Core::ERROR_NOT_SUPPORTED; } - - return (result); - } uint32_t Server::Service::Wakeup(const uint32_t timeout VARIABLE_IS_NOT_USED) { @@ -849,6 +980,11 @@ namespace PluginHost { result = Core::ERROR_NONE; #endif if (result == Core::ERROR_NONE) { + // Updates base class _state only — _current in StateMachine is NOT updated. + // Callers (HibernatedState::Activate and HibernatedState::Deactivate) are + // responsible for calling SetState(_stateActivated) immediately after Wakeup() + // returns to close the window where State() == ACTIVATED but _current still + // points to HibernatedState (which returns nullptr from QueryInterface). State(ACTIVATED); SYSLOG(Logging::Startup, (_T("Activated plugin from hibernation [%s]:[%s]"), ClassName().c_str(), Callsign().c_str())); } @@ -1214,7 +1350,7 @@ namespace PluginHost { { TRACE(Activity, (_T("Destruct a link with ID [%d] to [%s]"), Id(), RemoteId().c_str())); - // If we are still atatched to a service, detach, we are out of scope... + // If we are still attached to a service, detach, we are out of scope... CleanupService(); if (_security != nullptr) { diff --git a/Source/Thunder/PluginServer.h b/Source/Thunder/PluginServer.h index 56ec288ee..2b6cef155 100644 --- a/Source/Thunder/PluginServer.h +++ b/Source/Thunder/PluginServer.h @@ -683,6 +683,297 @@ namespace PluginHost { uint32_t _mask; state _state; }; + // ----------------------------------------------------------------------- + // StateMachine — lifecycle contract + // + // This documents the guarantees the state machine makes to plugin code, + // subscribers, and future maintainers. + // + // STATES + // Stable states (externally observable): + // DEACTIVATED — plugin is not loaded. _handler is nullptr. + // PRECONDITION — plugin is loaded but subsystem conditions not met. + // _handler exists, Initialize() has not been called. + // ACTIVATED — plugin is running. _handler, _jsonrpc, _stateControl + // are all valid. + // HIBERNATED — OOP process is frozen. Interfaces are valid but + // the process is suspended. + // UNAVAILABLE — explicitly marked not available. _handler is nullptr. + // + // Transient states (in-progress guards): + // ACTIVATION — Initialize() is running. All triggers return + // ERROR_INPROGRESS except Deactivate(INITIALIZATION_FAILED). + // DEACTIVATION — Deinitialize() is running. All triggers return + // ERROR_INPROGRESS. + // + // CALLBACK GUARANTEES + // The Callback registered at construction fires once per stable state + // transition, after the transition is complete and all locks are released. + // The state machine is in the new stable state when the callback fires. + // + // What is observable from within a callback: + // ACTIVATION fires: Initialize() has not yet been called. + // QueryInterface returns nullptr (ACTIVATION state). + // ACTIVATED fires: Initialize() completed successfully. + // Attach() completed. QueryInterface is valid. + // DEACTIVATION fires: Deinitialize() has not yet been called. + // QueryInterface is still valid (_handler alive). + // DEACTIVATED fires: Deinitialize() and UnloadPlugin() completed. + // QueryInterface returns nullptr. + // UNAVAILABLE fires: state set. No plugin lifecycle calls were made. + // + // What is legal from within a callback: + // - QueryInterface on this service: legal for ACTIVATED and DEACTIVATION + // callbacks. Returns nullptr for all other states. + // - Triggers on OTHER services: legal. Each service has its own + // _transitionLock. + // - Triggers on THIS service: illegal. _transitionLock is still held. + // Calling a public trigger from within a callback on the same thread + // fires ASSERT in debug builds and deadlocks in release builds. + // + // REENTRANCY + // Public triggers (Activate, Deactivate, Hibernate, Unavailable, + // Reevaluate) are serialized by _transitionLock. At most one transition + // executor exists per Service at any time. + // + // Internal triggers (_Activate, _Deactivate) bypass _transitionLock and + // are only legal from within an active transition (state class methods). + // ASSERT(_transitionThread == Core::Thread::ThreadId()) guards misuse. + // + // QueryInterface never holds _transitionLock and is never blocked by + // an active transition. It uses _pluginHandling only. + // + // PROCESS CRASH HANDLING + // When an OOP plugin process crashes during ACTIVATED state: + // - The RPC connection detects the disconnect and calls back into + // Service via the IConnectionServer::INotification interface. + // - Deactivate(FAILURE) is triggered, which follows the normal + // ACTIVATED -> DEACTIVATION -> DEACTIVATED path. + // - DeinitializePlugin() calls _handler->Deinitialize() on the + // now-dead proxy — this is safe because Thunder's COM-RPC layer + // handles disconnected proxies without crashing. + // - ReleaseInterfaces() terminates and releases the connection. + // - _lastId records the connection ID so the next Activate() can + // forcefully kill any zombie OOP process via _administrator.Destroy(). + // + // During ACTIVATION (crash mid-Initialize()): + // - REPORT_DURATION_WARNING detects the failure via HasError(). + // - Deactivate(INITIALIZATION_FAILED) is the expected recovery path. + // - ActivationState::Deactivate handles this as the sole legal + // deactivation from the ACTIVATION transient state. + // + // During DEACTIVATION (crash mid-Deinitialize()): + // - DeinitializePlugin() completes without crashing (dead proxy). + // - Normal teardown continues. State reaches DEACTIVATED. + // - The zombie process is cleaned up on the next Activate() via + // _administrator.Destroy(_lastId). + // ----------------------------------------------------------------------- + + // ----------------------------------------------------------------------- + // StateMachine — lock ordering + // + // Three locks are in play within Service. They must always be acquired + // in the order listed below. Acquiring in any other order is a deadlock. + // + // 1. _transitionLock (StateMachine) + // Outermost. Held for the full duration of a lifecycle transition + // including the post-transition callback. Ensures exactly one + // transition executor exists at any time. + // NOT held during QueryInterface. + // + // 2. Service::Lock() (_adminLock in PluginHost::Service base) + // Held briefly inside a transition to guard state mutation and + // subsystem bit changes. Never held across blocking plugin calls. + // + // 3. _pluginHandling (Service) + // Innermost. Held briefly in QueryInterface and ReleaseInterfaces + // to guard _handler lifetime. Never held while calling into plugin. + // + // Special cases: + // - _transitionLock is intentionally released around sm.Evaluate() + // to allow RecursiveNotification to call Reevaluate() on this + // service without deadlocking. Safe because SetState(DEACTIVATION) + // is always called before the release — the transient state rejects + // all concurrent operations. + // + // - _notificationLock (Notifiers) is independent of all three locks + // above. It is never held during foreign code — observer lists are + // snapshotted under the lock and callbacks fire after it is released. + // + // - QueryInterface never holds _transitionLock. It uses _pluginHandling + // only, and only ActivatedState::QueryInterface reaches plugin code. + // ----------------------------------------------------------------------- + class StateMachine { + public: + // Base state — default implementations return ERROR_ILLEGAL_STATE. + // Concrete states only override what is legal for them. + class StateBase { + public: + virtual ~StateBase() = default; + virtual IShell::state Id() const = 0; + virtual Core::hresult Activate(StateMachine&, const reason) { return Core::ERROR_ILLEGAL_STATE; } + virtual Core::hresult Deactivate(StateMachine&, const reason) { return Core::ERROR_ILLEGAL_STATE; } + virtual Core::hresult Hibernate(StateMachine&, const uint32_t) { return Core::ERROR_ILLEGAL_STATE; } + virtual Core::hresult Unavailable(StateMachine&, const reason) { return Core::ERROR_ILLEGAL_STATE; } + virtual void Reevaluate(StateMachine&) {} + virtual void* QueryInterface(StateMachine&, const uint32_t, const bool) { return nullptr; } + }; + + class DeactivatedState : public StateBase { + public: + IShell::state Id() const override { return IShell::DEACTIVATED; } + Core::hresult Activate(StateMachine&, const reason) override; + Core::hresult Unavailable(StateMachine&, const reason) override; + }; + + class PreconditionState : public StateBase { + public: + IShell::state Id() const override { return IShell::PRECONDITION; } + Core::hresult Deactivate(StateMachine&, const reason) override; + void Reevaluate(StateMachine&) override; + }; + + class ActivationState : public StateBase { + public: + IShell::state Id() const override { return IShell::ACTIVATION; } + Core::hresult Activate(StateMachine&, const reason) override { return Core::ERROR_INPROGRESS; } + Core::hresult Deactivate(StateMachine&, const reason) override; + Core::hresult Hibernate(StateMachine&, const uint32_t) override { return Core::ERROR_INPROGRESS; } + Core::hresult Unavailable(StateMachine&, const reason) override { return Core::ERROR_INPROGRESS; } + }; + + class ActivatedState : public StateBase { + public: + IShell::state Id() const override { return IShell::ACTIVATED; } + Core::hresult Deactivate(StateMachine&, const reason) override; + Core::hresult Hibernate(StateMachine&, const uint32_t timeout) override; + void Reevaluate(StateMachine&) override; + void* QueryInterface(StateMachine&, const uint32_t id, const bool asIUnknown) override; + }; + + class DeactivationState : public StateBase { + public: + IShell::state Id() const override { return IShell::DEACTIVATION; } + Core::hresult Activate(StateMachine&, const reason) override { return Core::ERROR_INPROGRESS; } + Core::hresult Deactivate(StateMachine&, const reason) override { return Core::ERROR_INPROGRESS; } + Core::hresult Hibernate(StateMachine&, const uint32_t) override { return Core::ERROR_INPROGRESS; } + Core::hresult Unavailable(StateMachine&, const reason) override { return Core::ERROR_INPROGRESS; } + }; + + class HibernatedState : public StateBase { + public: + IShell::state Id() const override { return IShell::HIBERNATED; } + Core::hresult Activate(StateMachine&, const reason) override; + Core::hresult Deactivate(StateMachine&, const reason) override; + }; + + class UnavailableState : public StateBase { + public: + IShell::state Id() const override { return IShell::UNAVAILABLE; } + Core::hresult Deactivate(StateMachine&, const reason) override; + }; + + public: + using Callback = std::function; + + StateMachine() = delete; + StateMachine(StateMachine&&) = delete; + StateMachine(const StateMachine&) = delete; + StateMachine& operator=(StateMachine&&) = delete; + StateMachine& operator=(const StateMachine&) = delete; + + StateMachine(Service& parent, Callback callback) + : _parent(parent) + , _callback(std::move(callback)) + , _transitionLock() + , _transitionThread(0) + , _current(&_stateDeactivated) + { + } + ~StateMachine() = default; + + public: + inline IShell::state Current() const { + return _current.load(std::memory_order_acquire)->Id(); + } + + inline void SetState(StateBase& newState) { + _current.store(&newState, std::memory_order_release); + // PluginHost::Service::State(value) is a plain _state = value assignment + // with no lock — calling this while holding _parent.Lock() is safe. + _parent.State(newState.Id()); + } + + inline bool IsTransitionThread() const { + return _transitionThread.load(std::memory_order_acquire) == Core::Thread::ThreadId(); + } + + // Public triggers — acquire transition lock before dispatching. + // Guarantees exactly one transition executor at a time. + // QueryInterface intentionally does NOT take this lock. + Core::hresult Activate(const reason why) { ASSERT(!IsTransitionThread()); Core::SafeSyncType guard(_transitionLock); _transitionThread = Core::Thread::ThreadId(); Core::hresult result = _Activate(why); _transitionThread = 0; return result; } + Core::hresult Deactivate(const reason why) { ASSERT(!IsTransitionThread()); Core::SafeSyncType guard(_transitionLock); _transitionThread = Core::Thread::ThreadId(); Core::hresult result = _Deactivate(why); _transitionThread = 0; return result; } + Core::hresult Hibernate(const uint32_t timeout) { ASSERT(!IsTransitionThread()); Core::SafeSyncType guard(_transitionLock); _transitionThread = Core::Thread::ThreadId(); Core::hresult result = _current.load(std::memory_order_acquire)->Hibernate(*this, timeout); _transitionThread = 0; return result; } + Core::hresult Unavailable(const reason why) { ASSERT(!IsTransitionThread()); Core::SafeSyncType guard(_transitionLock); _transitionThread = Core::Thread::ThreadId(); Core::hresult result = _current.load(std::memory_order_acquire)->Unavailable(*this, why); _transitionThread = 0; return result; } + void Reevaluate() { ASSERT(_transitionThread != Core::Thread::ThreadId()); Core::SafeSyncType guard(_transitionLock); _transitionThread = Core::Thread::ThreadId(); _current.load(std::memory_order_acquire)->Reevaluate(*this); _transitionThread = 0; } + void* QueryInterface(const uint32_t id, const bool asIUnknown) { return _current.load(std::memory_order_acquire)->QueryInterface(*this, id, asIUnknown); } + + // Internal triggers — no lock. Called by state class methods + // that are already executing under _transitionLock. + Core::hresult _Activate(const reason why) { ASSERT(IsTransitionThread()); return _current.load(std::memory_order_acquire)->Activate(*this, why); } + Core::hresult _Deactivate(const reason why) { ASSERT(IsTransitionThread()); return _current.load(std::memory_order_acquire)->Deactivate(*this, why); } + + // Called from within a state method to trigger cascading re-evaluation + // of dependent services. Temporarily releases _transitionLock so + // RecursiveNotification can call Reevaluate() on this service without + // deadlocking. Safe because SetState(DEACTIVATION) must be called + // before this — the transient state rejects concurrent operations. + // + // DEBT: this is controlled lock borrowing, not RAII-managed ownership. + // The ASSERT(IsTransitionThread()) guards against misuse in debug builds + // but compiles out in release. If _transitionLock is ever not held when + // this is called in release, Unlock() on an unheld mutex is UB. + // Mitigation: IsTransitionThread() is also asserted on all internal + // triggers (_Activate, _Deactivate) that are the only legal callers of + // state methods — making it structurally difficult to reach Evaluate() + // outside a transition without triggering an earlier assert. + void Evaluate() { + ASSERT(IsTransitionThread()); + _transitionThread = 0; + _transitionLock.Unlock(); + _parent._administrator.Evaluate(); + _transitionLock.Lock(); + _transitionThread = Core::Thread::ThreadId(); + } + + // Static state instances — shared across all plugins. + // Safe because state objects carry no per-plugin data. + static DeactivatedState _stateDeactivated; + static PreconditionState _statePrecondition; + static ActivationState _stateActivation; + static ActivatedState _stateActivated; + static DeactivationState _stateDeactivation; + static HibernatedState _stateHibernated; + static UnavailableState _stateUnavailable; + + // State classes access Service members via _parent (C++11 nested class access rules) + private: + Service& _parent; + Callback _callback; + mutable Core::CriticalSection _transitionLock; + // Debug: tracks which thread owns the current transition. + // ASSERT fires if a public trigger is called re-entrantly + // from within a callback or state method on the same thread. + // Atomic because the ASSERT reads it before _transitionLock + // is acquired — concurrent writes from other threads must be safe. + // Compiled out in release builds. + std::atomic _transitionThread; + // Atomic pointer — written by SetState() during transitions, + // read concurrently by QueryInterface() and trigger methods. + // Preserves State pattern dispatch without a mutex on reads. + std::atomic _current; + }; + class ControlData { public: ControlData() = delete; @@ -831,7 +1122,6 @@ namespace PluginHost { Service(const PluginHost::Config& server, const Plugin::Config& plugin, ServiceMap& administrator, const mode type, const Core::ProxyType& handler) : PluginHost::Service(plugin, server.WebPrefix(), server.PersistentPath(), server.DataPath(), server.VolatilePath()) , _pluginHandling() - , _queryInterfaceLock() , _handler(nullptr) , _extended(nullptr) , _webRequest(nullptr) @@ -854,6 +1144,38 @@ namespace PluginHost { , _composit(*this) , _jobs(administrator) , _type(type) + , _stateMachine(*this, [this](const IShell::state newState) { + const string callSign(PluginHost::Service::Configuration().Callsign.Value()); + const Core::EnumerateType textReason(_reason); + switch (newState) { + case IShell::ACTIVATED: + _administrator.Activated(callSign, this); + #ifdef THUNDER_RESTFULL_API + Notify(EMPTY_STRING, string(_T("{\"state\":\"activated\",\"reason\":\"")) + textReason.Data() + _T("\"}")); + #endif + Notify(_T("statechange"), string(_T("{\"state\":\"activated\",\"reason\":\"")) + textReason.Data() + _T("\"}")); + break; + case IShell::DEACTIVATED: + _administrator.Deinitialized(callSign, this); + #ifdef THUNDER_RESTFULL_API + Notify(EMPTY_STRING, string(_T("{\"state\":\"deactivated\",\"reason\":\"")) + textReason.Data() + _T("\"}")); + #endif + Notify(_T("statechange"), string(_T("{\"state\":\"deactivated\",\"reason\":\"")) + textReason.Data() + _T("\"}")); + break; + case IShell::PRECONDITION: + _administrator.Deinitialized(callSign, this); + break; + case IShell::UNAVAILABLE: + _administrator.Unavailable(callSign, this); + #ifdef THUNDER_RESTFULL_API + Notify(EMPTY_STRING, string(_T("{\"state\":\"unavailable\",\"reason\":\"")) + textReason.Data() + _T("\"}")); + #endif + Notify(_T("statechange"), string(_T("{\"state\":\"unavailable\",\"reason\":\"")) + textReason.Data() + _T("\"}")); + break; + default: + break; + } + }) { _jobs.Slots(_metadata.MaxRequests()); } @@ -1221,36 +1543,7 @@ namespace PluginHost { } inline void Evaluate() { - Lock(); - - uint32_t subsystems = _administrator.SubSystemInfo().Value(); - - IShell::state current(State()); - - // Active or not, update the condition state !!!! - if ((_precondition.Evaluate(subsystems) == true) && (current == IShell::PRECONDITION)) { - if (_precondition.IsMet() == true) { - - Unlock(); - - Activate(_reason); - - Lock(); - } - } - - if ((_termination.Evaluate(subsystems) == true) && (current == IShell::ACTIVATED)) { - if (_termination.IsMet() == false) { - - Unlock(); - - Deactivate(IShell::CONDITIONS); - - Lock(); - } - } - - Unlock(); + _stateMachine.Reevaluate(); } inline bool PostMortemAllowed(PluginHost::IShell::reason why) const { return (_administrator.Configuration().PostMortemAllowed(why)); @@ -1438,6 +1731,16 @@ namespace PluginHost { } private: + // Work methods called by StateMachine during transitions. + // Each does exactly one thing. No guard logic, no state changes, + // no notifications — those are all owned by StateMachine. + Core::hresult LoadPlugin(); // AcquireInterfaces + Core::hresult InitializePlugin(); // _handler->Initialize() + void DeinitializePlugin(); // _handler->Deinitialize() + void UnloadPlugin(); // ReleaseInterfaces + void Attach(); // WebServer + JSONRPC + external connector + StateControl + void Detach(); // reverse of Attach + uint32_t Wakeup(const uint32_t timeout); #ifdef HIBERNATE_SUPPORT_ENABLED @@ -1691,8 +1994,6 @@ namespace PluginHost { private: mutable Core::CriticalSection _pluginHandling; - mutable Core::CriticalSection _queryInterfaceLock; // a little shortcut to protect both the QueryInterface from Plugin deinitializing which could make the QueryInterface to the OOP part of a plugin crash (and to facilitate sending the Plugin Deactivated notifications while still allow QueryInterface to be called) - // the whole plugin state handling including locking needs a redesign... // The handlers that implement the actual logic behind the service IPlugin* _handler; @@ -1720,6 +2021,7 @@ namespace PluginHost { Core::SinkType _composit; Jobs _jobs; mode _type; + StateMachine _stateMachine; static Core::ProxyType _unavailableHandler; static Core::ProxyType _missingHandler; @@ -2766,55 +3068,89 @@ namespace PluginHost { } void Notify(const string& callsign, PluginHost::IShell* entry, Core::hresult (PluginHost::IPlugin::INotification::*notificatonmethod)(const string& callsign, IShell* plugin)) { + // Phase 1 — snapshot observer list under lock. + // Observers are AddRef'd so they stay alive across the unlock boundary. + // Foreign code never runs while _notificationLock is held. + std::vector snapshot; + _notificationLock.Lock(); + snapshot.reserve(_notifiers.size()); + for (auto& notifier : _notifiers) { + if (notifier.second.SendNotification(callsign, entry) == true) { + notifier.first->AddRef(); + snapshot.push_back(notifier.first); + } + } + _notificationLock.Unlock(); - auto it = _notifiers.begin(); - while (it != _notifiers.end()) { - if (it->second.SendNotification(callsign, entry) == true) { - Core::hresult result = (it->first->*notificatonmethod)(callsign, entry); - if (result == Core::ERROR_CANCEL) { - PluginHost::IPlugin::INotification* foundnotification = it->first; + // Phase 2 — fire callbacks outside lock. + // Observers may now safely call Unregister() or trigger transitions. + std::vector toRemove; + for (PluginHost::IPlugin::INotification* observer : snapshot) { + if ((observer->*notificatonmethod)(callsign, entry) == Core::ERROR_CANCEL) { + toRemove.push_back(observer); + } + observer->Release(); + } + + // Phase 3 — remove ERROR_CANCEL observers under lock. + // Preserves the ERROR_CANCEL API contract without holding + // _notificationLock during foreign code execution. + if (toRemove.empty() == false) { + _notificationLock.Lock(); + for (PluginHost::IPlugin::INotification* observer : toRemove) { + auto range = _notifiers.equal_range(observer); + for (auto it = range.first; it != range.second; ++it) { it = _notifiers.erase(it); - foundnotification->Release(); - foundnotification = nullptr; - } else { - ++it; } - } else { - ++it; + observer->Release(); } + _notificationLock.Unlock(); } - - _notificationLock.Unlock(); } void Notify(const string& callsign, PluginHost::IShell* entry, void (PluginHost::IPlugin::ILifeTime::*notificatonmethod)(const string& callsign, IShell* plugin)) const { - _notificationLock.Lock(); + // Phase 1 — snapshot under lock. + std::vector snapshot; + _notificationLock.Lock(); + snapshot.reserve(_notifiers.size()); for (const auto& notifier : _notifiers) { if (notifier.second.SendNotification(callsign, entry) == true) { PluginHost::IPlugin::ILifeTime* lifeTime = notifier.first->QueryInterface(); if (lifeTime != nullptr) { - (lifeTime->*notificatonmethod)(callsign, entry); - lifeTime->Release(); + snapshot.push_back(lifeTime); } } } - _notificationLock.Unlock(); + + // Phase 2 — fire outside lock. ILifeTime has no ERROR_CANCEL — no removal needed. + for (PluginHost::IPlugin::ILifeTime* lifeTime : snapshot) { + (lifeTime->*notificatonmethod)(callsign, entry); + lifeTime->Release(); + } } template void Visit(FUNCTION function) const { - _notificationLock.Lock(); + // Snapshot under lock — function may call back into ServiceMap. + std::vector snapshot; + _notificationLock.Lock(); + snapshot.reserve(_notifiers.size()); for (const auto& notifier : _notifiers) { - function(notifier.first); + notifier.first->AddRef(); + snapshot.push_back(notifier.first); } - _notificationLock.Unlock(); + + for (PluginHost::IPlugin::INotification* observer : snapshot) { + function(observer); + observer->Release(); + } } private: From 1eb1ac30e420f59f18f1a3fc15d06f5dbf085c4c Mon Sep 17 00:00:00 2001 From: Bram Oosterhuis Date: Wed, 13 May 2026 16:29:05 +0200 Subject: [PATCH 02/26] Use RAII to safely yield transition ownership in Evaluate() --- Source/Thunder/PluginServer.h | 44 +++++++++++++++++++++-------------- 1 file changed, 27 insertions(+), 17 deletions(-) diff --git a/Source/Thunder/PluginServer.h b/Source/Thunder/PluginServer.h index 2b6cef155..6bdb8b70f 100644 --- a/Source/Thunder/PluginServer.h +++ b/Source/Thunder/PluginServer.h @@ -923,27 +923,37 @@ namespace PluginHost { Core::hresult _Activate(const reason why) { ASSERT(IsTransitionThread()); return _current.load(std::memory_order_acquire)->Activate(*this, why); } Core::hresult _Deactivate(const reason why) { ASSERT(IsTransitionThread()); return _current.load(std::memory_order_acquire)->Deactivate(*this, why); } + struct TransitionYield { + TransitionYield() = delete; + TransitionYield(const TransitionYield&) = delete; + TransitionYield(TransitionYield&&) = delete; + TransitionYield& operator=(const TransitionYield&) = delete; + TransitionYield& operator=(TransitionYield&&) = delete; + + explicit TransitionYield(StateMachine& sm) + : _sm(sm) + { + _sm._transitionThread = 0; + _sm._transitionLock.Unlock(); + } + ~TransitionYield() + { + _sm._transitionLock.Lock(); + _sm._transitionThread = Core::Thread::ThreadId(); + } + StateMachine& _sm; + }; + // Called from within a state method to trigger cascading re-evaluation - // of dependent services. Temporarily releases _transitionLock so - // RecursiveNotification can call Reevaluate() on this service without - // deadlocking. Safe because SetState(DEACTIVATION) must be called - // before this — the transient state rejects concurrent operations. - // - // DEBT: this is controlled lock borrowing, not RAII-managed ownership. - // The ASSERT(IsTransitionThread()) guards against misuse in debug builds - // but compiles out in release. If _transitionLock is ever not held when - // this is called in release, Unlock() on an unheld mutex is UB. - // Mitigation: IsTransitionThread() is also asserted on all internal - // triggers (_Activate, _Deactivate) that are the only legal callers of - // state methods — making it structurally difficult to reach Evaluate() - // outside a transition without triggering an earlier assert. + // of dependent services. Temporarily yields _transitionLock via + // TransitionYield so RecursiveNotification can call Reevaluate() on + // this service without deadlocking. Safe because SetState(DEACTIVATION) + // must be called before this — the transient state rejects all + // concurrent operations during the yield window. void Evaluate() { ASSERT(IsTransitionThread()); - _transitionThread = 0; - _transitionLock.Unlock(); + TransitionYield yield(*this); _parent._administrator.Evaluate(); - _transitionLock.Lock(); - _transitionThread = Core::Thread::ThreadId(); } // Static state instances — shared across all plugins. From fb28577e4bcd46788e5a73ac5539f88e31bda6f1 Mon Sep 17 00:00:00 2001 From: Bram Oosterhuis Date: Tue, 9 Jun 2026 21:38:43 +0200 Subject: [PATCH 03/26] refactor(pluginserver): replace unsafe TransitionYield with RAII TransitionSuspender Replaced the manual lock management inside `TransitionYield` with a private, nested RAII class `TransitionSuspender` within `StateMachine`. - Eliminates potential Undefined Behavior (UB) in Release builds where `Unlock()` could accidentally be called on an unheld mutex when assertions are disabled. - Guarantees proper scope-based lock re-acquisition and thread-ID tracking upon leaving `Evaluate()`, ensuring state machine consistency. - Enhances encapsulation by hiding internal transition-locking mechanics from the outside world. --- Source/Thunder/PluginServer.h | 49 ++++++++++++++++++----------------- 1 file changed, 25 insertions(+), 24 deletions(-) diff --git a/Source/Thunder/PluginServer.h b/Source/Thunder/PluginServer.h index 699a045a7..360eebfde 100644 --- a/Source/Thunder/PluginServer.h +++ b/Source/Thunder/PluginServer.h @@ -873,6 +873,27 @@ namespace PluginHost { Core::hresult Deactivate(StateMachine&, const reason) override; }; + private: + class TransitionSuspender { + public: + TransitionSuspender(Core::CriticalSection& lock, std::atomic& transitionThread) + : _lock(lock) + , _transitionThread(transitionThread) + { + _transitionThread = 0; + _lock.Unlock(); + } + + ~TransitionSuspender() { + _lock.Lock(); + _transitionThread = Core::Thread::ThreadId(); + } + + private: + Core::CriticalSection& _lock; + std::atomic& _transitionThread; + }; + public: using Callback = std::function; @@ -918,41 +939,21 @@ namespace PluginHost { void Reevaluate() { ASSERT(_transitionThread != Core::Thread::ThreadId()); Core::SafeSyncType guard(_transitionLock); _transitionThread = Core::Thread::ThreadId(); _current.load(std::memory_order_acquire)->Reevaluate(*this); _transitionThread = 0; } void* QueryInterface(const uint32_t id, const bool asIUnknown) { return _current.load(std::memory_order_acquire)->QueryInterface(*this, id, asIUnknown); } + private: // Internal triggers — no lock. Called by state class methods // that are already executing under _transitionLock. Core::hresult _Activate(const reason why) { ASSERT(IsTransitionThread()); return _current.load(std::memory_order_acquire)->Activate(*this, why); } Core::hresult _Deactivate(const reason why) { ASSERT(IsTransitionThread()); return _current.load(std::memory_order_acquire)->Deactivate(*this, why); } - struct TransitionYield { - TransitionYield() = delete; - TransitionYield(const TransitionYield&) = delete; - TransitionYield(TransitionYield&&) = delete; - TransitionYield& operator=(const TransitionYield&) = delete; - TransitionYield& operator=(TransitionYield&&) = delete; - - explicit TransitionYield(StateMachine& sm) - : _sm(sm) - { - _sm._transitionThread = 0; - _sm._transitionLock.Unlock(); - } - ~TransitionYield() - { - _sm._transitionLock.Lock(); - _sm._transitionThread = Core::Thread::ThreadId(); - } - StateMachine& _sm; - }; - // Called from within a state method to trigger cascading re-evaluation // of dependent services. Temporarily yields _transitionLock via - // TransitionYield so RecursiveNotification can call Reevaluate() on + // TransitionSuspender so RecursiveNotification can call Reevaluate() on // this service without deadlocking. Safe because SetState(DEACTIVATION) // must be called before this — the transient state rejects all - // concurrent operations during the yield window. + // concurrent operations during the yield window void Evaluate() { ASSERT(IsTransitionThread()); - TransitionYield yield(*this); + TransitionSuspender suspend(_transitionLock, _transitionThread); _parent._administrator.Evaluate(); } From 3376a7503f061bdc0ad3c3890d8db599df13f9b4 Mon Sep 17 00:00:00 2001 From: Bram Oosterhuis Date: Tue, 9 Jun 2026 23:55:50 +0200 Subject: [PATCH 04/26] refactor: integrate Suspend and Resume fully into StateMachine Move Suspend and Resume handling completely into the StateMachine to ensure proper lifecycle tracking and robust thread synchronization. - Eliminate fragile cross-state delegation from PreconditionState by introducing stateless worker methods RequestSuspend() and RequestResume() on the Service class. - Fix potential distributed deadlocks and threadpool starvation by ensuring Service::Lock() is released BEFORE invoking the blocking out-of-process COM/RPC _stateControl->Request() calls in both suspend and resume flows. - Protect _stateControl lifecycle during RPC calls by capturing a local pointer copy under a short-lived Service::Lock() within the transition thread. - Prevent ERROR_ILLEGAL_STATE regressions by explicitly overriding Resume and Suspend in PreconditionState, ensuring symmetric lifecycle state transitions. - Resolve reentrancy/ASSERT risks in DeactivatedState::Resume by invoking the internal _Activate() trigger directly, keeping the operation safe within the existing transition thread context without re-acquiring _transitionLock. - Enforce architectural invariants by adding ASSERT(IsTransitionThread()) guards to the new worker methods. --- Source/Thunder/PluginServer.cpp | 163 ++++++++++++++++++-------------- Source/Thunder/PluginServer.h | 21 +++- 2 files changed, 111 insertions(+), 73 deletions(-) diff --git a/Source/Thunder/PluginServer.cpp b/Source/Thunder/PluginServer.cpp index 10ec5ed4c..aa4111e83 100644 --- a/Source/Thunder/PluginServer.cpp +++ b/Source/Thunder/PluginServer.cpp @@ -332,47 +332,9 @@ namespace PluginHost { return _stateMachine.Activate(why); } - uint32_t Server::Service::Resume(const reason why) /* override */ { - uint32_t result = Core::ERROR_NONE; - - // Read state snapshot and release Lock() before calling Activate(). - // Holding Lock() across Activate() inverts the lock order: - // Resume: Lock() → _transitionLock (via Activate) - // Transitions: _transitionLock → Lock() - // That inversion is a deadlock. The snapshot is acceptable — Resume() - // was already operating on a snapshot since state could change between - // the check and the Activate() call anyway. - Lock(); - IShell::state currentState(State()); - Unlock(); - - if (currentState == IShell::state::ACTIVATION) { - result = Core::ERROR_INPROGRESS; - } else if ((currentState == IShell::state::DEACTIVATION) || (currentState == IShell::state::DESTROYED) || (currentState == IShell::state::HIBERNATED)) { - result = Core::ERROR_ILLEGAL_STATE; - } else if (currentState == IShell::state::DEACTIVATED) { - result = Activate(why); - Lock(); - currentState = State(); - Unlock(); - } - - if (currentState == IShell::ACTIVATED) { - Lock(); - // See if we need can and should RESUME. - if (_stateControl == nullptr) { - result = Core::ERROR_BAD_REQUEST; - } - else { - // We have a StateControl interface, so at least start resuming, if not already resumed :-) - if (_stateControl->State() == PluginHost::IStateControl::SUSPENDED) { - result = _stateControl->Request(PluginHost::IStateControl::RESUME); - } - } - Unlock(); - } - - return (result); + uint32_t Server::Service::Resume(const reason why) /* override */ + { + return _stateMachine.Resume(why); } Core::hresult Server::Service::Deactivate(const reason why) /* override */ @@ -380,47 +342,26 @@ namespace PluginHost { return _stateMachine.Deactivate(why); } - uint32_t Server::Service::Suspend(const reason why) { - + uint32_t Server::Service::Suspend(const reason why) /* override */ + { uint32_t result = Core::ERROR_NONE; if (StartMode() == PluginHost::IShell::startmode::DEACTIVATED) { - // We need to shutdown completely - result = Deactivate(why); - } - else { - Lock(); - - IShell::state currentState(State()); - - if (currentState == IShell::state::DEACTIVATION) { - result = Core::ERROR_INPROGRESS; - } else if ((currentState == IShell::state::ACTIVATION) || (currentState == IShell::state::DESTROYED) || (currentState == IShell::state::HIBERNATED)) { - result = Core::ERROR_ILLEGAL_STATE; - } else if ((currentState == IShell::state::ACTIVATED) || (currentState == IShell::state::PRECONDITION)) { - // See if we need can and should SUSPEND. - if (_stateControl == nullptr) { - result = Core::ERROR_BAD_REQUEST; - } - else { - // We have a StateControl interface, so at least start suspending, if not already suspended :-) - if (_stateControl->State() == PluginHost::IStateControl::RESUMED) { - result = _stateControl->Request(PluginHost::IStateControl::SUSPEND); - } - } - } - - Unlock(); + result = _stateMachine.Deactivate(why); + } else { + result = _stateMachine.Suspend(why); } return (result); } - Core::hresult Server::Service::Unavailable(const reason why) /* override */ { + Core::hresult Server::Service::Unavailable(const reason why) /* override */ + { return _stateMachine.Unavailable(why); } - Core::hresult Server::Service::Hibernate(const uint32_t timeout VARIABLE_IS_NOT_USED) /* override */ { + Core::hresult Server::Service::Hibernate(const uint32_t timeout VARIABLE_IS_NOT_USED) /* override */ + { return _stateMachine.Hibernate(timeout); } @@ -506,6 +447,16 @@ namespace PluginHost { return Core::ERROR_NONE; } + uint32_t Server::Service::StateMachine::DeactivatedState::Resume(StateMachine & sm, const reason why) + { + Core::hresult result = sm._Activate(why); + if (result != Core::ERROR_NONE) { + return result; + } + + return sm._current.load(std::memory_order_relaxed)->Resume(sm, why); + } + // ------------------------------------------------------------------------- // PreconditionState // ------------------------------------------------------------------------- @@ -524,6 +475,16 @@ namespace PluginHost { return Core::ERROR_NONE; } + uint32_t Server::Service::StateMachine::PreconditionState::Resume(StateMachine & sm, const reason /* why */) + { + return sm._parent.RequestResume(); + } + + uint32_t Server::Service::StateMachine::PreconditionState::Suspend(StateMachine& sm, const reason /* why */) + { + return sm._parent.RequestSuspend(); + } + void Server::Service::StateMachine::PreconditionState::Reevaluate(StateMachine& sm) { sm._parent.Lock(); @@ -700,6 +661,16 @@ namespace PluginHost { return result; } + uint32_t Server::Service::StateMachine::ActivatedState::Resume(StateMachine & sm, const reason why VARIABLE_IS_NOT_USED) + { + return sm._parent.RequestResume(); + } + + uint32_t Server::Service::StateMachine::ActivatedState::Suspend(StateMachine & sm, const reason /* why */) + { + return sm._parent.RequestSuspend(); + } + void Server::Service::StateMachine::ActivatedState::Reevaluate(StateMachine& sm) { sm._parent.Lock(); @@ -950,6 +921,54 @@ namespace PluginHost { } } + uint32_t Server::Service::RequestResume() + { + ASSERT(_stateMachine.IsTransitionThread()); + + uint32_t result = Core::ERROR_NONE; + PluginHost::IStateControl* stateControl = nullptr; + + Lock(); + if (_stateControl == nullptr) { + result = Core::ERROR_BAD_REQUEST; + } else { + stateControl = _stateControl; + } + Unlock(); + + if (stateControl != nullptr) { + if (stateControl->State() == PluginHost::IStateControl::SUSPENDED) { + result = stateControl->Request(PluginHost::IStateControl::RESUME); + } + } + + return result; + } + + uint32_t Server::Service::RequestSuspend() + { + ASSERT(_stateMachine.IsTransitionThread()); + + uint32_t result = Core::ERROR_NONE; + PluginHost::IStateControl* stateControl = nullptr; + + Lock(); + if (_stateControl == nullptr) { + result = Core::ERROR_BAD_REQUEST; + } else { + stateControl = _stateControl; + } + Unlock(); + + if (stateControl != nullptr) { + if (stateControl->State() == PluginHost::IStateControl::RESUMED) { + result = stateControl->Request(PluginHost::IStateControl::SUSPEND); + } + } + + return result; + } + uint32_t Server::Service::Wakeup(const uint32_t timeout VARIABLE_IS_NOT_USED) { Core::hresult result = Core::ERROR_NONE; @@ -1572,4 +1591,4 @@ namespace PluginHost { } } -} +} \ No newline at end of file diff --git a/Source/Thunder/PluginServer.h b/Source/Thunder/PluginServer.h index 360eebfde..5893d1419 100644 --- a/Source/Thunder/PluginServer.h +++ b/Source/Thunder/PluginServer.h @@ -815,6 +815,8 @@ namespace PluginHost { virtual Core::hresult Deactivate(StateMachine&, const reason) { return Core::ERROR_ILLEGAL_STATE; } virtual Core::hresult Hibernate(StateMachine&, const uint32_t) { return Core::ERROR_ILLEGAL_STATE; } virtual Core::hresult Unavailable(StateMachine&, const reason) { return Core::ERROR_ILLEGAL_STATE; } + virtual uint32_t Resume(StateMachine&, const reason) { return Core::ERROR_ILLEGAL_STATE; } + virtual uint32_t Suspend(StateMachine&, const reason) { return Core::ERROR_ILLEGAL_STATE; } virtual void Reevaluate(StateMachine&) {} virtual void* QueryInterface(StateMachine&, const uint32_t, const bool) { return nullptr; } }; @@ -824,12 +826,15 @@ namespace PluginHost { IShell::state Id() const override { return IShell::DEACTIVATED; } Core::hresult Activate(StateMachine&, const reason) override; Core::hresult Unavailable(StateMachine&, const reason) override; + uint32_t Resume(StateMachine&, const reason) override; }; class PreconditionState : public StateBase { public: IShell::state Id() const override { return IShell::PRECONDITION; } Core::hresult Deactivate(StateMachine&, const reason) override; + uint32_t Resume(StateMachine&, const reason) override; + uint32_t Suspend(StateMachine&, const reason) override; void Reevaluate(StateMachine&) override; }; @@ -840,6 +845,7 @@ namespace PluginHost { Core::hresult Deactivate(StateMachine&, const reason) override; Core::hresult Hibernate(StateMachine&, const uint32_t) override { return Core::ERROR_INPROGRESS; } Core::hresult Unavailable(StateMachine&, const reason) override { return Core::ERROR_INPROGRESS; } + uint32_t Resume(StateMachine&, const reason) override { return Core::ERROR_INPROGRESS; } }; class ActivatedState : public StateBase { @@ -847,6 +853,8 @@ namespace PluginHost { IShell::state Id() const override { return IShell::ACTIVATED; } Core::hresult Deactivate(StateMachine&, const reason) override; Core::hresult Hibernate(StateMachine&, const uint32_t timeout) override; + uint32_t Resume(StateMachine&, const reason) override; + uint32_t Suspend(StateMachine&, const reason) override; void Reevaluate(StateMachine&) override; void* QueryInterface(StateMachine&, const uint32_t id, const bool asIUnknown) override; }; @@ -858,6 +866,7 @@ namespace PluginHost { Core::hresult Deactivate(StateMachine&, const reason) override { return Core::ERROR_INPROGRESS; } Core::hresult Hibernate(StateMachine&, const uint32_t) override { return Core::ERROR_INPROGRESS; } Core::hresult Unavailable(StateMachine&, const reason) override { return Core::ERROR_INPROGRESS; } + uint32_t Suspend(StateMachine&, const reason) override { return Core::ERROR_INPROGRESS; } }; class HibernatedState : public StateBase { @@ -876,6 +885,12 @@ namespace PluginHost { private: class TransitionSuspender { public: + TransitionSuspender() = delete; + TransitionSuspender(const TransitionSuspender&) = delete; + TransitionSuspender(TransitionSuspender&&) = delete; + TransitionSuspender& operator=(const TransitionSuspender&) = delete; + TransitionSuspender& operator=(TransitionSuspender&&) = delete; + TransitionSuspender(Core::CriticalSection& lock, std::atomic& transitionThread) : _lock(lock) , _transitionThread(transitionThread) @@ -936,7 +951,9 @@ namespace PluginHost { Core::hresult Deactivate(const reason why) { ASSERT(!IsTransitionThread()); Core::SafeSyncType guard(_transitionLock); _transitionThread = Core::Thread::ThreadId(); Core::hresult result = _Deactivate(why); _transitionThread = 0; return result; } Core::hresult Hibernate(const uint32_t timeout) { ASSERT(!IsTransitionThread()); Core::SafeSyncType guard(_transitionLock); _transitionThread = Core::Thread::ThreadId(); Core::hresult result = _current.load(std::memory_order_acquire)->Hibernate(*this, timeout); _transitionThread = 0; return result; } Core::hresult Unavailable(const reason why) { ASSERT(!IsTransitionThread()); Core::SafeSyncType guard(_transitionLock); _transitionThread = Core::Thread::ThreadId(); Core::hresult result = _current.load(std::memory_order_acquire)->Unavailable(*this, why); _transitionThread = 0; return result; } - void Reevaluate() { ASSERT(_transitionThread != Core::Thread::ThreadId()); Core::SafeSyncType guard(_transitionLock); _transitionThread = Core::Thread::ThreadId(); _current.load(std::memory_order_acquire)->Reevaluate(*this); _transitionThread = 0; } + uint32_t Resume(const reason why) { ASSERT(!IsTransitionThread()); Core::SafeSyncType guard(_transitionLock); _transitionThread = Core::Thread::ThreadId(); uint32_t result = _current.load(std::memory_order_acquire)->Resume(*this, why); _transitionThread = 0; return result; } + uint32_t Suspend(const reason why) { ASSERT(!IsTransitionThread()); Core::SafeSyncType guard(_transitionLock); _transitionThread = Core::Thread::ThreadId(); uint32_t result = _current.load(std::memory_order_acquire)->Suspend(*this, why); _transitionThread = 0; return (result); } + void Reevaluate() { ASSERT(_transitionThread != Core::Thread::ThreadId()); Core::SafeSyncType guard(_transitionLock); _transitionThread = Core::Thread::ThreadId(); _current.load(std::memory_order_acquire)->Reevaluate(*this); _transitionThread = 0; } void* QueryInterface(const uint32_t id, const bool asIUnknown) { return _current.load(std::memory_order_acquire)->QueryInterface(*this, id, asIUnknown); } private: @@ -1751,6 +1768,8 @@ namespace PluginHost { void UnloadPlugin(); // ReleaseInterfaces void Attach(); // WebServer + JSONRPC + external connector + StateControl void Detach(); // reverse of Attach + uint32_t RequestSuspend(); // _stateControl->Request(SUSPEND) if RESUMED + uint32_t RequestResume(); // _stateControl->Request(RESUME) if SUSPENDED uint32_t Wakeup(const uint32_t timeout); From 78c9f80a6c1cd205fc7bb32d687cdd40852e88ba Mon Sep 17 00:00:00 2001 From: Bram Oosterhuis Date: Wed, 10 Jun 2026 00:41:21 +0200 Subject: [PATCH 05/26] refactor(server): add DestroyedState tombstone to fix lifecycle race Introduce a 'DestroyedState' tombstone to the StateMachine to resolve the lifecycle divergence where services could be destroyed while in an active state. - Implement StateMachine::Tombstone() to perform a lock-safe, atomic transition to the DestroyedState. - Ensure that Destroy() properly awaits in-flight transitions by acquiring the _transitionLock before tombstoning. - Guarantee consistent state synchronization between the internal state machine and the base class enum. - Ensure all service entry points (QueryInterface, Activate, etc.) return safe error codes immediately after destruction. --- Source/Thunder/PluginServer.cpp | 3 ++- Source/Thunder/PluginServer.h | 19 ++++++++++++------- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/Source/Thunder/PluginServer.cpp b/Source/Thunder/PluginServer.cpp index aa4111e83..c75d10331 100644 --- a/Source/Thunder/PluginServer.cpp +++ b/Source/Thunder/PluginServer.cpp @@ -376,6 +376,7 @@ namespace PluginHost { Server::Service::StateMachine::DeactivationState Server::Service::StateMachine::_stateDeactivation; Server::Service::StateMachine::HibernatedState Server::Service::StateMachine::_stateHibernated; Server::Service::StateMachine::UnavailableState Server::Service::StateMachine::_stateUnavailable; + Server::Service::StateMachine::DestroyedState Server::Service::StateMachine::_stateDestroyed; // ------------------------------------------------------------------------- // DeactivatedState @@ -1591,4 +1592,4 @@ namespace PluginHost { } } -} \ No newline at end of file +} diff --git a/Source/Thunder/PluginServer.h b/Source/Thunder/PluginServer.h index 5893d1419..236cb7496 100644 --- a/Source/Thunder/PluginServer.h +++ b/Source/Thunder/PluginServer.h @@ -882,6 +882,12 @@ namespace PluginHost { Core::hresult Deactivate(StateMachine&, const reason) override; }; + class DestroyedState : public StateBase { + public: + IShell::state Id() const override { return IShell::DESTROYED; } + // All triggers inherit ERROR_ILLEGAL_STATE / nullptr from StateBase. + }; + private: class TransitionSuspender { public: @@ -955,6 +961,7 @@ namespace PluginHost { uint32_t Suspend(const reason why) { ASSERT(!IsTransitionThread()); Core::SafeSyncType guard(_transitionLock); _transitionThread = Core::Thread::ThreadId(); uint32_t result = _current.load(std::memory_order_acquire)->Suspend(*this, why); _transitionThread = 0; return (result); } void Reevaluate() { ASSERT(_transitionThread != Core::Thread::ThreadId()); Core::SafeSyncType guard(_transitionLock); _transitionThread = Core::Thread::ThreadId(); _current.load(std::memory_order_acquire)->Reevaluate(*this); _transitionThread = 0; } void* QueryInterface(const uint32_t id, const bool asIUnknown) { return _current.load(std::memory_order_acquire)->QueryInterface(*this, id, asIUnknown); } + void Tombstone() { Core::SafeSyncType guard(_transitionLock); _parent.Lock(); SetState(_stateDestroyed); _parent.Unlock(); } private: // Internal triggers — no lock. Called by state class methods @@ -983,6 +990,7 @@ namespace PluginHost { static DeactivationState _stateDeactivation; static HibernatedState _stateHibernated; static UnavailableState _stateUnavailable; + static DestroyedState _stateDestroyed; // State classes access Service members via _parent (C++11 nested class access rules) private: @@ -1305,13 +1313,10 @@ namespace PluginHost { } void Destroy() { - Lock(); - - // It's reference counted, so just take it out of the list, state to DESTROYED - // Also unsubscribe all subscribers. They need to go.. - State(DESTROYED); - - Unlock(); + // Tombstone the state machine first — waits for any in-flight transition + // to complete then atomically switches _current and _state to DESTROYED. + // After this returns, all triggers return ERROR_ILLEGAL_STATE / nullptr. + _stateMachine.Tombstone(); } // The service might be still alive and refered to in the request/links but they will From b23c936f269f1f595b13f4e5ad2fa81fe4e634eb Mon Sep 17 00:00:00 2001 From: Bram Oosterhuis Date: Wed, 10 Jun 2026 09:39:01 +0200 Subject: [PATCH 06/26] use memory_order_acquire for _current load in DeactivatedState::Resume Enforce consistency by upgrading the atomic load of '_current' from relaxed to acquire before dispatching the Resume trigger. - Eliminate an architectural inconsistency that was misleading to readers, bringing Resume inline with Suspend, Reevaluate, and QueryInterface. - Note: The relaxed ordering was technically thread-safe because this load always follows a release store on the exact same transition thread, but explicit acquire semantics improve code readability and maintainability. --- Source/Thunder/PluginServer.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Source/Thunder/PluginServer.cpp b/Source/Thunder/PluginServer.cpp index c75d10331..151aa6ce0 100644 --- a/Source/Thunder/PluginServer.cpp +++ b/Source/Thunder/PluginServer.cpp @@ -455,7 +455,7 @@ namespace PluginHost { return result; } - return sm._current.load(std::memory_order_relaxed)->Resume(sm, why); + return sm._current.load(std::memory_order_acquire)->Resume(sm, why); } // ------------------------------------------------------------------------- From 1e91cf0d6b7b6d0b2fbc3d9c88d1d53508c3cc05 Mon Sep 17 00:00:00 2001 From: Bram Oosterhuis Date: Wed, 10 Jun 2026 09:40:45 +0200 Subject: [PATCH 07/26] Add ASSERT to Tombstone() enforcing DEACTIVATED precondition MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Destroy() is a purely administrative operation — it removes the service from the map without deactivating it. The caller is responsible for deactivating the plugin first. Make that precondition explicit so violations are caught immediately in debug builds. --- Source/Thunder/PluginServer.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Source/Thunder/PluginServer.h b/Source/Thunder/PluginServer.h index 236cb7496..79f517a3d 100644 --- a/Source/Thunder/PluginServer.h +++ b/Source/Thunder/PluginServer.h @@ -961,7 +961,7 @@ namespace PluginHost { uint32_t Suspend(const reason why) { ASSERT(!IsTransitionThread()); Core::SafeSyncType guard(_transitionLock); _transitionThread = Core::Thread::ThreadId(); uint32_t result = _current.load(std::memory_order_acquire)->Suspend(*this, why); _transitionThread = 0; return (result); } void Reevaluate() { ASSERT(_transitionThread != Core::Thread::ThreadId()); Core::SafeSyncType guard(_transitionLock); _transitionThread = Core::Thread::ThreadId(); _current.load(std::memory_order_acquire)->Reevaluate(*this); _transitionThread = 0; } void* QueryInterface(const uint32_t id, const bool asIUnknown) { return _current.load(std::memory_order_acquire)->QueryInterface(*this, id, asIUnknown); } - void Tombstone() { Core::SafeSyncType guard(_transitionLock); _parent.Lock(); SetState(_stateDestroyed); _parent.Unlock(); } + void Tombstone() { Core::SafeSyncType guard(_transitionLock); ASSERT(_current.load(std::memory_order_acquire) == &_stateDeactivated); _parent.Lock(); SetState(_stateDestroyed); _parent.Unlock(); } private: // Internal triggers — no lock. Called by state class methods From 66014ae0c7115801740f4062fa1417a9fbb5f788 Mon Sep 17 00:00:00 2001 From: Bram Oosterhuis Date: Wed, 10 Jun 2026 09:43:02 +0200 Subject: [PATCH 08/26] Update StateMachine documentation to reflect current design - Add DESTROYED to the STATES section including tombstone precondition - Add Resume and Suspend to the public triggers list in REENTRANCY - Add ServiceMap::_adminLock as level 0 in the lock ordering, above _transitionLock, documenting the hold during Tombstone() - Remove stale reference to TransitionYield, replaced by TransitionSuspender --- Source/Thunder/PluginServer.h | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/Source/Thunder/PluginServer.h b/Source/Thunder/PluginServer.h index 79f517a3d..a3c2c9051 100644 --- a/Source/Thunder/PluginServer.h +++ b/Source/Thunder/PluginServer.h @@ -699,6 +699,10 @@ namespace PluginHost { // HIBERNATED — OOP process is frozen. Interfaces are valid but // the process is suspended. // UNAVAILABLE — explicitly marked not available. _handler is nullptr. + // DESTROYED — terminal tombstone state. All triggers return + // ERROR_ILLEGAL_STATE. QueryInterface returns nullptr. + // Reached via Tombstone() which requires the service to + // be DEACTIVATED first — enforced by ASSERT in debug builds. // // Transient states (in-progress guards): // ACTIVATION — Initialize() is running. All triggers return @@ -733,8 +737,8 @@ namespace PluginHost { // // REENTRANCY // Public triggers (Activate, Deactivate, Hibernate, Unavailable, - // Reevaluate) are serialized by _transitionLock. At most one transition - // executor exists per Service at any time. + // Resume, Suspend, Reevaluate) are serialized by _transitionLock. + // At most one transition executor exists per Service at any time. // // Internal triggers (_Activate, _Deactivate) bypass _transitionLock and // are only legal from within an active transition (state class methods). @@ -775,10 +779,14 @@ namespace PluginHost { // Three locks are in play within Service. They must always be acquired // in the order listed below. Acquiring in any other order is a deadlock. // + // 0. ServiceMap::_adminLock + // Outermost of all. Held by ServiceMap::Destroy() across the + // Tombstone() call. Never acquired from within a transition. + // // 1. _transitionLock (StateMachine) - // Outermost. Held for the full duration of a lifecycle transition - // including the post-transition callback. Ensures exactly one - // transition executor exists at any time. + // Held for the full duration of a lifecycle transition including + // the post-transition callback. Ensures exactly one transition + // executor exists at any time. // NOT held during QueryInterface. // // 2. Service::Lock() (_adminLock in PluginHost::Service base) From fa15ec57483935bb422ee832b7bbc577123fdec8 Mon Sep 17 00:00:00 2001 From: Bram Oosterhuis Date: Wed, 10 Jun 2026 09:43:50 +0200 Subject: [PATCH 09/26] Remove stale state guard and State(ACTIVATED) from Wakeup() Both were left over from the original monolithic Activate/Deactivate methods. Wakeup() is now only reachable via HibernatedState, making the HIBERNATED guard dead code. State(ACTIVATED) was redundant since both callers already call SetState(_stateActivated) after Wakeup() returns. Add ASSERT guards consistent with other work methods. --- Source/Thunder/PluginServer.cpp | 49 ++++++++++++--------------------- 1 file changed, 18 insertions(+), 31 deletions(-) diff --git a/Source/Thunder/PluginServer.cpp b/Source/Thunder/PluginServer.cpp index 151aa6ce0..ae065803a 100644 --- a/Source/Thunder/PluginServer.cpp +++ b/Source/Thunder/PluginServer.cpp @@ -970,49 +970,36 @@ namespace PluginHost { return result; } - uint32_t Server::Service::Wakeup(const uint32_t timeout VARIABLE_IS_NOT_USED) { + uint32_t Server::Service::Wakeup(const uint32_t timeout VARIABLE_IS_NOT_USED) + { + ASSERT(_stateMachine.IsTransitionThread()); + ASSERT(_connection != nullptr); + Core::hresult result = Core::ERROR_NONE; - IShell::state currentState(State()); + RPC::IMonitorableProcess* local = _connection->QueryInterface(); - if (currentState != IShell::state::HIBERNATED) { - result = Core::ERROR_ILLEGAL_STATE; + if (local == nullptr) { + result = Core::ERROR_BAD_REQUEST; } else { - ASSERT(_connection != nullptr); - - // Oke we have an Connection so there is something to Wakeup.. - RPC::IMonitorableProcess* local = _connection->QueryInterface< RPC::IMonitorableProcess>(); - - if (local == nullptr) { - result = Core::ERROR_BAD_REQUEST; - } - else { #ifdef HIBERNATE_SUPPORT_ENABLED - pid_t parentPID = local->ParentPID(); + pid_t parentPID = local->ParentPID(); - // There is no recovery path while doing Wakeup, don't care about errors - WakeupChildren(parentPID, timeout); + // There is no recovery path while doing Wakeup, don't care about errors + WakeupChildren(parentPID, timeout); - TRACE(Activity, (_T("Wakeup of plugin [%s] process [%u]"), Callsign().c_str(), parentPID)); - result = WakeupProcess(timeout, parentPID, _administrator.Configuration().HibernateLocator().c_str(), _T(""), &_hibernateStorage); -#else - result = Core::ERROR_NONE; + TRACE(Activity, (_T("Wakeup of plugin [%s] process [%u]"), Callsign().c_str(), parentPID)); + result = WakeupProcess(timeout, parentPID, _administrator.Configuration().HibernateLocator().c_str(), _T(""), &_hibernateStorage); #endif - if (result == Core::ERROR_NONE) { - // Updates base class _state only — _current in StateMachine is NOT updated. - // Callers (HibernatedState::Activate and HibernatedState::Deactivate) are - // responsible for calling SetState(_stateActivated) immediately after Wakeup() - // returns to close the window where State() == ACTIVATED but _current still - // points to HibernatedState (which returns nullptr from QueryInterface). - State(ACTIVATED); - SYSLOG(Logging::Startup, (_T("Activated plugin from hibernation [%s]:[%s]"), ClassName().c_str(), Callsign().c_str())); - } - local->Release(); + if (result == Core::ERROR_NONE) { + SYSLOG(Logging::Startup, (_T("Activated plugin from hibernation [%s]:[%s]"), ClassName().c_str(), Callsign().c_str())); } + + local->Release(); } - return (result); + return result; } #ifdef HIBERNATE_SUPPORT_ENABLED From 608f2c7fa23154a600a4717d1ab120ec1a8e155b Mon Sep 17 00:00:00 2001 From: Bram Oosterhuis Date: Wed, 10 Jun 2026 11:38:59 +0200 Subject: [PATCH 10/26] Make StateMachine re-entrancy guard portable and active in all builds The re-entrancy guard used a thread id compared against 0 as a "no thread" sentinel. pthread_t has no portable null value, so this relied on a glibc/musl assumption. It also lived behind ASSERT, leaving release builds with no protection at all, while _transitionLock is recursive and does not block same-thread re-entry on its own. Split the guard into an active flag plus an owner id so the id never needs a sentinel, and turn re-entry detection into real control flow: public triggers now return ERROR_ILLEGAL_STATE on same-thread re-entry in every build. Wrap ownership in a TransitionScope RAII helper so the flag is cleared even if a state method throws. --- Source/Thunder/PluginServer.h | 180 +++++++++++++++++++++++++++++----- 1 file changed, 154 insertions(+), 26 deletions(-) diff --git a/Source/Thunder/PluginServer.h b/Source/Thunder/PluginServer.h index a3c2c9051..35379fd92 100644 --- a/Source/Thunder/PluginServer.h +++ b/Source/Thunder/PluginServer.h @@ -742,7 +742,10 @@ namespace PluginHost { // // Internal triggers (_Activate, _Deactivate) bypass _transitionLock and // are only legal from within an active transition (state class methods). - // ASSERT(_transitionThread == Core::Thread::ThreadId()) guards misuse. + // IsTransitionThread() detects re-entry on the same thread and the public + // triggers reject it with ERROR_ILLEGAL_STATE. This guard is active in all + // builds, not just debug — _transitionLock is recursive and does not block + // same-thread re-entry on its own. // // QueryInterface never holds _transitionLock and is never blocked by // an active transition. It uses _pluginHandling only. @@ -897,6 +900,11 @@ namespace PluginHost { }; private: + // Temporarily yields _transitionLock from within a transition (used by + // Evaluate()). Clears the active flag during the yield so a re-entrant + // trigger on this thread is not mistaken for ongoing ownership, then + // republishes owner + flag on relock. Mirrors the entry/exit ordering + // contract documented on the member fields. class TransitionSuspender { public: TransitionSuspender() = delete; @@ -905,22 +913,57 @@ namespace PluginHost { TransitionSuspender& operator=(const TransitionSuspender&) = delete; TransitionSuspender& operator=(TransitionSuspender&&) = delete; - TransitionSuspender(Core::CriticalSection& lock, std::atomic& transitionThread) + TransitionSuspender(Core::CriticalSection& lock, std::atomic& active, std::atomic& owner) : _lock(lock) - , _transitionThread(transitionThread) + , _active(active) + , _owner(owner) { - _transitionThread = 0; + _active.store(false, std::memory_order_release); _lock.Unlock(); } ~TransitionSuspender() { _lock.Lock(); - _transitionThread = Core::Thread::ThreadId(); + _owner.store(Core::Thread::ThreadId(), std::memory_order_relaxed); + _active.store(true, std::memory_order_release); } private: Core::CriticalSection& _lock; - std::atomic& _transitionThread; + std::atomic& _active; + std::atomic& _owner; + }; + + // RAII ownership of a transition. Publishes owner + active flag on + // construction following the documented ordering, and clears the flag on + // destruction — including when a state method throws, which is why the + // owner reset cannot live inline in the triggers. Constructed only after + // _transitionLock is held. + class TransitionScope { + public: + TransitionScope() = delete; + TransitionScope(const TransitionScope&) = delete; + TransitionScope(TransitionScope&&) = delete; + TransitionScope& operator=(const TransitionScope&) = delete; + TransitionScope& operator=(TransitionScope&&) = delete; + + TransitionScope(std::atomic& active, std::atomic& owner) + : _active(active) + { + // Owner first (relaxed), then flag (release): a concurrent reader + // that sees active==true is guaranteed to see this owner. + owner.store(Core::Thread::ThreadId(), std::memory_order_relaxed); + _active.store(true, std::memory_order_release); + } + + ~TransitionScope() { + // Flag down (release). Owner is left as-is — it is only read while + // active, so it needs no reset. + _active.store(false, std::memory_order_release); + } + + private: + std::atomic& _active; }; public: @@ -936,7 +979,8 @@ namespace PluginHost { : _parent(parent) , _callback(std::move(callback)) , _transitionLock() - , _transitionThread(0) + , _transitionActive(false) + , _transitionOwner() , _current(&_stateDeactivated) { } @@ -954,22 +998,94 @@ namespace PluginHost { _parent.State(newState.Id()); } - inline bool IsTransitionThread() const { - return _transitionThread.load(std::memory_order_acquire) == Core::Thread::ThreadId(); + inline bool IsTransitionThread() const + { + // Read the flag first (acquire). _transitionOwner is only valid while + // active, and the entry ordering guarantees a published owner is visible + // once active reads true. + return _transitionActive.load(std::memory_order_acquire) + && (_transitionOwner.load(std::memory_order_relaxed) == Core::Thread::ThreadId()); } // Public triggers — acquire transition lock before dispatching. // Guarantees exactly one transition executor at a time. // QueryInterface intentionally does NOT take this lock. - Core::hresult Activate(const reason why) { ASSERT(!IsTransitionThread()); Core::SafeSyncType guard(_transitionLock); _transitionThread = Core::Thread::ThreadId(); Core::hresult result = _Activate(why); _transitionThread = 0; return result; } - Core::hresult Deactivate(const reason why) { ASSERT(!IsTransitionThread()); Core::SafeSyncType guard(_transitionLock); _transitionThread = Core::Thread::ThreadId(); Core::hresult result = _Deactivate(why); _transitionThread = 0; return result; } - Core::hresult Hibernate(const uint32_t timeout) { ASSERT(!IsTransitionThread()); Core::SafeSyncType guard(_transitionLock); _transitionThread = Core::Thread::ThreadId(); Core::hresult result = _current.load(std::memory_order_acquire)->Hibernate(*this, timeout); _transitionThread = 0; return result; } - Core::hresult Unavailable(const reason why) { ASSERT(!IsTransitionThread()); Core::SafeSyncType guard(_transitionLock); _transitionThread = Core::Thread::ThreadId(); Core::hresult result = _current.load(std::memory_order_acquire)->Unavailable(*this, why); _transitionThread = 0; return result; } - uint32_t Resume(const reason why) { ASSERT(!IsTransitionThread()); Core::SafeSyncType guard(_transitionLock); _transitionThread = Core::Thread::ThreadId(); uint32_t result = _current.load(std::memory_order_acquire)->Resume(*this, why); _transitionThread = 0; return result; } - uint32_t Suspend(const reason why) { ASSERT(!IsTransitionThread()); Core::SafeSyncType guard(_transitionLock); _transitionThread = Core::Thread::ThreadId(); uint32_t result = _current.load(std::memory_order_acquire)->Suspend(*this, why); _transitionThread = 0; return (result); } - void Reevaluate() { ASSERT(_transitionThread != Core::Thread::ThreadId()); Core::SafeSyncType guard(_transitionLock); _transitionThread = Core::Thread::ThreadId(); _current.load(std::memory_order_acquire)->Reevaluate(*this); _transitionThread = 0; } - void* QueryInterface(const uint32_t id, const bool asIUnknown) { return _current.load(std::memory_order_acquire)->QueryInterface(*this, id, asIUnknown); } - void Tombstone() { Core::SafeSyncType guard(_transitionLock); ASSERT(_current.load(std::memory_order_acquire) == &_stateDeactivated); _parent.Lock(); SetState(_stateDestroyed); _parent.Unlock(); } + Core::hresult Activate(const reason why) + { + if (IsTransitionThread()) { + return Core::ERROR_ILLEGAL_STATE; + } + Core::SafeSyncType guard(_transitionLock); + TransitionScope scope(_transitionActive, _transitionOwner); + return _Activate(why); + } + Core::hresult Deactivate(const reason why) + { + if (IsTransitionThread()) { + return Core::ERROR_ILLEGAL_STATE; + } + Core::SafeSyncType guard(_transitionLock); + TransitionScope scope(_transitionActive, _transitionOwner); + return _Deactivate(why); + } + Core::hresult Hibernate(const uint32_t timeout) + { + if (IsTransitionThread()) { + return Core::ERROR_ILLEGAL_STATE; + } + Core::SafeSyncType guard(_transitionLock); + TransitionScope scope(_transitionActive, _transitionOwner); + return _current.load(std::memory_order_acquire)->Hibernate(*this, timeout); + } + Core::hresult Unavailable(const reason why) + { + if (IsTransitionThread()) { + return Core::ERROR_ILLEGAL_STATE; + } + Core::SafeSyncType guard(_transitionLock); + TransitionScope scope(_transitionActive, _transitionOwner); + return _current.load(std::memory_order_acquire)->Unavailable(*this, why); + } + uint32_t Resume(const reason why) + { + if (IsTransitionThread()) { + return Core::ERROR_ILLEGAL_STATE; + } + Core::SafeSyncType guard(_transitionLock); + TransitionScope scope(_transitionActive, _transitionOwner); + return _current.load(std::memory_order_acquire)->Resume(*this, why); + } + uint32_t Suspend(const reason why) + { + if (IsTransitionThread()) { + return Core::ERROR_ILLEGAL_STATE; + } + Core::SafeSyncType guard(_transitionLock); + TransitionScope scope(_transitionActive, _transitionOwner); + return _current.load(std::memory_order_acquire)->Suspend(*this, why); + } + void Reevaluate() + { + if (IsTransitionThread()) { + return; + } + Core::SafeSyncType guard(_transitionLock); + TransitionScope scope(_transitionActive, _transitionOwner); + _current.load(std::memory_order_acquire)->Reevaluate(*this); + } + + void* QueryInterface(const uint32_t id, const bool asIUnknown) + { + return _current.load(std::memory_order_acquire)->QueryInterface(*this, id, asIUnknown); + } + void Tombstone() + { + Core::SafeSyncType guard(_transitionLock); + ASSERT(_current.load(std::memory_order_acquire) == &_stateDeactivated); + _parent.Lock(); + SetState(_stateDestroyed); + _parent.Unlock(); + } private: // Internal triggers — no lock. Called by state class methods @@ -985,7 +1101,7 @@ namespace PluginHost { // concurrent operations during the yield window void Evaluate() { ASSERT(IsTransitionThread()); - TransitionSuspender suspend(_transitionLock, _transitionThread); + TransitionSuspender suspend(_transitionLock, _transitionActive, _transitionOwner); _parent._administrator.Evaluate(); } @@ -1005,13 +1121,25 @@ namespace PluginHost { Service& _parent; Callback _callback; mutable Core::CriticalSection _transitionLock; - // Debug: tracks which thread owns the current transition. - // ASSERT fires if a public trigger is called re-entrantly - // from within a callback or state method on the same thread. - // Atomic because the ASSERT reads it before _transitionLock - // is acquired — concurrent writes from other threads must be safe. - // Compiled out in release builds. - std::atomic _transitionThread; + // Re-entrancy guard — active in ALL builds, not just debug. _transitionLock + // is a recursive mutex (PTHREAD_MUTEX_RECURSIVE), so it does NOT block a + // re-entrant Lock() from the same thread. A callback or plugin that triggers + // a transition on its own Service would otherwise nest two transitions on one + // thread and corrupt _current. This guard detects that and returns + // ERROR_ILLEGAL_STATE instead. + // + // Two fields because Core::thread_id (pthread_t) has no portable "no thread" + // sentinel — comparing against 0 is a glibc/musl assumption. _transitionOwner + // is only meaningful while _transitionActive is true; its value is ignored + // otherwise, so it never needs a sentinel. + // + // Ordering contract (see triggers): on entry, write _transitionOwner first + // (relaxed) then _transitionActive=true (release); on exit, write + // _transitionActive=false (release) and leave _transitionOwner untouched. + // Readers load _transitionActive (acquire) and only read _transitionOwner + // when it is true. + std::atomic _transitionActive; + std::atomic _transitionOwner; // Atomic pointer — written by SetState() during transitions, // read concurrently by QueryInterface() and trigger methods. // Preserves State pattern dispatch without a mutex on reads. From 4890997f6efb7d00b24b31cfa71a26a4551c7b69 Mon Sep 17 00:00:00 2001 From: Bram Oosterhuis Date: Wed, 10 Jun 2026 11:56:54 +0200 Subject: [PATCH 11/26] Replace dead hibernate state guards with lock-free invariant checks The state checks after HibernateProcess() guarded against a concurrent Wakeup() or Deactivate() changing state mid-hibernation. Under the StateMachine model _transitionLock is held for the whole transition, so no concurrent transition can run and these checks can no longer fire. In Hibernate() the check was a standalone guard, now an ASSERT so the invariant stays verifiable in debug without a dead branch or an empty lock/unlock pair in release. In HibernateChildren() the loop break is kept as a visible abort path but no longer takes Service::Lock() to read State(), consistent with the other lock-free State() reads in this file. --- Source/Thunder/PluginServer.cpp | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/Source/Thunder/PluginServer.cpp b/Source/Thunder/PluginServer.cpp index ae065803a..fb43e2a49 100644 --- a/Source/Thunder/PluginServer.cpp +++ b/Source/Thunder/PluginServer.cpp @@ -608,17 +608,11 @@ namespace PluginHost { TRACE(Activity, (_T("Hibernation of plugin [%s] process [%u]"), sm._parent.Callsign().c_str(), parentPID)); Core::hresult result = HibernateProcess(timeout, parentPID, sm._parent._administrator.Configuration().HibernateLocator().c_str(), _T(""), &sm._parent._hibernateStorage); - sm._parent.Lock(); - // Note: in the original design this checked whether a concurrent Wakeup() or - // Deactivate() had changed state during HibernateProcess(). Under the new design - // _transitionLock is held throughout — no concurrent transition can run — so this - // check can never fire. Retained as a defensive guard only. - if (sm._parent.State() != IShell::HIBERNATED) { - SYSLOG(Logging::Startup, (_T("Hibernation aborted of plugin [%s] process [%u]"), sm._parent.Callsign().c_str(), parentPID)); - sm._parent.Unlock(); - return Core::ERROR_ABORTED; - } - sm._parent.Unlock(); + // _transitionLock is held for the full transition, so no concurrent Wakeup() or + // Deactivate() can change state across the blocking HibernateProcess() call. The + // original runtime check here can never fire under that model — assert the + // invariant instead so a future change to the lock model is caught in debug. + ASSERT(sm._parent.State() == IShell::HIBERNATED); if (result == HIBERNATE_ERROR_NONE) { result = sm._parent.HibernateChildren(parentPID, timeout); @@ -1017,14 +1011,13 @@ namespace PluginHost { for (auto iter = childrenPIDs.begin(); iter != childrenPIDs.end(); ++iter) { TRACE(Activity, (_T("Hibernation of plugin [%s] child process [%u]"), Callsign().c_str(), *iter)); - Lock(); + if (State() != IShell::HIBERNATED) { SYSLOG(Logging::Startup, (_T("Hibernation aborted of plugin [%s] child process [%u]"), Callsign().c_str(), *iter)); result = Core::ERROR_ABORTED; - Unlock(); break; } - Unlock(); + result = HibernateProcess(timeout, *iter, _administrator.Configuration().HibernateLocator().c_str(), _T(""), &_hibernateStorage); if (result == HIBERNATE_ERROR_NONE) { // Hibernate Children of this process From 1128a5b3dc0bc3bda3803f1d79cae91a4914e243 Mon Sep 17 00:00:00 2001 From: Bram Oosterhuis Date: Wed, 10 Jun 2026 12:30:08 +0200 Subject: [PATCH 12/26] Tighten StateMachine visibility MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The state classes and their base were public but are never named outside the StateMachine. Make the whole hierarchy private — it is an implementation detail. SetState() is only called by the state classes, so move it to private as well. Drop Current(), which had no callers; state is already reachable through Service::State(). IsTransitionThread() and the triggers stay public: the work methods in the cpp assert on the former and Service drives the latter. No behaviour change. --- Source/Thunder/PluginServer.h | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/Source/Thunder/PluginServer.h b/Source/Thunder/PluginServer.h index 35379fd92..73cf34bf4 100644 --- a/Source/Thunder/PluginServer.h +++ b/Source/Thunder/PluginServer.h @@ -815,7 +815,7 @@ namespace PluginHost { // only, and only ActivatedState::QueryInterface reaches plugin code. // ----------------------------------------------------------------------- class StateMachine { - public: + private: // Base state — default implementations return ERROR_ILLEGAL_STATE. // Concrete states only override what is legal for them. class StateBase { @@ -986,18 +986,16 @@ namespace PluginHost { } ~StateMachine() = default; - public: - inline IShell::state Current() const { - return _current.load(std::memory_order_acquire)->Id(); - } - - inline void SetState(StateBase& newState) { + private: + inline void SetState(StateBase& newState) + { _current.store(&newState, std::memory_order_release); // PluginHost::Service::State(value) is a plain _state = value assignment // with no lock — calling this while holding _parent.Lock() is safe. _parent.State(newState.Id()); } + public: inline bool IsTransitionThread() const { // Read the flag first (acquire). _transitionOwner is only valid while From 15f848563c1a6fd0c0d10eee7ea484151b0cccc4 Mon Sep 17 00:00:00 2001 From: Bram Oosterhuis Date: Wed, 10 Jun 2026 12:36:31 +0200 Subject: [PATCH 13/26] Refactor formatting in PluginServer to improve code readability --- Source/Thunder/PluginServer.cpp | 3 +- Source/Thunder/PluginServer.h | 83 +++++++++++++++++++-------------- 2 files changed, 48 insertions(+), 38 deletions(-) diff --git a/Source/Thunder/PluginServer.cpp b/Source/Thunder/PluginServer.cpp index fb43e2a49..c5a2fc3ca 100644 --- a/Source/Thunder/PluginServer.cpp +++ b/Source/Thunder/PluginServer.cpp @@ -975,8 +975,7 @@ namespace PluginHost { if (local == nullptr) { result = Core::ERROR_BAD_REQUEST; - } - else { + } else { #ifdef HIBERNATE_SUPPORT_ENABLED pid_t parentPID = local->ParentPID(); diff --git a/Source/Thunder/PluginServer.h b/Source/Thunder/PluginServer.h index 73cf34bf4..57eaaad33 100644 --- a/Source/Thunder/PluginServer.h +++ b/Source/Thunder/PluginServer.h @@ -821,15 +821,15 @@ namespace PluginHost { class StateBase { public: virtual ~StateBase() = default; - virtual IShell::state Id() const = 0; - virtual Core::hresult Activate(StateMachine&, const reason) { return Core::ERROR_ILLEGAL_STATE; } - virtual Core::hresult Deactivate(StateMachine&, const reason) { return Core::ERROR_ILLEGAL_STATE; } - virtual Core::hresult Hibernate(StateMachine&, const uint32_t) { return Core::ERROR_ILLEGAL_STATE; } - virtual Core::hresult Unavailable(StateMachine&, const reason) { return Core::ERROR_ILLEGAL_STATE; } - virtual uint32_t Resume(StateMachine&, const reason) { return Core::ERROR_ILLEGAL_STATE; } - virtual uint32_t Suspend(StateMachine&, const reason) { return Core::ERROR_ILLEGAL_STATE; } - virtual void Reevaluate(StateMachine&) {} - virtual void* QueryInterface(StateMachine&, const uint32_t, const bool) { return nullptr; } + virtual IShell::state Id() const = 0; + virtual Core::hresult Activate(StateMachine&, const reason) { return Core::ERROR_ILLEGAL_STATE; } + virtual Core::hresult Deactivate(StateMachine&, const reason) { return Core::ERROR_ILLEGAL_STATE; } + virtual Core::hresult Hibernate(StateMachine&, const uint32_t) { return Core::ERROR_ILLEGAL_STATE; } + virtual Core::hresult Unavailable(StateMachine&, const reason) { return Core::ERROR_ILLEGAL_STATE; } + virtual uint32_t Resume(StateMachine&, const reason) { return Core::ERROR_ILLEGAL_STATE; } + virtual uint32_t Suspend(StateMachine&, const reason) { return Core::ERROR_ILLEGAL_STATE; } + virtual void Reevaluate(StateMachine&) {} + virtual void* QueryInterface(StateMachine&, const uint32_t, const bool) { return nullptr; } }; class DeactivatedState : public StateBase { @@ -837,26 +837,26 @@ namespace PluginHost { IShell::state Id() const override { return IShell::DEACTIVATED; } Core::hresult Activate(StateMachine&, const reason) override; Core::hresult Unavailable(StateMachine&, const reason) override; - uint32_t Resume(StateMachine&, const reason) override; + uint32_t Resume(StateMachine&, const reason) override; }; class PreconditionState : public StateBase { public: IShell::state Id() const override { return IShell::PRECONDITION; } Core::hresult Deactivate(StateMachine&, const reason) override; - uint32_t Resume(StateMachine&, const reason) override; - uint32_t Suspend(StateMachine&, const reason) override; - void Reevaluate(StateMachine&) override; + uint32_t Resume(StateMachine&, const reason) override; + uint32_t Suspend(StateMachine&, const reason) override; + void Reevaluate(StateMachine&) override; }; class ActivationState : public StateBase { public: IShell::state Id() const override { return IShell::ACTIVATION; } - Core::hresult Activate(StateMachine&, const reason) override { return Core::ERROR_INPROGRESS; } + Core::hresult Activate(StateMachine&, const reason) override { return Core::ERROR_INPROGRESS; } Core::hresult Deactivate(StateMachine&, const reason) override; Core::hresult Hibernate(StateMachine&, const uint32_t) override { return Core::ERROR_INPROGRESS; } Core::hresult Unavailable(StateMachine&, const reason) override { return Core::ERROR_INPROGRESS; } - uint32_t Resume(StateMachine&, const reason) override { return Core::ERROR_INPROGRESS; } + uint32_t Resume(StateMachine&, const reason) override { return Core::ERROR_INPROGRESS; } }; class ActivatedState : public StateBase { @@ -864,20 +864,20 @@ namespace PluginHost { IShell::state Id() const override { return IShell::ACTIVATED; } Core::hresult Deactivate(StateMachine&, const reason) override; Core::hresult Hibernate(StateMachine&, const uint32_t timeout) override; - uint32_t Resume(StateMachine&, const reason) override; - uint32_t Suspend(StateMachine&, const reason) override; - void Reevaluate(StateMachine&) override; - void* QueryInterface(StateMachine&, const uint32_t id, const bool asIUnknown) override; + uint32_t Resume(StateMachine&, const reason) override; + uint32_t Suspend(StateMachine&, const reason) override; + void Reevaluate(StateMachine&) override; + void* QueryInterface(StateMachine&, const uint32_t id, const bool asIUnknown) override; }; class DeactivationState : public StateBase { public: IShell::state Id() const override { return IShell::DEACTIVATION; } - Core::hresult Activate(StateMachine&, const reason) override { return Core::ERROR_INPROGRESS; } - Core::hresult Deactivate(StateMachine&, const reason) override { return Core::ERROR_INPROGRESS; } - Core::hresult Hibernate(StateMachine&, const uint32_t) override { return Core::ERROR_INPROGRESS; } - Core::hresult Unavailable(StateMachine&, const reason) override { return Core::ERROR_INPROGRESS; } - uint32_t Suspend(StateMachine&, const reason) override { return Core::ERROR_INPROGRESS; } + Core::hresult Activate(StateMachine&, const reason) override { return Core::ERROR_INPROGRESS; } + Core::hresult Deactivate(StateMachine&, const reason) override { return Core::ERROR_INPROGRESS; } + Core::hresult Hibernate(StateMachine&, const uint32_t) override { return Core::ERROR_INPROGRESS; } + Core::hresult Unavailable(StateMachine&, const reason) override { return Core::ERROR_INPROGRESS; } + uint32_t Suspend(StateMachine&, const reason) override { return Core::ERROR_INPROGRESS; } }; class HibernatedState : public StateBase { @@ -922,7 +922,8 @@ namespace PluginHost { _lock.Unlock(); } - ~TransitionSuspender() { + ~TransitionSuspender() + { _lock.Lock(); _owner.store(Core::Thread::ThreadId(), std::memory_order_relaxed); _active.store(true, std::memory_order_release); @@ -956,7 +957,8 @@ namespace PluginHost { _active.store(true, std::memory_order_release); } - ~TransitionScope() { + ~TransitionScope() + { // Flag down (release). Owner is left as-is — it is only read while // active, so it needs no reset. _active.store(false, std::memory_order_release); @@ -1088,8 +1090,16 @@ namespace PluginHost { private: // Internal triggers — no lock. Called by state class methods // that are already executing under _transitionLock. - Core::hresult _Activate(const reason why) { ASSERT(IsTransitionThread()); return _current.load(std::memory_order_acquire)->Activate(*this, why); } - Core::hresult _Deactivate(const reason why) { ASSERT(IsTransitionThread()); return _current.load(std::memory_order_acquire)->Deactivate(*this, why); } + Core::hresult _Activate(const reason why) + { + ASSERT(IsTransitionThread()); + return _current.load(std::memory_order_acquire)->Activate(*this, why); + } + Core::hresult _Deactivate(const reason why) + { + ASSERT(IsTransitionThread()); + return _current.load(std::memory_order_acquire)->Deactivate(*this, why); + } // Called from within a state method to trigger cascading re-evaluation // of dependent services. Temporarily yields _transitionLock via @@ -1097,7 +1107,8 @@ namespace PluginHost { // this service without deadlocking. Safe because SetState(DEACTIVATION) // must be called before this — the transient state rejects all // concurrent operations during the yield window - void Evaluate() { + void Evaluate() + { ASSERT(IsTransitionThread()); TransitionSuspender suspend(_transitionLock, _transitionActive, _transitionOwner); _parent._administrator.Evaluate(); @@ -1105,16 +1116,16 @@ namespace PluginHost { // Static state instances — shared across all plugins. // Safe because state objects carry no per-plugin data. - static DeactivatedState _stateDeactivated; + static DeactivatedState _stateDeactivated; static PreconditionState _statePrecondition; - static ActivationState _stateActivation; - static ActivatedState _stateActivated; + static ActivationState _stateActivation; + static ActivatedState _stateActivated; static DeactivationState _stateDeactivation; - static HibernatedState _stateHibernated; - static UnavailableState _stateUnavailable; - static DestroyedState _stateDestroyed; + static HibernatedState _stateHibernated; + static UnavailableState _stateUnavailable; + static DestroyedState _stateDestroyed; - // State classes access Service members via _parent (C++11 nested class access rules) + // State classes access Service members via _parent (C++11 nested class access rules) private: Service& _parent; Callback _callback; From 2913049c5742841a6dd5c10e7c22569c02ed20c2 Mon Sep 17 00:00:00 2001 From: Bram Oosterhuis Date: Wed, 10 Jun 2026 13:35:42 +0200 Subject: [PATCH 14/26] ci: Enable coredump and backtrace collection for smoke test debugging Configure kernel core dumps to /tmp/cores, extract backtraces with gdb, and upload as artifacts. Use always() condition to capture crashes even when test runner tolerates failures with || true. --- .../workflows/Test_Thunder_Test_Support.yml | 43 ++++++++++++++++++- 1 file changed, 41 insertions(+), 2 deletions(-) diff --git a/.github/workflows/Test_Thunder_Test_Support.yml b/.github/workflows/Test_Thunder_Test_Support.yml index b8dccceaa..85eba01d0 100644 --- a/.github/workflows/Test_Thunder_Test_Support.yml +++ b/.github/workflows/Test_Thunder_Test_Support.yml @@ -188,12 +188,39 @@ jobs: -DTEST_PLUGIN_PATH="${PWD}/${{matrix.build_type}}/install/usr/lib/thunder/plugins" cmake --build ${{matrix.build_type}}/build/ThunderNanoServicesTests + - name: Enable core dumps + run: | + ulimit -c unlimited + sudo mkdir -p /tmp/cores + sudo chmod 777 /tmp/cores + echo "kernel.core_pattern=/tmp/cores/core.%e.%p.%t" | sudo tee /proc/sys/kernel/core_pattern + sudo sysctl -p + sudo apt-get install -y gdb + - name: Run smoke test run: | + ulimit -c unlimited LD_LIBRARY_PATH="${{matrix.build_type}}/install/usr/lib:$LD_LIBRARY_PATH" \ ${{matrix.build_type}}/build/Thunder/Tests/test_support/tests/thunder_test_runtime_smoke \ --gtest_output="xml:smoke-test-results.xml" \ - --gtest_color=yes + --gtest_color=yes || true # Allow failure so backtrace step runs + + - name: Dump backtrace on crash + if: ${{ always() }} # Change to always() instead of failure() + run: | + BIN="${{matrix.build_type}}/build/Thunder/Tests/test_support/tests/thunder_test_runtime_smoke" + CORES_FOUND=0 + for core in /tmp/cores/core.*; do + [ -e "$core" ] || continue + CORES_FOUND=1 + echo "===== backtrace for $core =====" + LD_LIBRARY_PATH="${{matrix.build_type}}/install/usr/lib:$LD_LIBRARY_PATH" \ + gdb -batch -ex "thread apply all bt full" -ex "quit" "$BIN" "$core" 2>&1 | tee -a backtrace.txt + done + if [ $CORES_FOUND -eq 0 ]; then + echo "No core files found in /tmp/cores" + ls -la /tmp/cores/ || echo "/tmp/cores does not exist" + fi # - name: Run plugin test (COM-RPC + JSON-RPC + events) # run: | @@ -203,9 +230,21 @@ jobs: # --gtest_color=yes - name: Upload test results + if: ${{ always() }} uses: actions/upload-artifact@v4 with: name: test-results-${{matrix.build_type}} path: | smoke-test-results.xml - # plugin-test-results.xml \ No newline at end of file + # plugin-test-results.xml + if-no-files-found: warn + + - name: Upload crash artifacts + if: ${{ always() }} + uses: actions/upload-artifact@v4 + with: + name: crash-artifacts-${{matrix.build_type}} + path: | + backtrace.txt + /tmp/cores/ + if-no-files-found: ignore From ed8904cf456124b1025068749ef332ba48b255e2 Mon Sep 17 00:00:00 2001 From: Bram Oosterhuis Date: Wed, 10 Jun 2026 13:39:51 +0200 Subject: [PATCH 15/26] fix: Correct core pattern configuration for core dumps in smoke test --- .github/workflows/Test_Thunder_Test_Support.yml | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/.github/workflows/Test_Thunder_Test_Support.yml b/.github/workflows/Test_Thunder_Test_Support.yml index 85eba01d0..61b4fdcd0 100644 --- a/.github/workflows/Test_Thunder_Test_Support.yml +++ b/.github/workflows/Test_Thunder_Test_Support.yml @@ -193,8 +193,7 @@ jobs: ulimit -c unlimited sudo mkdir -p /tmp/cores sudo chmod 777 /tmp/cores - echo "kernel.core_pattern=/tmp/cores/core.%e.%p.%t" | sudo tee /proc/sys/kernel/core_pattern - sudo sysctl -p + echo "kernel.core_pattern=/tmp/cores/core.%e.%p.%t" | sudo tee /proc/sys/kernel/ sudo apt-get install -y gdb - name: Run smoke test From d54e7afc90668ffd567dbe7776ded4798a0766b0 Mon Sep 17 00:00:00 2001 From: Bram Oosterhuis Date: Wed, 10 Jun 2026 13:59:33 +0200 Subject: [PATCH 16/26] fix: Update core pattern configuration for core dumps in smoke test --- .github/workflows/Test_Thunder_Test_Support.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/Test_Thunder_Test_Support.yml b/.github/workflows/Test_Thunder_Test_Support.yml index 61b4fdcd0..e1f3036bb 100644 --- a/.github/workflows/Test_Thunder_Test_Support.yml +++ b/.github/workflows/Test_Thunder_Test_Support.yml @@ -193,7 +193,7 @@ jobs: ulimit -c unlimited sudo mkdir -p /tmp/cores sudo chmod 777 /tmp/cores - echo "kernel.core_pattern=/tmp/cores/core.%e.%p.%t" | sudo tee /proc/sys/kernel/ + echo "/tmp/cores/core.%e.%p.%t" | sudo tee /proc/sys/kernel/core_pattern sudo apt-get install -y gdb - name: Run smoke test From b40e3f7f625d0455d98008b0821e2467ed537f32 Mon Sep 17 00:00:00 2001 From: Bram Oosterhuis Date: Wed, 10 Jun 2026 15:05:21 +0200 Subject: [PATCH 17/26] Fix SIGABRT in Server::Notification during shutdown deactivation When the Controller deactivates during shutdown, the DEACTIVATED callback fires a statechange notification after UnloadPlugin() has already released _handler. Server::Notification then asserted on ClassType() being non-null, which is null at that point, aborting the process. The function body already guards on controller != nullptr and treats it as a no-op, so a null handler here is a legal shutdown state, not an error. Drop that half of the assert and keep only the real invariant (_controller.IsValid()). StateControlStateChange is left unchanged: it dereferences controller unconditionally and its callback is unregistered in Detach() before teardown, so a null there would be a genuine bug. --- Source/Thunder/PluginServer.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/Source/Thunder/PluginServer.cpp b/Source/Thunder/PluginServer.cpp index c5a2fc3ca..dbc470aa4 100644 --- a/Source/Thunder/PluginServer.cpp +++ b/Source/Thunder/PluginServer.cpp @@ -1463,10 +1463,14 @@ namespace PluginHost { void Server::Notification(const string& callsign, const string& event, const string& parameters) { - ASSERT((_controller.IsValid() == true) && (_controller->ClassType() != nullptr)); + ASSERT(_controller.IsValid() == true); Plugin::Controller* controller = _controller->ClassType(); + // controller may be nullptr here: a statechange notification can be delivered + // after the Controller has released its handler during shutdown deactivation. + // The guard below already handles that case as a no-op — it is not an error. + // Break a recursive loop, if it tries to arise ;-) if ( (controller != nullptr) && (callsign != controller->Callsign()) ) { From 3c88af221ebba0507a71c6307374617913e61e7a Mon Sep 17 00:00:00 2001 From: Bram Oosterhuis Date: Wed, 10 Jun 2026 15:10:37 +0200 Subject: [PATCH 18/26] Remove || true workaround from smoke test step The || true was added to let the backtrace step run while the test was crashing during teardown. With the crash fixed and the backtrace step on always(), it is no longer needed and would mask any future abort by keeping the job green. Drop it so the job fails honestly on a crash. --- .github/workflows/Test_Thunder_Test_Support.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/Test_Thunder_Test_Support.yml b/.github/workflows/Test_Thunder_Test_Support.yml index e1f3036bb..4685bc88c 100644 --- a/.github/workflows/Test_Thunder_Test_Support.yml +++ b/.github/workflows/Test_Thunder_Test_Support.yml @@ -202,10 +202,10 @@ jobs: LD_LIBRARY_PATH="${{matrix.build_type}}/install/usr/lib:$LD_LIBRARY_PATH" \ ${{matrix.build_type}}/build/Thunder/Tests/test_support/tests/thunder_test_runtime_smoke \ --gtest_output="xml:smoke-test-results.xml" \ - --gtest_color=yes || true # Allow failure so backtrace step runs + --gtest_color=yes - name: Dump backtrace on crash - if: ${{ always() }} # Change to always() instead of failure() + if: ${{ always() }} run: | BIN="${{matrix.build_type}}/build/Thunder/Tests/test_support/tests/thunder_test_runtime_smoke" CORES_FOUND=0 From 8d17011599fd556393f092a667cdd543a1ac2c45 Mon Sep 17 00:00:00 2001 From: Bram Oosterhuis Date: Mon, 15 Jun 2026 11:18:33 +0200 Subject: [PATCH 19/26] fix: Set state to Deactivated in activation path for PluginServer --- Source/Thunder/PluginServer.cpp | 1 + Source/Thunder/PluginServer.h | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/Source/Thunder/PluginServer.cpp b/Source/Thunder/PluginServer.cpp index dbc470aa4..1494a008a 100644 --- a/Source/Thunder/PluginServer.cpp +++ b/Source/Thunder/PluginServer.cpp @@ -492,6 +492,7 @@ namespace PluginHost { const uint32_t subsystems(sm._parent._administrator.SubSystemInfo().Value()); if ((sm._parent._precondition.Evaluate(subsystems) == true) && (sm._parent._precondition.IsMet() == true)) { + sm.SetState(_stateDeactivated); // re-enter the activation path via DeactivatedState::Activate sm._parent.Unlock(); sm._Activate(sm._parent._reason); } else { diff --git a/Source/Thunder/PluginServer.h b/Source/Thunder/PluginServer.h index 57eaaad33..2127772ce 100644 --- a/Source/Thunder/PluginServer.h +++ b/Source/Thunder/PluginServer.h @@ -1864,7 +1864,7 @@ namespace PluginHost { ASSERT (loadedLib.IsLoaded() == true); Core::System::ModuleBuildRefImpl moduleBuildRef(reinterpret_cast(loadedLib.LoadFunction(_T("ModuleBuildRef")))); if (moduleBuildRef != nullptr) { - _metadata.Hash(moduleBuildRef()); + _metadata.Hash(moduleBuildRef()); } _metadata = service; if (_metadata.IsValid() == true) { From e9e45053c50792c493e203af08b8a059255685f9 Mon Sep 17 00:00:00 2001 From: Bram Oosterhuis Date: Mon, 15 Jun 2026 11:18:49 +0200 Subject: [PATCH 20/26] feat: Add ReentrantControl header for state machine testing --- Tests/statemachine/ReentrantControl.h | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) create mode 100644 Tests/statemachine/ReentrantControl.h diff --git a/Tests/statemachine/ReentrantControl.h b/Tests/statemachine/ReentrantControl.h new file mode 100644 index 000000000..ea5090607 --- /dev/null +++ b/Tests/statemachine/ReentrantControl.h @@ -0,0 +1,19 @@ +#pragma once + +#include + +namespace Thunder { +namespace TestSupport { + + enum class ReentrantTrigger : uint8_t { + Activate, + Deactivate, + Unavailable, + Hibernate + }; + + extern ReentrantTrigger g_reentrantTrigger; // set by the test before activation + extern uint32_t g_reentrantResult; // captured result of the re-entrant call + +} // namespace TestSupport +} // namespace Thunder From 14d5b7a5c7646f1e5cc4604d0d58e1655f2c17c8 Mon Sep 17 00:00:00 2001 From: Bram Oosterhuis Date: Mon, 22 Jun 2026 19:01:46 +0200 Subject: [PATCH 21/26] fix regression: Implement QueryInterface for DeactivationState and refactor ActivatedState --- Source/Thunder/PluginServer.cpp | 49 +++++++-------------------------- Source/Thunder/PluginServer.h | 38 +++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 39 deletions(-) diff --git a/Source/Thunder/PluginServer.cpp b/Source/Thunder/PluginServer.cpp index 1494a008a..d0e5e0676 100644 --- a/Source/Thunder/PluginServer.cpp +++ b/Source/Thunder/PluginServer.cpp @@ -378,6 +378,15 @@ namespace PluginHost { Server::Service::StateMachine::UnavailableState Server::Service::StateMachine::_stateUnavailable; Server::Service::StateMachine::DestroyedState Server::Service::StateMachine::_stateDestroyed; + // ------------------------------------------------------------------------- + // DeactivationState + // ------------------------------------------------------------------------- + void* Server::Service::StateMachine::DeactivationState::QueryInterface(StateMachine& sm, const uint32_t id, const bool asIUnknown) + { + return sm.ForwardToHandler(id, asIUnknown); + } + + // ------------------------------------------------------------------------- // DeactivatedState // ------------------------------------------------------------------------- @@ -682,45 +691,7 @@ namespace PluginHost { void* Server::Service::StateMachine::ActivatedState::QueryInterface(StateMachine& sm, const uint32_t id, const bool asIUnknown) { - // Take a ref-counted local copy of _handler under _pluginHandling. - // This keeps _handler alive across the lock boundary even if - // UnloadPlugin() runs concurrently on another thread. - sm._parent._pluginHandling.Lock(); - - if (id == PluginHost::IDispatcher::ID) { - // Fast path: return the cached _jsonrpc pointer directly rather than - // routing through _handler->QueryInterface(). This is intentional. - // _jsonrpc is set once in AcquireInterfaces() via - // _handler->QueryInterface() and is the same pointer the - // plugin would return. Bypassing the plugin's own QueryInterface avoids - // a potential lock inversion if the plugin's QI acquires internal locks. - // Assumption: plugins do not conditionally expose IDispatcher at runtime. - // If a plugin gates IDispatcher on runtime state, this fast path will - // return the cached pointer regardless — revisit if that pattern emerges. - IDispatcher* jsonrpc = sm._parent._jsonrpc; - if (jsonrpc != nullptr) { - jsonrpc->AddRef(); - } - sm._parent._pluginHandling.Unlock(); - if (jsonrpc != nullptr) { - return asIUnknown == false ? static_cast(jsonrpc) : static_cast(static_cast(jsonrpc)); - } - return nullptr; - } - - IPlugin* handler = sm._parent._handler; - if (handler != nullptr) { - handler->AddRef(); - } - sm._parent._pluginHandling.Unlock(); - - if (handler == nullptr) { - return nullptr; - } - - void* result = handler->QueryInterface(id, asIUnknown); - handler->Release(); - return result; + return sm.ForwardToHandler(id, asIUnknown); } // ------------------------------------------------------------------------- diff --git a/Source/Thunder/PluginServer.h b/Source/Thunder/PluginServer.h index 2127772ce..b6da50f68 100644 --- a/Source/Thunder/PluginServer.h +++ b/Source/Thunder/PluginServer.h @@ -878,6 +878,7 @@ namespace PluginHost { Core::hresult Hibernate(StateMachine&, const uint32_t) override { return Core::ERROR_INPROGRESS; } Core::hresult Unavailable(StateMachine&, const reason) override { return Core::ERROR_INPROGRESS; } uint32_t Suspend(StateMachine&, const reason) override { return Core::ERROR_INPROGRESS; } + void* QueryInterface(StateMachine&, const uint32_t id, const bool asIUnknown) override; }; class HibernatedState : public StateBase { @@ -1101,6 +1102,43 @@ namespace PluginHost { return _current.load(std::memory_order_acquire)->Deactivate(*this, why); } + void* ForwardToHandler(const uint32_t id, const bool asIUnknown) + { + // Take a ref-counted local copy under _pluginHandling. This keeps the handler + // alive across the lock boundary even if UnloadPlugin() runs concurrently. + _parent._pluginHandling.Lock(); + + if (id == PluginHost::IDispatcher::ID) { + // Fast path: return the cached _jsonrpc pointer directly. Intentional — + // _jsonrpc is set once in AcquireInterfaces() via the same QueryInterface + // call the plugin would otherwise receive. Assumption: plugins do not + // conditionally expose IDispatcher at runtime. + IDispatcher* jsonrpc = _parent._jsonrpc; + if (jsonrpc != nullptr) { + jsonrpc->AddRef(); + } + _parent._pluginHandling.Unlock(); + if (jsonrpc != nullptr) { + return asIUnknown == false ? static_cast(jsonrpc) : static_cast(static_cast(jsonrpc)); + } + return nullptr; + } + + IPlugin* handler = _parent._handler; + if (handler != nullptr) { + handler->AddRef(); + } + _parent._pluginHandling.Unlock(); + + if (handler == nullptr) { + return nullptr; + } + + void* result = handler->QueryInterface(id, asIUnknown); + handler->Release(); + return result; + } + // Called from within a state method to trigger cascading re-evaluation // of dependent services. Temporarily yields _transitionLock via // TransitionSuspender so RecursiveNotification can call Reevaluate() on From 89c97a64ac0740ca314fa09d6ae1218a8a269a4c Mon Sep 17 00:00:00 2001 From: Bram Oosterhuis Date: Mon, 22 Jun 2026 19:16:34 +0200 Subject: [PATCH 22/26] fix regression: evaluate both conditions on every Reevaluate, not just the current state's --- Source/Thunder/PluginServer.cpp | 12 ++++-------- Source/Thunder/PluginServer.h | 15 +++++++++++---- 2 files changed, 15 insertions(+), 12 deletions(-) diff --git a/Source/Thunder/PluginServer.cpp b/Source/Thunder/PluginServer.cpp index d0e5e0676..e692c7fc2 100644 --- a/Source/Thunder/PluginServer.cpp +++ b/Source/Thunder/PluginServer.cpp @@ -495,12 +495,10 @@ namespace PluginHost { return sm._parent.RequestSuspend(); } - void Server::Service::StateMachine::PreconditionState::Reevaluate(StateMachine& sm) + void Server::Service::StateMachine::PreconditionState::Reevaluate(StateMachine& sm, const bool preconditionChanged, const bool /* terminationChanged */) { sm._parent.Lock(); - const uint32_t subsystems(sm._parent._administrator.SubSystemInfo().Value()); - - if ((sm._parent._precondition.Evaluate(subsystems) == true) && (sm._parent._precondition.IsMet() == true)) { + if ((preconditionChanged == true) && (sm._parent._precondition.IsMet() == true)) { sm.SetState(_stateDeactivated); // re-enter the activation path via DeactivatedState::Activate sm._parent.Unlock(); sm._Activate(sm._parent._reason); @@ -676,12 +674,10 @@ namespace PluginHost { return sm._parent.RequestSuspend(); } - void Server::Service::StateMachine::ActivatedState::Reevaluate(StateMachine& sm) + void Server::Service::StateMachine::ActivatedState::Reevaluate(StateMachine& sm, const bool /* preconditionChanged */, const bool terminationChanged) { sm._parent.Lock(); - const uint32_t subsystems(sm._parent._administrator.SubSystemInfo().Value()); - - if ((sm._parent._termination.Evaluate(subsystems) == true) && (sm._parent._termination.IsMet() == false)) { + if ((terminationChanged == true) && (sm._parent._termination.IsMet() == false)) { sm._parent.Unlock(); sm._Deactivate(IShell::CONDITIONS); } else { diff --git a/Source/Thunder/PluginServer.h b/Source/Thunder/PluginServer.h index b6da50f68..4ecba36da 100644 --- a/Source/Thunder/PluginServer.h +++ b/Source/Thunder/PluginServer.h @@ -828,7 +828,7 @@ namespace PluginHost { virtual Core::hresult Unavailable(StateMachine&, const reason) { return Core::ERROR_ILLEGAL_STATE; } virtual uint32_t Resume(StateMachine&, const reason) { return Core::ERROR_ILLEGAL_STATE; } virtual uint32_t Suspend(StateMachine&, const reason) { return Core::ERROR_ILLEGAL_STATE; } - virtual void Reevaluate(StateMachine&) {} + virtual void Reevaluate(StateMachine&, const bool /* preconditionChanged */, const bool /* terminationChanged */) {} virtual void* QueryInterface(StateMachine&, const uint32_t, const bool) { return nullptr; } }; @@ -846,7 +846,7 @@ namespace PluginHost { Core::hresult Deactivate(StateMachine&, const reason) override; uint32_t Resume(StateMachine&, const reason) override; uint32_t Suspend(StateMachine&, const reason) override; - void Reevaluate(StateMachine&) override; + void Reevaluate(StateMachine&, const bool preconditionChanged, const bool terminationChanged) override; }; class ActivationState : public StateBase { @@ -866,7 +866,7 @@ namespace PluginHost { Core::hresult Hibernate(StateMachine&, const uint32_t timeout) override; uint32_t Resume(StateMachine&, const reason) override; uint32_t Suspend(StateMachine&, const reason) override; - void Reevaluate(StateMachine&) override; + void Reevaluate(StateMachine&, const bool preconditionChanged, const bool terminationChanged) override; void* QueryInterface(StateMachine&, const uint32_t id, const bool asIUnknown) override; }; @@ -1072,7 +1072,14 @@ namespace PluginHost { } Core::SafeSyncType guard(_transitionLock); TransitionScope scope(_transitionActive, _transitionOwner); - _current.load(std::memory_order_acquire)->Reevaluate(*this); + + _parent.Lock(); + const uint32_t subsystems(_parent._administrator.SubSystemInfo().Value()); + const bool preconditionChanged(_parent._precondition.Evaluate(subsystems)); + const bool terminationChanged(_parent._termination.Evaluate(subsystems)); + _parent.Unlock(); + + _current.load(std::memory_order_acquire)->Reevaluate(*this, preconditionChanged, terminationChanged); } void* QueryInterface(const uint32_t id, const bool asIUnknown) From 3d43fed42ca24b4257fe87d09c5428d951786873 Mon Sep 17 00:00:00 2001 From: Bram Oosterhuis Date: Mon, 22 Jun 2026 19:27:00 +0200 Subject: [PATCH 23/26] Disable all hibernation logic if not enabled --- Source/Thunder/PluginServer.cpp | 22 ++++++++++++---------- Source/Thunder/PluginServer.h | 21 ++++++++++++++++++--- 2 files changed, 30 insertions(+), 13 deletions(-) diff --git a/Source/Thunder/PluginServer.cpp b/Source/Thunder/PluginServer.cpp index e692c7fc2..67f280062 100644 --- a/Source/Thunder/PluginServer.cpp +++ b/Source/Thunder/PluginServer.cpp @@ -362,7 +362,11 @@ namespace PluginHost { Core::hresult Server::Service::Hibernate(const uint32_t timeout VARIABLE_IS_NOT_USED) /* override */ { +#ifdef HIBERNATE_SUPPORT_ENABLED return _stateMachine.Hibernate(timeout); +#else + return Core::ERROR_NOT_SUPPORTED; +#endif } // ------------------------------------------------------------------------- @@ -374,7 +378,9 @@ namespace PluginHost { Server::Service::StateMachine::ActivationState Server::Service::StateMachine::_stateActivation; Server::Service::StateMachine::ActivatedState Server::Service::StateMachine::_stateActivated; Server::Service::StateMachine::DeactivationState Server::Service::StateMachine::_stateDeactivation; +#ifdef HIBERNATE_SUPPORT_ENABLED Server::Service::StateMachine::HibernatedState Server::Service::StateMachine::_stateHibernated; +#endif Server::Service::StateMachine::UnavailableState Server::Service::StateMachine::_stateUnavailable; Server::Service::StateMachine::DestroyedState Server::Service::StateMachine::_stateDestroyed; @@ -582,6 +588,7 @@ namespace PluginHost { return Core::ERROR_NONE; } +#ifdef HIBERNATE_SUPPORT_ENABLED Core::hresult Server::Service::StateMachine::ActivatedState::Hibernate(StateMachine& sm, const uint32_t timeout VARIABLE_IS_NOT_USED) { if (sm._parent.AllowedHibernate() == false) { @@ -608,7 +615,6 @@ namespace PluginHost { // more accurate but requires an IShell::state enum change (tracked as debt). sm.SetState(_stateHibernated); -#ifdef HIBERNATE_SUPPORT_ENABLED pid_t parentPID = local->ParentPID(); local->Release(); sm._parent.Unlock(); @@ -633,10 +639,6 @@ namespace PluginHost { } sm._parent.Lock(); -#else - local->Release(); - Core::hresult result = Core::ERROR_NONE; -#endif // coverity[DEADCODE] — on the non-HIBERNATE_ENABLED path result is always // ERROR_NONE here, making the else-if appear unreachable to Coverity. // Both branches are reachable when HIBERNATE_SUPPORT_ENABLED is defined. @@ -663,6 +665,7 @@ namespace PluginHost { return result; } +#endif uint32_t Server::Service::StateMachine::ActivatedState::Resume(StateMachine & sm, const reason why VARIABLE_IS_NOT_USED) { @@ -693,7 +696,7 @@ namespace PluginHost { // ------------------------------------------------------------------------- // HibernatedState // ------------------------------------------------------------------------- - +#ifdef HIBERNATE_SUPPORT_ENABLED Core::hresult Server::Service::StateMachine::HibernatedState::Activate(StateMachine& sm, const reason why VARIABLE_IS_NOT_USED) { const Core::hresult result = sm._parent.Wakeup(3000); @@ -713,7 +716,7 @@ namespace PluginHost { sm.SetState(_stateActivated); // _current must be updated before _Deactivate return sm._Deactivate(why); } - +#endif // ------------------------------------------------------------------------- // UnavailableState // ------------------------------------------------------------------------- @@ -932,6 +935,7 @@ namespace PluginHost { return result; } +#ifdef HIBERNATE_SUPPORT_ENABLED uint32_t Server::Service::Wakeup(const uint32_t timeout VARIABLE_IS_NOT_USED) { ASSERT(_stateMachine.IsTransitionThread()); @@ -944,7 +948,6 @@ namespace PluginHost { if (local == nullptr) { result = Core::ERROR_BAD_REQUEST; } else { -#ifdef HIBERNATE_SUPPORT_ENABLED pid_t parentPID = local->ParentPID(); // There is no recovery path while doing Wakeup, don't care about errors @@ -952,7 +955,7 @@ namespace PluginHost { TRACE(Activity, (_T("Wakeup of plugin [%s] process [%u]"), Callsign().c_str(), parentPID)); result = WakeupProcess(timeout, parentPID, _administrator.Configuration().HibernateLocator().c_str(), _T(""), &_hibernateStorage); -#endif + if (result == Core::ERROR_NONE) { SYSLOG(Logging::Startup, (_T("Activated plugin from hibernation [%s]:[%s]"), ClassName().c_str(), Callsign().c_str())); } @@ -963,7 +966,6 @@ namespace PluginHost { return result; } -#ifdef HIBERNATE_SUPPORT_ENABLED uint32_t Server::Service::HibernateChildren(const pid_t parentPID, const uint32_t timeout) { Core::hresult result = Core::ERROR_NONE; diff --git a/Source/Thunder/PluginServer.h b/Source/Thunder/PluginServer.h index 4ecba36da..a3b712c13 100644 --- a/Source/Thunder/PluginServer.h +++ b/Source/Thunder/PluginServer.h @@ -824,7 +824,11 @@ namespace PluginHost { virtual IShell::state Id() const = 0; virtual Core::hresult Activate(StateMachine&, const reason) { return Core::ERROR_ILLEGAL_STATE; } virtual Core::hresult Deactivate(StateMachine&, const reason) { return Core::ERROR_ILLEGAL_STATE; } +#ifdef HIBERNATE_SUPPORT_ENABLED virtual Core::hresult Hibernate(StateMachine&, const uint32_t) { return Core::ERROR_ILLEGAL_STATE; } +#else + virtual Core::hresult Hibernate(StateMachine&, const uint32_t) { return Core::ERROR_NOT_SUPPORTED; } +#endif virtual Core::hresult Unavailable(StateMachine&, const reason) { return Core::ERROR_ILLEGAL_STATE; } virtual uint32_t Resume(StateMachine&, const reason) { return Core::ERROR_ILLEGAL_STATE; } virtual uint32_t Suspend(StateMachine&, const reason) { return Core::ERROR_ILLEGAL_STATE; } @@ -854,7 +858,9 @@ namespace PluginHost { IShell::state Id() const override { return IShell::ACTIVATION; } Core::hresult Activate(StateMachine&, const reason) override { return Core::ERROR_INPROGRESS; } Core::hresult Deactivate(StateMachine&, const reason) override; +#ifdef HIBERNATE_SUPPORT_ENABLED Core::hresult Hibernate(StateMachine&, const uint32_t) override { return Core::ERROR_INPROGRESS; } +#endif Core::hresult Unavailable(StateMachine&, const reason) override { return Core::ERROR_INPROGRESS; } uint32_t Resume(StateMachine&, const reason) override { return Core::ERROR_INPROGRESS; } }; @@ -863,7 +869,9 @@ namespace PluginHost { public: IShell::state Id() const override { return IShell::ACTIVATED; } Core::hresult Deactivate(StateMachine&, const reason) override; +#ifdef HIBERNATE_SUPPORT_ENABLED Core::hresult Hibernate(StateMachine&, const uint32_t timeout) override; +#endif uint32_t Resume(StateMachine&, const reason) override; uint32_t Suspend(StateMachine&, const reason) override; void Reevaluate(StateMachine&, const bool preconditionChanged, const bool terminationChanged) override; @@ -875,18 +883,22 @@ namespace PluginHost { IShell::state Id() const override { return IShell::DEACTIVATION; } Core::hresult Activate(StateMachine&, const reason) override { return Core::ERROR_INPROGRESS; } Core::hresult Deactivate(StateMachine&, const reason) override { return Core::ERROR_INPROGRESS; } +#ifdef HIBERNATE_SUPPORT_ENABLED Core::hresult Hibernate(StateMachine&, const uint32_t) override { return Core::ERROR_INPROGRESS; } +#endif Core::hresult Unavailable(StateMachine&, const reason) override { return Core::ERROR_INPROGRESS; } uint32_t Suspend(StateMachine&, const reason) override { return Core::ERROR_INPROGRESS; } void* QueryInterface(StateMachine&, const uint32_t id, const bool asIUnknown) override; }; +#ifdef HIBERNATE_SUPPORT_ENABLED class HibernatedState : public StateBase { public: IShell::state Id() const override { return IShell::HIBERNATED; } Core::hresult Activate(StateMachine&, const reason) override; Core::hresult Deactivate(StateMachine&, const reason) override; }; +#endif class UnavailableState : public StateBase { public: @@ -1029,6 +1041,7 @@ namespace PluginHost { TransitionScope scope(_transitionActive, _transitionOwner); return _Deactivate(why); } +#ifdef HIBERNATE_SUPPORT_ENABLED Core::hresult Hibernate(const uint32_t timeout) { if (IsTransitionThread()) { @@ -1038,6 +1051,7 @@ namespace PluginHost { TransitionScope scope(_transitionActive, _transitionOwner); return _current.load(std::memory_order_acquire)->Hibernate(*this, timeout); } +#endif Core::hresult Unavailable(const reason why) { if (IsTransitionThread()) { @@ -1166,7 +1180,9 @@ namespace PluginHost { static ActivationState _stateActivation; static ActivatedState _stateActivated; static DeactivationState _stateDeactivation; +#ifdef HIBERNATE_SUPPORT_ENABLED static HibernatedState _stateHibernated; +#endif static UnavailableState _stateUnavailable; static DestroyedState _stateDestroyed; @@ -1966,12 +1982,11 @@ namespace PluginHost { uint32_t RequestSuspend(); // _stateControl->Request(SUSPEND) if RESUMED uint32_t RequestResume(); // _stateControl->Request(RESUME) if SUSPENDED +#ifdef HIBERNATE_SUPPORT_ENABLED uint32_t Wakeup(const uint32_t timeout); - - #ifdef HIBERNATE_SUPPORT_ENABLED uint32_t HibernateChildren(const pid_t parentPID, const uint32_t timeout); uint32_t WakeupChildren(const pid_t parentPID, const uint32_t timeout); - #endif +#endif public: // do not restrict visibility for a virtual in a derived class From d4251169f11f1189cdbd2576368294d70a58e51c Mon Sep 17 00:00:00 2001 From: Bram Oosterhuis Date: Mon, 22 Jun 2026 19:44:34 +0200 Subject: [PATCH 24/26] Disable all suspend and resume functionality in state machine. --- Source/Thunder/PluginServer.cpp | 19 +++++++++++++++---- Source/Thunder/PluginServer.h | 18 ++++++++++++++++++ 2 files changed, 33 insertions(+), 4 deletions(-) diff --git a/Source/Thunder/PluginServer.cpp b/Source/Thunder/PluginServer.cpp index 67f280062..47827bd5e 100644 --- a/Source/Thunder/PluginServer.cpp +++ b/Source/Thunder/PluginServer.cpp @@ -332,16 +332,18 @@ namespace PluginHost { return _stateMachine.Activate(why); } - uint32_t Server::Service::Resume(const reason why) /* override */ - { - return _stateMachine.Resume(why); - } Core::hresult Server::Service::Deactivate(const reason why) /* override */ { return _stateMachine.Deactivate(why); } +#ifdef STATEMACHINE_SUSPEND_RESUME_ENABLED + uint32_t Server::Service::Resume(const reason why) /* override */ + { + return _stateMachine.Resume(why); + } + uint32_t Server::Service::Suspend(const reason why) /* override */ { uint32_t result = Core::ERROR_NONE; @@ -354,6 +356,7 @@ namespace PluginHost { return (result); } +#endif Core::hresult Server::Service::Unavailable(const reason why) /* override */ { @@ -463,6 +466,7 @@ namespace PluginHost { return Core::ERROR_NONE; } +#ifdef STATEMACHINE_SUSPEND_RESUME_ENABLED uint32_t Server::Service::StateMachine::DeactivatedState::Resume(StateMachine & sm, const reason why) { Core::hresult result = sm._Activate(why); @@ -472,6 +476,7 @@ namespace PluginHost { return sm._current.load(std::memory_order_acquire)->Resume(sm, why); } +#endif // ------------------------------------------------------------------------- // PreconditionState @@ -491,6 +496,7 @@ namespace PluginHost { return Core::ERROR_NONE; } +#ifdef STATEMACHINE_SUSPEND_RESUME_ENABLED uint32_t Server::Service::StateMachine::PreconditionState::Resume(StateMachine & sm, const reason /* why */) { return sm._parent.RequestResume(); @@ -500,6 +506,7 @@ namespace PluginHost { { return sm._parent.RequestSuspend(); } +#endif void Server::Service::StateMachine::PreconditionState::Reevaluate(StateMachine& sm, const bool preconditionChanged, const bool /* terminationChanged */) { @@ -667,6 +674,7 @@ namespace PluginHost { } #endif +#ifdef STATEMACHINE_SUSPEND_RESUME_ENABLED uint32_t Server::Service::StateMachine::ActivatedState::Resume(StateMachine & sm, const reason why VARIABLE_IS_NOT_USED) { return sm._parent.RequestResume(); @@ -676,6 +684,7 @@ namespace PluginHost { { return sm._parent.RequestSuspend(); } +#endif void Server::Service::StateMachine::ActivatedState::Reevaluate(StateMachine& sm, const bool /* preconditionChanged */, const bool terminationChanged) { @@ -887,6 +896,7 @@ namespace PluginHost { } } +#ifdef STATEMACHINE_SUSPEND_RESUME_ENABLED uint32_t Server::Service::RequestResume() { ASSERT(_stateMachine.IsTransitionThread()); @@ -934,6 +944,7 @@ namespace PluginHost { return result; } +#endif #ifdef HIBERNATE_SUPPORT_ENABLED uint32_t Server::Service::Wakeup(const uint32_t timeout VARIABLE_IS_NOT_USED) diff --git a/Source/Thunder/PluginServer.h b/Source/Thunder/PluginServer.h index a3b712c13..225d43ce4 100644 --- a/Source/Thunder/PluginServer.h +++ b/Source/Thunder/PluginServer.h @@ -830,8 +830,10 @@ namespace PluginHost { virtual Core::hresult Hibernate(StateMachine&, const uint32_t) { return Core::ERROR_NOT_SUPPORTED; } #endif virtual Core::hresult Unavailable(StateMachine&, const reason) { return Core::ERROR_ILLEGAL_STATE; } +#ifdef STATEMACHINE_SUSPEND_RESUME_ENABLED virtual uint32_t Resume(StateMachine&, const reason) { return Core::ERROR_ILLEGAL_STATE; } virtual uint32_t Suspend(StateMachine&, const reason) { return Core::ERROR_ILLEGAL_STATE; } +#endif virtual void Reevaluate(StateMachine&, const bool /* preconditionChanged */, const bool /* terminationChanged */) {} virtual void* QueryInterface(StateMachine&, const uint32_t, const bool) { return nullptr; } }; @@ -841,15 +843,19 @@ namespace PluginHost { IShell::state Id() const override { return IShell::DEACTIVATED; } Core::hresult Activate(StateMachine&, const reason) override; Core::hresult Unavailable(StateMachine&, const reason) override; +#ifdef STATEMACHINE_SUSPEND_RESUME_ENABLED uint32_t Resume(StateMachine&, const reason) override; +#endif }; class PreconditionState : public StateBase { public: IShell::state Id() const override { return IShell::PRECONDITION; } Core::hresult Deactivate(StateMachine&, const reason) override; +#ifdef STATEMACHINE_SUSPEND_RESUME_ENABLED uint32_t Resume(StateMachine&, const reason) override; uint32_t Suspend(StateMachine&, const reason) override; +#endif void Reevaluate(StateMachine&, const bool preconditionChanged, const bool terminationChanged) override; }; @@ -862,7 +868,9 @@ namespace PluginHost { Core::hresult Hibernate(StateMachine&, const uint32_t) override { return Core::ERROR_INPROGRESS; } #endif Core::hresult Unavailable(StateMachine&, const reason) override { return Core::ERROR_INPROGRESS; } +#ifdef STATEMACHINE_SUSPEND_RESUME_ENABLED uint32_t Resume(StateMachine&, const reason) override { return Core::ERROR_INPROGRESS; } +#endif }; class ActivatedState : public StateBase { @@ -872,8 +880,10 @@ namespace PluginHost { #ifdef HIBERNATE_SUPPORT_ENABLED Core::hresult Hibernate(StateMachine&, const uint32_t timeout) override; #endif +#ifdef STATEMACHINE_SUSPEND_RESUME_ENABLED uint32_t Resume(StateMachine&, const reason) override; uint32_t Suspend(StateMachine&, const reason) override; +#endif void Reevaluate(StateMachine&, const bool preconditionChanged, const bool terminationChanged) override; void* QueryInterface(StateMachine&, const uint32_t id, const bool asIUnknown) override; }; @@ -887,7 +897,9 @@ namespace PluginHost { Core::hresult Hibernate(StateMachine&, const uint32_t) override { return Core::ERROR_INPROGRESS; } #endif Core::hresult Unavailable(StateMachine&, const reason) override { return Core::ERROR_INPROGRESS; } +#ifdef STATEMACHINE_SUSPEND_RESUME_ENABLED uint32_t Suspend(StateMachine&, const reason) override { return Core::ERROR_INPROGRESS; } +#endif void* QueryInterface(StateMachine&, const uint32_t id, const bool asIUnknown) override; }; @@ -1061,6 +1073,7 @@ namespace PluginHost { TransitionScope scope(_transitionActive, _transitionOwner); return _current.load(std::memory_order_acquire)->Unavailable(*this, why); } +#ifdef STATEMACHINE_SUSPEND_RESUME_ENABLED uint32_t Resume(const reason why) { if (IsTransitionThread()) { @@ -1079,6 +1092,7 @@ namespace PluginHost { TransitionScope scope(_transitionActive, _transitionOwner); return _current.load(std::memory_order_acquire)->Suspend(*this, why); } +#endif void Reevaluate() { if (IsTransitionThread()) { @@ -1908,8 +1922,10 @@ namespace PluginHost { Core::hresult Hibernate(const uint32_t timeout = 10000 /*ms*/) override; +#ifdef STATEMACHINE_SUSPEND_RESUME_ENABLED uint32_t Suspend(const reason why); uint32_t Resume(const reason why); +#endif reason Reason() const override { @@ -1979,8 +1995,10 @@ namespace PluginHost { void UnloadPlugin(); // ReleaseInterfaces void Attach(); // WebServer + JSONRPC + external connector + StateControl void Detach(); // reverse of Attach +#ifdef STATEMACHINE_SUSPEND_RESUME_ENABLED uint32_t RequestSuspend(); // _stateControl->Request(SUSPEND) if RESUMED uint32_t RequestResume(); // _stateControl->Request(RESUME) if SUSPENDED +#endif #ifdef HIBERNATE_SUPPORT_ENABLED uint32_t Wakeup(const uint32_t timeout); From f568c6d5c4c3929728bcc0e7a4941337229e7455 Mon Sep 17 00:00:00 2001 From: Bram Oosterhuis Date: Mon, 22 Jun 2026 19:48:33 +0200 Subject: [PATCH 25/26] Tests: add StateMachine lifecycle characterization suite --- Tests/CMakeLists.txt | 5 + Tests/statemachine/BravePlugin.cpp | 37 + Tests/statemachine/CMakeLists.txt | 59 ++ Tests/statemachine/FailInitPlugin.cpp | 36 + Tests/statemachine/Module.cpp | 3 + Tests/statemachine/Module.h | 10 + Tests/statemachine/ObserverControl.h | 11 + Tests/statemachine/ObserverPlugin.cpp | 64 ++ Tests/statemachine/ReentrantPlugin.cpp | 56 ++ Tests/statemachine/StateControlControl.h | 21 + Tests/statemachine/StateControlPlugin.cpp | 104 +++ Tests/statemachine/StateMachineTest.cpp | 759 ++++++++++++++++++ Tests/statemachine/provider/CMakeLists.txt | 31 + Tests/statemachine/provider/Module.cpp | 3 + Tests/statemachine/provider/Module.h | 10 + .../provider/StreamingProvider.cpp | 50 ++ .../statecontrol_oop/CMakeLists.txt | 32 + .../statemachine/statecontrol_oop/Module.cpp | 3 + Tests/statemachine/statecontrol_oop/Module.h | 10 + .../StateControlImplementation.cpp | 68 ++ .../StateControlOOPPlugin.cpp | 67 ++ .../statecontrol_oop/StateControlTests.cpp | 22 + 22 files changed, 1461 insertions(+) create mode 100644 Tests/statemachine/BravePlugin.cpp create mode 100644 Tests/statemachine/CMakeLists.txt create mode 100644 Tests/statemachine/FailInitPlugin.cpp create mode 100644 Tests/statemachine/Module.cpp create mode 100644 Tests/statemachine/Module.h create mode 100644 Tests/statemachine/ObserverControl.h create mode 100644 Tests/statemachine/ObserverPlugin.cpp create mode 100644 Tests/statemachine/ReentrantPlugin.cpp create mode 100644 Tests/statemachine/StateControlControl.h create mode 100644 Tests/statemachine/StateControlPlugin.cpp create mode 100644 Tests/statemachine/StateMachineTest.cpp create mode 100644 Tests/statemachine/provider/CMakeLists.txt create mode 100644 Tests/statemachine/provider/Module.cpp create mode 100644 Tests/statemachine/provider/Module.h create mode 100644 Tests/statemachine/provider/StreamingProvider.cpp create mode 100644 Tests/statemachine/statecontrol_oop/CMakeLists.txt create mode 100644 Tests/statemachine/statecontrol_oop/Module.cpp create mode 100644 Tests/statemachine/statecontrol_oop/Module.h create mode 100644 Tests/statemachine/statecontrol_oop/StateControlImplementation.cpp create mode 100644 Tests/statemachine/statecontrol_oop/StateControlOOPPlugin.cpp create mode 100644 Tests/statemachine/statecontrol_oop/StateControlTests.cpp diff --git a/Tests/CMakeLists.txt b/Tests/CMakeLists.txt index 4bd06e1d2..e9f624322 100644 --- a/Tests/CMakeLists.txt +++ b/Tests/CMakeLists.txt @@ -7,6 +7,7 @@ option(MESSAGEBUFFER_TEST "Test message buffer" OFF) option(UNRAVELLER "reveal thread details" OFF) option(STREAMJSON_GARBAGE_TEST "Reproducer for issue #1963: infinite loop on garbage data in StreamJSONType::ReceiveData()" OFF) option(ENABLE_TEST_RUNTIME "Build Thunder test support library for plugin integration tests" OFF) +option(STATEMACHINE_TEST "Test for the state machine plugin" OFF) if(BUILD_TESTS) add_subdirectory(unit) @@ -44,6 +45,10 @@ if(UNRAVELLER) add_subdirectory(unraveller) endif() +if(STATEMACHINE_TEST) + add_subdirectory(statemachine) +endif() + if(STREAMJSON_GARBAGE_TEST) add_subdirectory(streamjson-garbage) endif() diff --git a/Tests/statemachine/BravePlugin.cpp b/Tests/statemachine/BravePlugin.cpp new file mode 100644 index 000000000..6265cc235 --- /dev/null +++ b/Tests/statemachine/BravePlugin.cpp @@ -0,0 +1,37 @@ +#include "Module.h" + +namespace Thunder { +namespace Plugin { + + // Minimal in-process plugin: loads, initializes and tears down cleanly. + // Empty Locator in the config routes instantiation through the in-process + // service chain, matched on the bare class name "BravePlugin". + class BravePlugin : public PluginHost::IPlugin { + public: + BravePlugin() = default; + ~BravePlugin() override = default; + + BravePlugin(const BravePlugin&) = delete; + BravePlugin& operator=(const BravePlugin&) = delete; + + BEGIN_INTERFACE_MAP(BravePlugin) + INTERFACE_ENTRY(PluginHost::IPlugin) + END_INTERFACE_MAP + + const string Initialize(PluginHost::IShell* /* shell */) override + { + return string(); // empty == success + } + void Deinitialize(PluginHost::IShell* /* shell */) override + { + } + string Information() const override + { + return string(); + } + }; + + SERVICE_REGISTRATION(BravePlugin, 1, 0) + +} // namespace Plugin +} // namespace Thunder diff --git a/Tests/statemachine/CMakeLists.txt b/Tests/statemachine/CMakeLists.txt new file mode 100644 index 000000000..d2375e6de --- /dev/null +++ b/Tests/statemachine/CMakeLists.txt @@ -0,0 +1,59 @@ +set(TARGET thunder_test_statemachine) + +find_package(GTest REQUIRED) + +add_executable(${TARGET} + StateMachineTest.cpp + BravePlugin.cpp + FailInitPlugin.cpp + ReentrantPlugin.cpp + ObserverPlugin.cpp + StateControlPlugin.cpp + Module.cpp +) + +target_compile_definitions(${TARGET} + PRIVATE + MODULE_NAME=StateMachineTest + HOSTING_COMPROCESS=ThunderPlugin + THREADPOOL_COUNT=4 +) + +if(BUILD_REFERENCE) + target_compile_definitions(${TARGET} PRIVATE BUILD_REFERENCE=${BUILD_REFERENCE}) +endif() + +set(PLUGIN_DIR "${CMAKE_CURRENT_BINARY_DIR}/data/plugins" CACHE PATH "Directory where the test plugins are located") + + + +target_link_libraries(${TARGET} + PRIVATE + CompileSettings::CompileSettings + CompileSettingsDebug::CompileSettingsDebug + ${NAMESPACE}Plugins::${NAMESPACE}Plugins + thunder_test_support + GTest::gtest + GTest::gtest_main +) + +target_compile_definitions(${TARGET} PRIVATE + TEST_PLUGIN_DIR="${PLUGIN_DIR}" +) + +add_custom_command(TARGET ${TARGET} PRE_BUILD + COMMAND ${CMAKE_COMMAND} -E make_directory "${PLUGIN_DIR}" +) + +add_subdirectory(provider) +target_compile_definitions(${TARGET} PRIVATE + TEST_PLUGIN_LOCATOR="$" +) +add_dependencies(${TARGET} ThunderTestStreamingProvider) + +add_subdirectory(statecontrol_oop) +target_compile_definitions(${TARGET} PRIVATE + STATECTRL_OOP_LOCATOR="$") +add_dependencies(${TARGET} ThunderTestStateControlOOP) + +add_test(NAME ${TARGET} COMMAND ${TARGET}) diff --git a/Tests/statemachine/FailInitPlugin.cpp b/Tests/statemachine/FailInitPlugin.cpp new file mode 100644 index 000000000..17dc252f9 --- /dev/null +++ b/Tests/statemachine/FailInitPlugin.cpp @@ -0,0 +1,36 @@ +#include "Module.h" + +namespace Thunder { +namespace Plugin { + + // In-process plugin whose Initialize() reports failure, to drive the + // INITIALIZATION_FAILED branch (ends back in DEACTIVATED). + class FailInitPlugin : public PluginHost::IPlugin { + public: + FailInitPlugin() = default; + ~FailInitPlugin() override = default; + + FailInitPlugin(const FailInitPlugin&) = delete; + FailInitPlugin& operator=(const FailInitPlugin&) = delete; + + BEGIN_INTERFACE_MAP(FailInitPlugin) + INTERFACE_ENTRY(PluginHost::IPlugin) + END_INTERFACE_MAP + + const string Initialize(PluginHost::IShell* /* shell */) override + { + return _T("init refused"); // non-empty == failure + } + void Deinitialize(PluginHost::IShell* /* shell */) override + { + } + string Information() const override + { + return string(); + } + }; + + SERVICE_REGISTRATION(FailInitPlugin, 1, 0) + +} // namespace Plugin +} // namespace Thunder diff --git a/Tests/statemachine/Module.cpp b/Tests/statemachine/Module.cpp new file mode 100644 index 000000000..714f45038 --- /dev/null +++ b/Tests/statemachine/Module.cpp @@ -0,0 +1,3 @@ +#include "Module.h" + +MODULE_NAME_DECLARATION(BUILD_REFERENCE) \ No newline at end of file diff --git a/Tests/statemachine/Module.h b/Tests/statemachine/Module.h new file mode 100644 index 000000000..493ba0c38 --- /dev/null +++ b/Tests/statemachine/Module.h @@ -0,0 +1,10 @@ +#pragma once + +#ifndef MODULE_NAME +#define MODULE_NAME StateMachineTest +#endif + +#include + +#undef EXTERNAL +#define EXTERNAL \ No newline at end of file diff --git a/Tests/statemachine/ObserverControl.h b/Tests/statemachine/ObserverControl.h new file mode 100644 index 000000000..1c33c3cb8 --- /dev/null +++ b/Tests/statemachine/ObserverControl.h @@ -0,0 +1,11 @@ +#pragma once +#include + +namespace Thunder { +namespace TestSupport { + extern std::string g_observedCallsign; // set by the test before deactivating + extern bool g_deactivatedFired; + extern bool g_shellQiWorked; // QI during the Deactivated notification + extern bool g_handlerQiResolved; // QI during it (forwards to live handler -> non-null) +} +} diff --git a/Tests/statemachine/ObserverPlugin.cpp b/Tests/statemachine/ObserverPlugin.cpp new file mode 100644 index 000000000..eedb8df8e --- /dev/null +++ b/Tests/statemachine/ObserverPlugin.cpp @@ -0,0 +1,64 @@ +#include "Module.h" +#include "ObserverControl.h" + +namespace Thunder { + +namespace TestSupport { + std::string g_observedCallsign; + bool g_deactivatedFired = false; + bool g_shellQiWorked = false; + bool g_handlerQiResolved = false; +} + +namespace Plugin { + + // Observes plugin lifecycle notifications. On the observed plugin's + // Deactivated() it probes QueryInterface to pin the notification-ordering + // contract: fired in DEACTIVATION state, with _handler still alive, so both + // IShell and the handler interface resolve. + class ObserverPlugin : public PluginHost::IPlugin, public PluginHost::IPlugin::INotification { + public: + ObserverPlugin() = default; + ~ObserverPlugin() override = default; + ObserverPlugin(const ObserverPlugin&) = delete; + ObserverPlugin& operator=(const ObserverPlugin&) = delete; + + BEGIN_INTERFACE_MAP(ObserverPlugin) + INTERFACE_ENTRY(PluginHost::IPlugin) + INTERFACE_ENTRY(PluginHost::IPlugin::INotification) + END_INTERFACE_MAP + + const string Initialize(PluginHost::IShell* shell) override + { + shell->Register(static_cast(this)); + return string(); + } + void Deinitialize(PluginHost::IShell* shell) override + { + shell->Unregister(static_cast(this)); + } + string Information() const override { return string(); } + + void Activated(const string&, PluginHost::IShell*) override {} + void Unavailable(const string&, PluginHost::IShell*) override {} + void Deactivated(const string& callsign, PluginHost::IShell* plugin) override + { + if (callsign != TestSupport::g_observedCallsign) { + return; + } + TestSupport::g_deactivatedFired = true; + + PluginHost::IShell* asShell = plugin->QueryInterface(); + TestSupport::g_shellQiWorked = (asShell != nullptr); + if (asShell != nullptr) { asShell->Release(); } + + PluginHost::IPlugin* asPlugin = plugin->QueryInterface(); + TestSupport::g_handlerQiResolved = (asPlugin != nullptr); + if (asPlugin != nullptr) { asPlugin->Release(); } + } + }; + + SERVICE_REGISTRATION(ObserverPlugin, 1, 0) + +} // namespace Plugin +} // namespace Thunder diff --git a/Tests/statemachine/ReentrantPlugin.cpp b/Tests/statemachine/ReentrantPlugin.cpp new file mode 100644 index 000000000..41fa02628 --- /dev/null +++ b/Tests/statemachine/ReentrantPlugin.cpp @@ -0,0 +1,56 @@ +#include "Module.h" +#include "ReentrantControl.h" + +namespace Thunder { + +namespace TestSupport { + ReentrantTrigger g_reentrantTrigger = ReentrantTrigger::Deactivate; + uint32_t g_reentrantResult = Core::ERROR_NONE; +} + +namespace Plugin { + + // Calls a lifecycle trigger on its OWN shell from within Initialize, i.e. on + // the transition thread. The re-entrancy guard must reject every such call + // with ERROR_ILLEGAL_STATE without deadlocking or corrupting state. + class ReentrantPlugin : public PluginHost::IPlugin { + public: + ReentrantPlugin() = default; + ~ReentrantPlugin() override = default; + ReentrantPlugin(const ReentrantPlugin&) = delete; + ReentrantPlugin& operator=(const ReentrantPlugin&) = delete; + + BEGIN_INTERFACE_MAP(ReentrantPlugin) + INTERFACE_ENTRY(PluginHost::IPlugin) + END_INTERFACE_MAP + + const string Initialize(PluginHost::IShell* shell) override + { + using Trigger = TestSupport::ReentrantTrigger; + const auto why = PluginHost::IShell::reason::REQUESTED; + + switch (TestSupport::g_reentrantTrigger) { + case Trigger::Activate: + TestSupport::g_reentrantResult = shell->Activate(why); + break; + case Trigger::Deactivate: + TestSupport::g_reentrantResult = shell->Deactivate(why); + break; + case Trigger::Unavailable: + TestSupport::g_reentrantResult = shell->Unavailable(why); + break; + case Trigger::Hibernate: + TestSupport::g_reentrantResult = shell->Hibernate(0); + break; + } + + return string(); // Initialize itself succeeds; the re-entry was rejected. + } + void Deinitialize(PluginHost::IShell* /* shell */) override {} + string Information() const override { return string(); } + }; + + SERVICE_REGISTRATION(ReentrantPlugin, 1, 0) + +} // namespace Plugin +} // namespace Thunder diff --git a/Tests/statemachine/StateControlControl.h b/Tests/statemachine/StateControlControl.h new file mode 100644 index 000000000..d43c0f300 --- /dev/null +++ b/Tests/statemachine/StateControlControl.h @@ -0,0 +1,21 @@ +#pragma once +#include +#include + +namespace Thunder { +namespace TestSupport { + // Observation surface for the in-process StateControl mock. Because the mock + // runs in the test process, the test reads these directly. (An OOP variant + // cannot, which is why these tests do not transfer to OOP.) + extern std::vector g_stateControlRequests; // command per Request() call: SUSPEND=0, RESUME=1 + extern int g_stateControlSinkCount; // currently registered INotification sinks + extern int g_stateControlNotifyCount; // StateChange notifications emitted to sinks + + inline void ResetStateControlObservation() + { + g_stateControlRequests.clear(); + g_stateControlSinkCount = 0; + g_stateControlNotifyCount = 0; + } +} +} diff --git a/Tests/statemachine/StateControlPlugin.cpp b/Tests/statemachine/StateControlPlugin.cpp new file mode 100644 index 000000000..5b9a1699c --- /dev/null +++ b/Tests/statemachine/StateControlPlugin.cpp @@ -0,0 +1,104 @@ +#include "Module.h" +#include "StateControlControl.h" + +#include + +namespace Thunder { + +namespace TestSupport { + std::vector g_stateControlRequests; + int g_stateControlSinkCount = 0; + int g_stateControlNotifyCount = 0; +} + +namespace Plugin { + + // In-process plugin that implements IStateControl so the Service's live + // state-control wiring (AcquireInterfaces -> Register(&_composit) -> auto + // RESUME if configured; Detach -> Unregister) can be characterized. + // + // It records every Request it receives and every sink registration, so the + // test can prove what the framework does and, crucially, does NOT do (no + // framework-driven SUSPEND). + // + // NOTE: the IStateControl signature below matches the canonical Thunder + // interface. Sanity-check command/state enum members and Configure()'s + // return type against your ThunderInterfaces version; older trees differ. + class StateControlPlugin : public PluginHost::IPlugin, public PluginHost::IStateControl { + public: + StateControlPlugin() + : _adminLock() + , _state(PluginHost::IStateControl::SUSPENDED) // start SUSPENDED so an auto-resume is observable + , _sinks() + { + } + ~StateControlPlugin() override = default; + StateControlPlugin(const StateControlPlugin&) = delete; + StateControlPlugin& operator=(const StateControlPlugin&) = delete; + + BEGIN_INTERFACE_MAP(StateControlPlugin) + INTERFACE_ENTRY(PluginHost::IPlugin) + INTERFACE_ENTRY(PluginHost::IStateControl) + END_INTERFACE_MAP + + // IPlugin + const string Initialize(PluginHost::IShell* /* shell */) override { return string(); } + void Deinitialize(PluginHost::IShell* /* shell */) override {} + string Information() const override { return string(); } + + // IStateControl + uint32_t Configure(PluginHost::IShell* /* framework */) override + { + return Core::ERROR_NONE; + } + PluginHost::IStateControl::state State() const override + { + return _state; + } + uint32_t Request(const PluginHost::IStateControl::command command) override + { + std::lock_guard guard(_adminLock); + TestSupport::g_stateControlRequests.push_back(static_cast(command)); + + _state = (command == PluginHost::IStateControl::SUSPEND) + ? PluginHost::IStateControl::SUSPENDED + : PluginHost::IStateControl::RESUMED; + + // Always notify so the test sees that the framework's sink is wired + // and the outbound StateChange path fires. + for (auto* sink : _sinks) { + TestSupport::g_stateControlNotifyCount++; + sink->StateChange(_state); + } + return Core::ERROR_NONE; + } + void Register(PluginHost::IStateControl::INotification* notification) override + { + std::lock_guard guard(_adminLock); + notification->AddRef(); + _sinks.push_back(notification); + TestSupport::g_stateControlSinkCount = static_cast(_sinks.size()); + } + void Unregister(PluginHost::IStateControl::INotification* notification) override + { + std::lock_guard guard(_adminLock); + for (auto it = _sinks.begin(); it != _sinks.end(); ++it) { + if (*it == notification) { + (*it)->Release(); + _sinks.erase(it); + break; + } + } + TestSupport::g_stateControlSinkCount = static_cast(_sinks.size()); + } + + private: + mutable std::mutex _adminLock; + PluginHost::IStateControl::state _state; + std::vector _sinks; + }; + + SERVICE_REGISTRATION(StateControlPlugin, 1, 0) + +} // namespace Plugin +} // namespace Thunder diff --git a/Tests/statemachine/StateMachineTest.cpp b/Tests/statemachine/StateMachineTest.cpp new file mode 100644 index 000000000..aad7f0e3a --- /dev/null +++ b/Tests/statemachine/StateMachineTest.cpp @@ -0,0 +1,759 @@ +#include "Module.h" + +#include "ObserverControl.h" +#include "ReentrantControl.h" +#include "StateControlControl.h" + +#include "Thunder/PluginServer.h" +#include "ThunderTestRuntime.h" + +#include +#include + +namespace Thunder { +namespace TestCore { + namespace Tests { + + class StateMachineBaseline : public ::testing::Test { + protected: + static ThunderTestRuntime _runtime; + + static void SetUpTestSuite() + { + std::vector plugins; + + ThunderTestRuntime::PluginConfig brave; + brave.Callsign = _T("Brave"); + brave.ClassName = _T("BravePlugin"); // must match the bare class name + brave.Locator = _T(""); // empty -> in-process lookup + brave.StartMode = PluginHost::IShell::startmode::DEACTIVATED; + plugins.push_back(brave); + + ThunderTestRuntime::PluginConfig broken; + broken.Callsign = _T("Broken"); + broken.ClassName = _T("DoesNotExist"); // not registered in-process + broken.Locator = _T(""); + broken.StartMode = PluginHost::IShell::startmode::DEACTIVATED; + plugins.push_back(broken); + + ThunderTestRuntime::PluginConfig failInit; + failInit.Callsign = _T("FailInit"); + failInit.ClassName = _T("FailInitPlugin"); + failInit.Locator = _T(""); + failInit.StartMode = PluginHost::IShell::startmode::DEACTIVATED; + plugins.push_back(failInit); + + ThunderTestRuntime::PluginConfig streamer; + streamer.Callsign = _T("Streamer"); + streamer.ClassName = _T("StreamingProvider"); + streamer.Locator = _T(TEST_PLUGIN_LOCATOR); // real .so from the build tree + streamer.StartMode = PluginHost::IShell::startmode::DEACTIVATED; + plugins.push_back(streamer); + + ThunderTestRuntime::PluginConfig depStreaming; + depStreaming.Callsign = _T("DepStreaming"); + depStreaming.ClassName = _T("BravePlugin"); + depStreaming.Locator = _T(""); + depStreaming.StartMode = PluginHost::IShell::startmode::DEACTIVATED; + depStreaming.Precondition.Add() = PluginHost::ISubSystem::STREAMING; + depStreaming.Termination.Add() = PluginHost::ISubSystem::NOT_STREAMING; + plugins.push_back(depStreaming); + + ThunderTestRuntime::PluginConfig reentrant; + reentrant.Callsign = _T("Reentrant"); + reentrant.ClassName = _T("ReentrantPlugin"); + reentrant.Locator = _T(""); + reentrant.StartMode = PluginHost::IShell::startmode::DEACTIVATED; + plugins.push_back(reentrant); + + ThunderTestRuntime::PluginConfig observer; + observer.Callsign = _T("Observer"); + observer.ClassName = _T("ObserverPlugin"); + observer.Locator = _T(""); + observer.StartMode = PluginHost::IShell::startmode::ACTIVATED; + plugins.push_back(observer); + + ThunderTestRuntime::PluginConfig stateCtrl; + stateCtrl.Callsign = _T("StateCtrl"); + stateCtrl.ClassName = _T("StateControlPlugin"); + stateCtrl.Locator = _T(""); + stateCtrl.StartMode = PluginHost::IShell::startmode::DEACTIVATED; + plugins.push_back(stateCtrl); + + ThunderTestRuntime::PluginConfig stateCtrlNoResume; + stateCtrlNoResume.Callsign = _T("StateCtrlNoResume"); + stateCtrlNoResume.ClassName = _T("StateControlPlugin"); + stateCtrlNoResume.Locator = _T(""); + stateCtrlNoResume.StartMode = PluginHost::IShell::startmode::DEACTIVATED; + stateCtrlNoResume.Resumed = false; + plugins.push_back(stateCtrlNoResume); + + ThunderTestRuntime::PluginConfig stateCtrlOOP; + stateCtrlOOP.Callsign = _T("StateCtrlOOP"); + stateCtrlOOP.ClassName = _T("StateControlOOPPlugin"); + stateCtrlOOP.Locator = _T(STATECTRL_OOP_LOCATOR); // real .so from the build tree + stateCtrlOOP.StartMode = PluginHost::IShell::startmode::DEACTIVATED; + plugins.push_back(stateCtrlOOP); + + // ASSERT_EQ(_runtime.Initialize(plugins), Core::ERROR_NONE); + ASSERT_EQ(_runtime.Initialize(plugins, _T(TEST_PLUGIN_DIR)), Core::ERROR_NONE); + } + + static void TearDownTestSuite() + { + _runtime.Deinitialize(); + Core::Singleton::Dispose(); + } + + static void EnsureDeactivated(const string& callsign) + { + auto shell = _runtime.GetShell(callsign); + ASSERT_TRUE(shell.IsValid()); + if (shell->State() != PluginHost::IShell::DEACTIVATED) { + shell->Deactivate(PluginHost::IShell::reason::REQUESTED); + } + ASSERT_EQ(shell->State(), PluginHost::IShell::DEACTIVATED); + } + + static bool WaitForState(const Core::ProxyType& shell, + const PluginHost::IShell::state expected, + const uint32_t timeoutMs = 5000) + { + uint32_t waited = 0; + while ((shell->State() != expected) && (waited < timeoutMs)) { + SleepMs(10); + waited += 10; + } + return (shell->State() == expected); + } + + void SetUp() override + { + // Dependent before provider: deactivating the provider unsets STREAMING, + // which would otherwise async-deactivate the dependent mid-teardown. + EnsureDeactivated("Brave"); + EnsureDeactivated("DepStreaming"); + EnsureDeactivated("Streamer"); + EnsureDeactivated("Reentrant"); + } + }; + + ThunderTestRuntime StateMachineBaseline::_runtime; + + TEST_F(StateMachineBaseline, BravePluginLoadsDeactivated) + { + auto shell = _runtime.GetShell("Brave"); + + ASSERT_TRUE(shell.IsValid()); + EXPECT_EQ(shell->State(), PluginHost::IShell::DEACTIVATED); + } + + TEST_F(StateMachineBaseline, ActivateThenDeactivateRoundTrip) + { + auto shell = _runtime.GetShell("Brave"); + + ASSERT_TRUE(shell.IsValid()); + + EXPECT_EQ(shell->Activate(PluginHost::IShell::reason::REQUESTED), Core::ERROR_NONE); + EXPECT_EQ(shell->State(), PluginHost::IShell::ACTIVATED); + + EXPECT_EQ(shell->Deactivate(PluginHost::IShell::reason::REQUESTED), Core::ERROR_NONE); + EXPECT_EQ(shell->State(), PluginHost::IShell::DEACTIVATED); + } + + TEST_F(StateMachineBaseline, IllegalTriggersFromDeactivated) + { + auto shell = _runtime.GetShell("Brave"); + ASSERT_TRUE(shell.IsValid()); + + EXPECT_EQ(shell->Deactivate(PluginHost::IShell::reason::REQUESTED), Core::ERROR_ILLEGAL_STATE); + +#ifdef HIBERNATE_SUPPORT_ENABLED + EXPECT_EQ(shell->Hibernate(0), Core::ERROR_ILLEGAL_STATE); +#else + EXPECT_EQ(shell->Hibernate(0), Core::ERROR_NOT_SUPPORTED); +#endif + + EXPECT_EQ(shell->State(), PluginHost::IShell::DEACTIVATED); + } + + TEST_F(StateMachineBaseline, IllegalTriggersFromActivated) + { + auto shell = _runtime.GetShell("Brave"); + ASSERT_TRUE(shell.IsValid()); + ASSERT_EQ(shell->Activate(PluginHost::IShell::reason::REQUESTED), Core::ERROR_NONE); + + EXPECT_EQ(shell->Activate(PluginHost::IShell::reason::REQUESTED), Core::ERROR_ILLEGAL_STATE); + EXPECT_EQ(shell->Unavailable(PluginHost::IShell::reason::REQUESTED), Core::ERROR_ILLEGAL_STATE); + EXPECT_EQ(shell->State(), PluginHost::IShell::ACTIVATED); + } + + TEST_F(StateMachineBaseline, ConditionsDeactivationEndsInPrecondition) + { + auto shell = _runtime.GetShell("Brave"); + ASSERT_TRUE(shell.IsValid()); + ASSERT_EQ(shell->Activate(PluginHost::IShell::reason::REQUESTED), Core::ERROR_NONE); + + EXPECT_EQ(shell->Deactivate(PluginHost::IShell::reason::CONDITIONS), Core::ERROR_NONE); + EXPECT_EQ(shell->State(), PluginHost::IShell::PRECONDITION); + } + + TEST_F(StateMachineBaseline, UnavailableFromDeactivated) + { + auto shell = _runtime.GetShell("Brave"); + ASSERT_TRUE(shell.IsValid()); + + EXPECT_EQ(shell->Unavailable(PluginHost::IShell::reason::REQUESTED), Core::ERROR_NONE); + EXPECT_EQ(shell->State(), PluginHost::IShell::UNAVAILABLE); + } + + TEST_F(StateMachineBaseline, IllegalActivateFromUnavailable) + { + auto shell = _runtime.GetShell("Brave"); + ASSERT_TRUE(shell.IsValid()); + ASSERT_EQ(shell->Unavailable(PluginHost::IShell::reason::REQUESTED), Core::ERROR_NONE); + ASSERT_EQ(shell->State(), PluginHost::IShell::UNAVAILABLE); + + EXPECT_EQ(shell->Activate(PluginHost::IShell::reason::REQUESTED), Core::ERROR_ILLEGAL_STATE); + EXPECT_EQ(shell->State(), PluginHost::IShell::UNAVAILABLE); + + EXPECT_EQ(shell->Deactivate(PluginHost::IShell::reason::REQUESTED), Core::ERROR_NONE); + EXPECT_EQ(shell->State(), PluginHost::IShell::DEACTIVATED); + } + + TEST_F(StateMachineBaseline, InstantiationFailureStaysDeactivated) + { + auto shell = _runtime.GetShell("Broken"); + ASSERT_TRUE(shell.IsValid()); + EXPECT_EQ(shell->State(), PluginHost::IShell::DEACTIVATED); + + EXPECT_EQ(shell->Activate(PluginHost::IShell::reason::REQUESTED), Core::ERROR_UNAVAILABLE); + EXPECT_EQ(shell->State(), PluginHost::IShell::DEACTIVATED); + } + + TEST_F(StateMachineBaseline, InitializationFailureEndsDeactivated) + { + auto shell = _runtime.GetShell("FailInit"); + ASSERT_TRUE(shell.IsValid()); + EXPECT_EQ(shell->State(), PluginHost::IShell::DEACTIVATED); + + EXPECT_EQ(shell->Activate(PluginHost::IShell::reason::REQUESTED), Core::ERROR_GENERAL); + EXPECT_EQ(shell->State(), PluginHost::IShell::DEACTIVATED); + EXPECT_EQ(shell->Reason(), PluginHost::IShell::reason::INITIALIZATION_FAILED); + } + + TEST_F(StateMachineBaseline, ActivatePendingWhenSubsystemDown) + { + auto dep = _runtime.GetShell("DepStreaming"); + ASSERT_TRUE(dep.IsValid()); + + // STREAMING is unset at baseline: the provider controls it and is deactivated. + EXPECT_EQ(dep->Activate(PluginHost::IShell::reason::REQUESTED), Core::ERROR_PENDING_CONDITIONS); + EXPECT_EQ(dep->State(), PluginHost::IShell::PRECONDITION); + + EXPECT_EQ(dep->Deactivate(PluginHost::IShell::reason::REQUESTED), Core::ERROR_NONE); + EXPECT_EQ(dep->State(), PluginHost::IShell::DEACTIVATED); + } + + TEST_F(StateMachineBaseline, DependentAutoDeactivatesWhenSubsystemDisappears) + { + auto provider = _runtime.GetShell("Streamer"); + auto dep = _runtime.GetShell("DepStreaming"); + ASSERT_TRUE(provider.IsValid()); + ASSERT_TRUE(dep.IsValid()); + + // Provider up -> sets STREAMING. + ASSERT_EQ(provider->Activate(PluginHost::IShell::reason::REQUESTED), Core::ERROR_NONE); + + // Dependent activates (precondition met). + ASSERT_EQ(dep->Activate(PluginHost::IShell::reason::REQUESTED), Core::ERROR_NONE); + ASSERT_EQ(dep->State(), PluginHost::IShell::ACTIVATED); + + // Provider down -> framework unsets STREAMING -> termination -> auto-deactivate. + ASSERT_EQ(provider->Deactivate(PluginHost::IShell::reason::REQUESTED), Core::ERROR_NONE); + + EXPECT_TRUE(WaitForState(dep, PluginHost::IShell::PRECONDITION)); + EXPECT_EQ(dep->State(), PluginHost::IShell::PRECONDITION); + } + + TEST_F(StateMachineBaseline, DependentAutoActivatesWhenSubsystemAppears) + { + auto provider = _runtime.GetShell("Streamer"); + auto dep = _runtime.GetShell("DepStreaming"); + ASSERT_TRUE(provider.IsValid()); + ASSERT_TRUE(dep.IsValid()); + + // STREAMING down at baseline -> dependent is pending. + ASSERT_EQ(dep->Activate(PluginHost::IShell::reason::REQUESTED), Core::ERROR_PENDING_CONDITIONS); + ASSERT_EQ(dep->State(), PluginHost::IShell::PRECONDITION); + + // Provider up -> sets STREAMING -> framework should auto-activate the dependent. + ASSERT_EQ(provider->Activate(PluginHost::IShell::reason::REQUESTED), Core::ERROR_NONE); + + EXPECT_TRUE(WaitForState(dep, PluginHost::IShell::ACTIVATED)); + EXPECT_EQ(dep->State(), PluginHost::IShell::ACTIVATED); + } + + TEST_F(StateMachineBaseline, ReentrantTriggerFromInitializeIsRejected) + { + auto shell = _runtime.GetShell("Reentrant"); + ASSERT_TRUE(shell.IsValid()); + + const std::vector triggers = { + TestSupport::ReentrantTrigger::Deactivate, // headline + TestSupport::ReentrantTrigger::Activate, +#ifdef HIBERNATE_SUPPORT_ENABLED + TestSupport::ReentrantTrigger::Hibernate, +#endif + TestSupport::ReentrantTrigger::Unavailable, + }; + + for (const TestSupport::ReentrantTrigger trigger : triggers) { + TestSupport::g_reentrantTrigger = trigger; + TestSupport::g_reentrantResult = Core::ERROR_NONE; + + // Activation completes; the re-entrant trigger is rejected by the guard. + EXPECT_EQ(shell->Activate(PluginHost::IShell::reason::REQUESTED), Core::ERROR_NONE); + EXPECT_EQ(shell->State(), PluginHost::IShell::ACTIVATED); + EXPECT_EQ(TestSupport::g_reentrantResult, Core::ERROR_ILLEGAL_STATE); + + EXPECT_EQ(shell->Deactivate(PluginHost::IShell::reason::REQUESTED), Core::ERROR_NONE); + EXPECT_EQ(shell->State(), PluginHost::IShell::DEACTIVATED); + } + } + + TEST_F(StateMachineBaseline, DeactivatedNotificationFiresWithHandlerResolvable) + { + auto shell = _runtime.GetShell("Brave"); + ASSERT_TRUE(shell.IsValid()); + ASSERT_EQ(shell->Activate(PluginHost::IShell::reason::REQUESTED), Core::ERROR_NONE); + + TestSupport::g_observedCallsign = "Brave"; + TestSupport::g_deactivatedFired = false; + TestSupport::g_shellQiWorked = false; + TestSupport::g_handlerQiResolved = false; + + ASSERT_EQ(shell->Deactivate(PluginHost::IShell::reason::REQUESTED), Core::ERROR_NONE); + + EXPECT_TRUE(TestSupport::g_deactivatedFired); + EXPECT_TRUE(TestSupport::g_shellQiWorked); // IShell stays resolvable + EXPECT_TRUE(TestSupport::g_handlerQiResolved); // handler QI forwards during DEACTIVATION + } + + namespace { + Core::ProxyType ToPrecondition(ThunderTestRuntime& runtime) + { + auto dep = runtime.GetShell("DepStreaming"); + EXPECT_TRUE(dep.IsValid()); + EXPECT_EQ(dep->Activate(PluginHost::IShell::reason::REQUESTED), Core::ERROR_PENDING_CONDITIONS); + EXPECT_EQ(dep->State(), PluginHost::IShell::PRECONDITION); + return dep; + } + } + + // ACTIVATED -> Hibernate on an in-process plugin: _connection is null, so the + // guard returns ERROR_INPROC before any state change. One of the few Hibernate + // branches reachable without an OOP plugin. + TEST_F(StateMachineBaseline, HibernateInProcessReturnsInProc) + { + auto shell = _runtime.GetShell("Brave"); + ASSERT_TRUE(shell.IsValid()); + ASSERT_EQ(shell->Activate(PluginHost::IShell::reason::REQUESTED), Core::ERROR_NONE); + +#ifdef HIBERNATE_SUPPORT_ENABLED + EXPECT_EQ(shell->Hibernate(0), Core::ERROR_INPROC); +#else + EXPECT_EQ(shell->Hibernate(0), Core::ERROR_NOT_SUPPORTED); +#endif + EXPECT_EQ(shell->State(), PluginHost::IShell::ACTIVATED); + } + + // PRECONDITION rejects Activate / Hibernate / Unavailable (inherited base + // ILLEGAL_STATE). Only Deactivate and Reevaluate are legal in this state. + TEST_F(StateMachineBaseline, IllegalTriggersFromPrecondition) + { + auto dep = ToPrecondition(_runtime); + + EXPECT_EQ(dep->Activate(PluginHost::IShell::reason::REQUESTED), Core::ERROR_ILLEGAL_STATE); +#ifdef HIBERNATE_SUPPORT_ENABLED + EXPECT_EQ(dep->Hibernate(0), Core::ERROR_ILLEGAL_STATE); +#else + EXPECT_EQ(dep->Hibernate(0), Core::ERROR_NOT_SUPPORTED); +#endif + EXPECT_EQ(dep->Unavailable(PluginHost::IShell::reason::REQUESTED), Core::ERROR_ILLEGAL_STATE); + EXPECT_EQ(dep->State(), PluginHost::IShell::PRECONDITION); + + EXPECT_EQ(dep->Deactivate(PluginHost::IShell::reason::REQUESTED), Core::ERROR_NONE); + EXPECT_EQ(dep->State(), PluginHost::IShell::DEACTIVATED); + } + + // Deactivate(CONDITIONS) from PRECONDITION unloads the plugin but stays in + // PRECONDITION (the CONDITIONS branch of PreconditionState::Deactivate). The + // non-CONDITIONS branch is already covered by ActivatePendingWhenSubsystemDown. + TEST_F(StateMachineBaseline, ConditionsDeactivationFromPreconditionStaysPrecondition) + { + auto dep = ToPrecondition(_runtime); + + EXPECT_EQ(dep->Deactivate(PluginHost::IShell::reason::CONDITIONS), Core::ERROR_NONE); + EXPECT_EQ(dep->State(), PluginHost::IShell::PRECONDITION); + + EXPECT_EQ(dep->Deactivate(PluginHost::IShell::reason::REQUESTED), Core::ERROR_NONE); + EXPECT_EQ(dep->State(), PluginHost::IShell::DEACTIVATED); + } + + // UNAVAILABLE rejects Hibernate / Unavailable (inherited ILLEGAL_STATE); only + // Deactivate is legal. Complements IllegalActivateFromUnavailable, which covers + // the Activate rejection. + TEST_F(StateMachineBaseline, IllegalTriggersFromUnavailable) + { + auto shell = _runtime.GetShell("Brave"); + ASSERT_TRUE(shell.IsValid()); + ASSERT_EQ(shell->Unavailable(PluginHost::IShell::reason::REQUESTED), Core::ERROR_NONE); + ASSERT_EQ(shell->State(), PluginHost::IShell::UNAVAILABLE); + +#ifdef HIBERNATE_SUPPORT_ENABLED + EXPECT_EQ(shell->Hibernate(0), Core::ERROR_ILLEGAL_STATE); +#else + EXPECT_EQ(shell->Hibernate(0), Core::ERROR_NOT_SUPPORTED); +#endif + + EXPECT_EQ(shell->Unavailable(PluginHost::IShell::reason::REQUESTED), Core::ERROR_ILLEGAL_STATE); + EXPECT_EQ(shell->State(), PluginHost::IShell::UNAVAILABLE); + + EXPECT_EQ(shell->Deactivate(PluginHost::IShell::reason::REQUESTED), Core::ERROR_NONE); + EXPECT_EQ(shell->State(), PluginHost::IShell::DEACTIVATED); + } + + // QueryInterface dispatch per state: DEACTIVATED and PRECONDITION return null + // for the handler interface; ACTIVATED forwards to the live handler. Pins the + // per-state QI contract so a later refactor cannot silently change it. + TEST_F(StateMachineBaseline, QueryInterfaceNullWhenDeactivated) + { + auto* plugin = _runtime.QueryInterfaceByCallsign("Brave"); + EXPECT_EQ(plugin, nullptr); + if (plugin != nullptr) { + plugin->Release(); + } + } + + TEST_F(StateMachineBaseline, QueryInterfaceValidWhenActivated) + { + auto shell = _runtime.GetShell("Brave"); + ASSERT_TRUE(shell.IsValid()); + ASSERT_EQ(shell->Activate(PluginHost::IShell::reason::REQUESTED), Core::ERROR_NONE); + + auto* plugin = _runtime.QueryInterfaceByCallsign("Brave"); + EXPECT_NE(plugin, nullptr); + if (plugin != nullptr) { + plugin->Release(); + } + } + + TEST_F(StateMachineBaseline, QueryInterfaceNullWhenPrecondition) + { + ToPrecondition(_runtime); + + auto* plugin = _runtime.QueryInterfaceByCallsign("DepStreaming"); + EXPECT_EQ(plugin, nullptr); + if (plugin != nullptr) { + plugin->Release(); + } + } + + // On Activate the Service acquires IStateControl and registers its Composit + // sink. Pins that the live wiring is established. + TEST_F(StateMachineBaseline, StateControlSinkRegisteredOnActivate) + { + TestSupport::ResetStateControlObservation(); + + auto shell = _runtime.GetShell("StateCtrl"); + ASSERT_TRUE(shell.IsValid()); + ASSERT_EQ(shell->Activate(PluginHost::IShell::reason::REQUESTED), Core::ERROR_NONE); + + EXPECT_EQ(TestSupport::g_stateControlSinkCount, 1); + + EXPECT_EQ(shell->Deactivate(PluginHost::IShell::reason::REQUESTED), Core::ERROR_NONE); + } + + // A state change requested directly on the live interface propagates to the + // registered framework sink (Composit::StateChange -> StateControlStateChange). + // Pins the outbound notification path that must survive any cleanup. + TEST_F(StateMachineBaseline, StateControlChangePropagatesToFrameworkSink) + { + TestSupport::ResetStateControlObservation(); + + auto shell = _runtime.GetShell("StateCtrl"); + ASSERT_TRUE(shell.IsValid()); + ASSERT_EQ(shell->Activate(PluginHost::IShell::reason::REQUESTED), Core::ERROR_NONE); + + auto* control = _runtime.QueryInterfaceByCallsign("StateCtrl"); + ASSERT_NE(control, nullptr); + + const int before = TestSupport::g_stateControlNotifyCount; + EXPECT_EQ(control->Request(PluginHost::IStateControl::SUSPEND), Core::ERROR_NONE); + EXPECT_EQ(control->State(), PluginHost::IStateControl::SUSPENDED); + EXPECT_GT(TestSupport::g_stateControlNotifyCount, before); // framework sink was notified + + control->Release(); + EXPECT_EQ(shell->Deactivate(PluginHost::IShell::reason::REQUESTED), Core::ERROR_NONE); + } + + // On Deactivate the sink is unregistered (Detach) before teardown. Pins the + // lifetime ordering: no sink is left dangling over DeinitializePlugin/UnloadPlugin. + TEST_F(StateMachineBaseline, StateControlSinkUnregisteredOnDeactivate) + { + TestSupport::ResetStateControlObservation(); + + auto shell = _runtime.GetShell("StateCtrl"); + ASSERT_TRUE(shell.IsValid()); + ASSERT_EQ(shell->Activate(PluginHost::IShell::reason::REQUESTED), Core::ERROR_NONE); + ASSERT_EQ(TestSupport::g_stateControlSinkCount, 1); + + ASSERT_EQ(shell->Deactivate(PluginHost::IShell::reason::REQUESTED), Core::ERROR_NONE); + EXPECT_EQ(TestSupport::g_stateControlSinkCount, 0); + } + + // The money test for the removal question: over a full activate/deactivate + // cycle the framework must never drive a SUSPEND into the plugin. If this holds, + // RequestSuspend has no behavioural role and the helpers are safe to delete. + TEST_F(StateMachineBaseline, FrameworkNeverDrivesSuspend) + { + TestSupport::ResetStateControlObservation(); + + auto shell = _runtime.GetShell("StateCtrl"); + ASSERT_TRUE(shell.IsValid()); + ASSERT_EQ(shell->Activate(PluginHost::IShell::reason::REQUESTED), Core::ERROR_NONE); + + // string response; + + // const std::string params = "{\"callsign\": \"StateCtrl\"}"; + + // ASSERT_EQ(_runtime.Invoke("Controller.resume", params, response), Core::ERROR_NONE); + // ASSERT_EQ(_runtime.Invoke("Controller.suspend", params, response), Core::ERROR_NONE); + + ASSERT_EQ(shell->Deactivate(PluginHost::IShell::reason::REQUESTED), Core::ERROR_NONE); + + for (int command : TestSupport::g_stateControlRequests) { + EXPECT_NE(command, static_cast(PluginHost::IStateControl::SUSPEND)) + << "framework issued a SUSPEND during the lifecycle"; + } + } + + TEST_F(StateMachineBaseline, PluginStartSuspendedAndResume) + { + TestSupport::ResetStateControlObservation(); + + auto shell = _runtime.GetShell("StateCtrlNoResume"); + ASSERT_TRUE(shell.IsValid()); + + ASSERT_EQ(shell->Activate(PluginHost::IShell::reason::REQUESTED), Core::ERROR_NONE); + + EXPECT_EQ(TestSupport::g_stateControlSinkCount, 1); + EXPECT_TRUE(TestSupport::g_stateControlRequests.empty()); + + string response; + const string params = _T("{\"callsign\":\"StateCtrlNoResume\"}"); + + ASSERT_EQ(_runtime.Invoke(_T("Controller.resume"), params, response), Core::ERROR_NONE); + ASSERT_EQ(TestSupport::g_stateControlRequests.size(), 1u); + EXPECT_EQ(TestSupport::g_stateControlRequests.back(), static_cast(PluginHost::IStateControl::RESUME)); + + ASSERT_EQ(_runtime.Invoke(_T("Controller.suspend"), params, response), Core::ERROR_NONE); + ASSERT_EQ(TestSupport::g_stateControlRequests.size(), 2u); + EXPECT_EQ(TestSupport::g_stateControlRequests.back(), static_cast(PluginHost::IStateControl::SUSPEND)); + + ASSERT_EQ(shell->Deactivate(PluginHost::IShell::reason::REQUESTED), Core::ERROR_NONE); + + // Detach heeft de sink uitgeregistreerd vóór teardown. + EXPECT_EQ(TestSupport::g_stateControlSinkCount, 0); + } + + // 1) The Controller JSON-RPC lifecycle drives the same state transitions as the + // direct IShell triggers. Pins the path real clients use against the direct path. + TEST_F(StateMachineBaseline, ControllerLifecycleMatchesShell) + { + auto shell = _runtime.GetShell("Brave"); + ASSERT_TRUE(shell.IsValid()); + + string response; + const string params = _T("{\"callsign\":\"Brave\"}"); + + ASSERT_EQ(_runtime.Invoke(_T("Controller.activate"), params, response), Core::ERROR_NONE); + EXPECT_TRUE(WaitForState(shell, PluginHost::IShell::ACTIVATED)); + + ASSERT_EQ(_runtime.Invoke(_T("Controller.deactivate"), params, response), Core::ERROR_NONE); + EXPECT_TRUE(WaitForState(shell, PluginHost::IShell::DEACTIVATED)); + + ASSERT_EQ(_runtime.Invoke(_T("Controller.unavailable"), params, response), Core::ERROR_NONE); + EXPECT_TRUE(WaitForState(shell, PluginHost::IShell::UNAVAILABLE)); + + // Recovery: deactivate is the only legal trigger out of UNAVAILABLE. + ASSERT_EQ(_runtime.Invoke(_T("Controller.deactivate"), params, response), Core::ERROR_NONE); + EXPECT_TRUE(WaitForState(shell, PluginHost::IShell::DEACTIVATED)); + } + + // 2) Illegal lifecycle operations surface their error over JSON-RPC instead of + // being swallowed. Grounded, no assumptions: + // - Controller::Deactivate preserves ERROR_ILLEGAL_STATE and maps an unknown + // callsign to ERROR_UNKNOWN_KEY (Controller.cpp 1017-1041). + // - ThunderTestRuntime::Invoke returns dispatcher->Invoke's result, i.e. the + // method hresult; the passing ControllerLifecycleMatchesShell test confirms + // the method is actually invoked and ERROR_NONE round-trips. + // + // If a case ever comes back ERROR_NONE with the error in the response body + // instead, move that assertion to parsing `response`; the EXPECT_EQ failure will + // show the actual returned code. + TEST_F(StateMachineBaseline, ControllerDeactivateSurfacesErrorCodes) + { + auto shell = _runtime.GetShell("Brave"); + ASSERT_TRUE(shell.IsValid()); + ASSERT_EQ(shell->State(), PluginHost::IShell::DEACTIVATED); + + string response; + + // Already DEACTIVATED -> ILLEGAL_STATE, preserved by the Controller. + EXPECT_EQ(_runtime.Invoke(_T("Controller.deactivate"), _T("{\"callsign\":\"Brave\"}"), response), + Core::ERROR_ILLEGAL_STATE); + + // Unknown callsign -> UNKNOWN_KEY. + EXPECT_EQ(_runtime.Invoke(_T("Controller.deactivate"), _T("{\"callsign\":\"NoSuchPlugin\"}"), response), + Core::ERROR_UNKNOWN_KEY); + + // Legal deactivate of an ACTIVATED plugin -> ERROR_NONE (contrast). + ASSERT_EQ(shell->Activate(PluginHost::IShell::reason::REQUESTED), Core::ERROR_NONE); + EXPECT_EQ(_runtime.Invoke(_T("Controller.deactivate"), _T("{\"callsign\":\"Brave\"}"), response), + Core::ERROR_NONE); + EXPECT_EQ(shell->State(), PluginHost::IShell::DEACTIVATED); + } + + // 3) The deactivation reason round-trips into Reason(). Two distinct, confirmed + // reasons prove Reason() reflects the trigger rather than a hardcoded value, and + // that the CONDITIONS reason also routes to PRECONDITION rather than DEACTIVATED. + TEST_F(StateMachineBaseline, DeactivationReasonIsRecorded) + { + auto shell = _runtime.GetShell("Brave"); + ASSERT_TRUE(shell.IsValid()); + + ASSERT_EQ(shell->Activate(PluginHost::IShell::reason::REQUESTED), Core::ERROR_NONE); + ASSERT_EQ(shell->Deactivate(PluginHost::IShell::reason::REQUESTED), Core::ERROR_NONE); + EXPECT_EQ(shell->State(), PluginHost::IShell::DEACTIVATED); + EXPECT_EQ(shell->Reason(), PluginHost::IShell::reason::REQUESTED); + + ASSERT_EQ(shell->Activate(PluginHost::IShell::reason::REQUESTED), Core::ERROR_NONE); + ASSERT_EQ(shell->Deactivate(PluginHost::IShell::reason::CONDITIONS), Core::ERROR_NONE); + EXPECT_EQ(shell->State(), PluginHost::IShell::PRECONDITION); + EXPECT_EQ(shell->Reason(), PluginHost::IShell::reason::CONDITIONS); + + EXPECT_EQ(shell->Deactivate(PluginHost::IShell::reason::REQUESTED), Core::ERROR_NONE); // cleanup + } + + TEST_F(StateMachineBaseline, SubsystemCascadeRoundTrip) + { + auto provider = _runtime.GetShell("Streamer"); + auto dep = _runtime.GetShell("DepStreaming"); + ASSERT_TRUE(provider.IsValid()); + ASSERT_TRUE(dep.IsValid()); + + // Provider up -> STREAMING set -> dependent activates synchronously. + ASSERT_EQ(provider->Activate(PluginHost::IShell::reason::REQUESTED), Core::ERROR_NONE); + ASSERT_EQ(dep->Activate(PluginHost::IShell::reason::REQUESTED), Core::ERROR_NONE); + ASSERT_EQ(dep->State(), PluginHost::IShell::ACTIVATED); + + // Provider down -> STREAMING unset -> termination -> async auto-deactivate. + ASSERT_EQ(provider->Deactivate(PluginHost::IShell::reason::REQUESTED), Core::ERROR_NONE); + EXPECT_TRUE(WaitForState(dep, PluginHost::IShell::PRECONDITION)); + EXPECT_EQ(dep->Reason(), PluginHost::IShell::reason::CONDITIONS); + + // Provider up again -> dependent auto-activates again, no manual Activate. + ASSERT_EQ(provider->Activate(PluginHost::IShell::reason::REQUESTED), Core::ERROR_NONE); + EXPECT_TRUE(WaitForState(dep, PluginHost::IShell::ACTIVATED)); + + // Provider down again -> back to PRECONDITION. Repeatable, no leaked state. + ASSERT_EQ(provider->Deactivate(PluginHost::IShell::reason::REQUESTED), Core::ERROR_NONE); + EXPECT_TRUE(WaitForState(dep, PluginHost::IShell::PRECONDITION)); + + EXPECT_EQ(dep->Deactivate(PluginHost::IShell::reason::REQUESTED), Core::ERROR_NONE); // cleanup + } + + // Guards the precondition-freshness fix across repeated cycling. A single + // round-trip proves the fix once; edge-triggered conditions are prone to drift + // that only shows after several cycles, so this drives the controlling subsystem + // up and down repeatedly and asserts the dependent tracks it every time. + // + // Pairs with SubsystemReactivatesAfterTerminationDeactivation (the single + // round-trip). Both rely on the entry-point evaluating BOTH conditions on every + // event (StateMachine::Reevaluate), so the dependent's _precondition stays fresh + // while it is ACTIVATED and re-arms when the subsystem reappears. + TEST_F(StateMachineBaseline, SubsystemCascadeSurvivesRepeatedCycling) + { + auto provider = _runtime.GetShell("Streamer"); + auto dep = _runtime.GetShell("DepStreaming"); + ASSERT_TRUE(provider.IsValid()); + ASSERT_TRUE(dep.IsValid()); + + // Bring the dependent up the first time. + ASSERT_EQ(provider->Activate(PluginHost::IShell::reason::REQUESTED), Core::ERROR_NONE); + ASSERT_EQ(dep->Activate(PluginHost::IShell::reason::REQUESTED), Core::ERROR_NONE); + ASSERT_EQ(dep->State(), PluginHost::IShell::ACTIVATED); + + for (int cycle = 0; cycle < 5; ++cycle) { + ASSERT_EQ(provider->Deactivate(PluginHost::IShell::reason::REQUESTED), Core::ERROR_NONE); + ASSERT_TRUE(WaitForState(dep, PluginHost::IShell::PRECONDITION)) << "down, cycle " << cycle; + EXPECT_EQ(dep->Reason(), PluginHost::IShell::reason::CONDITIONS) << "cycle " << cycle; + + ASSERT_EQ(provider->Activate(PluginHost::IShell::reason::REQUESTED), Core::ERROR_NONE); + ASSERT_TRUE(WaitForState(dep, PluginHost::IShell::ACTIVATED)) << "up, cycle " << cycle; + } + + // cleanup + ASSERT_EQ(provider->Deactivate(PluginHost::IShell::reason::REQUESTED), Core::ERROR_NONE); + ASSERT_TRUE(WaitForState(dep, PluginHost::IShell::PRECONDITION)); + dep->Deactivate(PluginHost::IShell::reason::REQUESTED); + } + + // 6) The Controller emits its lifecycle "statechange" event to a JSON-RPC + // subscriber. Closes the outbound half: a state transition is not only applied + // but announced. Event name verified against JLifeTime.h (registered as + // "statechange", line 152/211); the payload carries the callsign, which is what + // the handler checks, so no assumption is made about the exact JSON structure. + // + // Requires , and (add the includes if the harness does + // not already pull them in). + TEST_F(StateMachineBaseline, ControllerEmitsStateChangeEvent) + { + auto link = _runtime.CreateJSONRPCLink("Controller"); + ASSERT_TRUE(link.IsValid()); + + std::atomic eventCount{ 0 }; + std::atomic sawBrave{ false }; + + ASSERT_EQ(link->Subscribe(_T("statechange"), + [&](const string& /* designator */, const string& /* index */, const string& params) { + eventCount++; + if (params.find(_T("Brave")) != string::npos) { + sawBrave = true; + } + }), + Core::ERROR_NONE); + + string response; + ASSERT_EQ(_runtime.Invoke(_T("Controller.activate"), _T("{\"callsign\":\"Brave\"}"), response), Core::ERROR_NONE); + + // Delivered asynchronously; bounded poll rather than assuming a WaitFor helper. + for (int i = 0; (i < 200) && (sawBrave.load() == false); ++i) { + std::this_thread::sleep_for(std::chrono::milliseconds(25)); + } + + EXPECT_TRUE(sawBrave.load()) + << "no 'statechange' event carrying 'Brave' arrived after " << eventCount.load() << " events"; + + link->Unsubscribe(_T("statechange")); + + auto shell = _runtime.GetShell("Brave"); + if (shell.IsValid()) { + shell->Deactivate(PluginHost::IShell::reason::REQUESTED); + } + } + } // namespace Tests +} // namespace TestCore +} // namespace Thunder diff --git a/Tests/statemachine/provider/CMakeLists.txt b/Tests/statemachine/provider/CMakeLists.txt new file mode 100644 index 000000000..7debaf990 --- /dev/null +++ b/Tests/statemachine/provider/CMakeLists.txt @@ -0,0 +1,31 @@ +set(PLUGIN_TARGET ThunderTestStreamingProvider) + +add_library(${PLUGIN_TARGET} SHARED + StreamingProvider.cpp + Module.cpp +) + +target_compile_definitions(${PLUGIN_TARGET} + PRIVATE + MODULE_NAME=StreamingProvider +) + +if(BUILD_REFERENCE) + target_compile_definitions(${PLUGIN_TARGET} PRIVATE BUILD_REFERENCE=${BUILD_REFERENCE}) +endif() + +target_link_libraries(${PLUGIN_TARGET} + PRIVATE + ${NAMESPACE}Core::${NAMESPACE}Core + ${NAMESPACE}COM::${NAMESPACE}COM + ${NAMESPACE}Messaging::${NAMESPACE}Messaging + ${NAMESPACE}Plugins::${NAMESPACE}Plugins +) + +add_custom_command(TARGET ThunderTestStreamingProvider POST_BUILD + COMMAND ${CMAKE_COMMAND} -E make_directory "${PLUGIN_DIR}" + COMMAND ${CMAKE_COMMAND} -E create_symlink + "$" + "${PLUGIN_DIR}/$" + COMMENT "Creating symlink for ThunderTestStreamingProvider" +) diff --git a/Tests/statemachine/provider/Module.cpp b/Tests/statemachine/provider/Module.cpp new file mode 100644 index 000000000..714f45038 --- /dev/null +++ b/Tests/statemachine/provider/Module.cpp @@ -0,0 +1,3 @@ +#include "Module.h" + +MODULE_NAME_DECLARATION(BUILD_REFERENCE) \ No newline at end of file diff --git a/Tests/statemachine/provider/Module.h b/Tests/statemachine/provider/Module.h new file mode 100644 index 000000000..f6525fef5 --- /dev/null +++ b/Tests/statemachine/provider/Module.h @@ -0,0 +1,10 @@ +#pragma once + +#ifndef MODULE_NAME +#define MODULE_NAME StreamingProvider +#endif + +#include + +#undef EXTERNAL +#define EXTERNAL \ No newline at end of file diff --git a/Tests/statemachine/provider/StreamingProvider.cpp b/Tests/statemachine/provider/StreamingProvider.cpp new file mode 100644 index 000000000..7fa61665a --- /dev/null +++ b/Tests/statemachine/provider/StreamingProvider.cpp @@ -0,0 +1,50 @@ +#include "Module.h" + +namespace Thunder { +namespace Plugin { + + // Real .so plugin that CONTROLS the STREAMING subsystem. Because it has a + // non-empty locator, LoadMetadata fills _control, so STREAMING is treated as + // externally controlled and is unset at startup. + class StreamingProvider : public PluginHost::IPlugin { + public: + StreamingProvider() = default; + ~StreamingProvider() override = default; + StreamingProvider(const StreamingProvider&) = delete; + StreamingProvider& operator=(const StreamingProvider&) = delete; + + BEGIN_INTERFACE_MAP(StreamingProvider) + INTERFACE_ENTRY(PluginHost::IPlugin) + END_INTERFACE_MAP + + const string Initialize(PluginHost::IShell* shell) override + { + PluginHost::ISubSystem* subSystem = shell->SubSystems(); + if (subSystem != nullptr) { + subSystem->Set(PluginHost::ISubSystem::STREAMING, nullptr); // controlled -> allowed + subSystem->Release(); + } + return string(); + } + void Deinitialize(PluginHost::IShell* /* shell */) override + { + // The framework unsets controlled subsystems on deactivation + // (PluginServer.cpp, SubSystemControl loop), so nothing to do here. + } + string Information() const override + { + return string(); + } + }; + + namespace { + static Metadata metadata( + 1, 0, 0, + {}, // preconditions + {}, // terminations + { PluginHost::ISubSystem::STREAMING } // controls + ); + } + +} // namespace Plugin +} // namespace Thunder diff --git a/Tests/statemachine/statecontrol_oop/CMakeLists.txt b/Tests/statemachine/statecontrol_oop/CMakeLists.txt new file mode 100644 index 000000000..4160f48dc --- /dev/null +++ b/Tests/statemachine/statecontrol_oop/CMakeLists.txt @@ -0,0 +1,32 @@ +set(PLUGIN_TARGET ThunderTestStateControlOOP) + +add_library(${PLUGIN_TARGET} SHARED + StateControlOOPPlugin.cpp + StateControlImplementation.cpp + Module.cpp +) + +target_compile_definitions(${PLUGIN_TARGET} + PRIVATE + MODULE_NAME=StateControlOOP +) + +if(BUILD_REFERENCE) + target_compile_definitions(${PLUGIN_TARGET} PRIVATE BUILD_REFERENCE=${BUILD_REFERENCE}) +endif() + +target_link_libraries(${PLUGIN_TARGET} + PRIVATE + ${NAMESPACE}Core::${NAMESPACE}Core + ${NAMESPACE}COM::${NAMESPACE}COM + ${NAMESPACE}Messaging::${NAMESPACE}Messaging + ${NAMESPACE}Plugins::${NAMESPACE}Plugins +) + +add_custom_command(TARGET ThunderTestStateControlOOP POST_BUILD + COMMAND ${CMAKE_COMMAND} -E make_directory "${PLUGIN_DIR}" + COMMAND ${CMAKE_COMMAND} -E create_symlink + "$" + "${PLUGIN_DIR}/$" + COMMENT "Creating symlink for ThunderTestStateControlOOP" +) diff --git a/Tests/statemachine/statecontrol_oop/Module.cpp b/Tests/statemachine/statecontrol_oop/Module.cpp new file mode 100644 index 000000000..714f45038 --- /dev/null +++ b/Tests/statemachine/statecontrol_oop/Module.cpp @@ -0,0 +1,3 @@ +#include "Module.h" + +MODULE_NAME_DECLARATION(BUILD_REFERENCE) \ No newline at end of file diff --git a/Tests/statemachine/statecontrol_oop/Module.h b/Tests/statemachine/statecontrol_oop/Module.h new file mode 100644 index 000000000..f6525fef5 --- /dev/null +++ b/Tests/statemachine/statecontrol_oop/Module.h @@ -0,0 +1,10 @@ +#pragma once + +#ifndef MODULE_NAME +#define MODULE_NAME StreamingProvider +#endif + +#include + +#undef EXTERNAL +#define EXTERNAL \ No newline at end of file diff --git a/Tests/statemachine/statecontrol_oop/StateControlImplementation.cpp b/Tests/statemachine/statecontrol_oop/StateControlImplementation.cpp new file mode 100644 index 000000000..7089e7dcb --- /dev/null +++ b/Tests/statemachine/statecontrol_oop/StateControlImplementation.cpp @@ -0,0 +1,68 @@ +#include "Module.h" +#include +#include + +namespace Thunder { +namespace Plugin { + + // Out-of-process implementation of IStateControl. Lives in the child process, + // so its state is only observable from the parent via State() and via the + // StateChange notifications it emits to the (proxied) registered sink. + class StateControlImplementation : public PluginHost::IStateControl { + public: + StateControlImplementation() + : _adminLock() + , _state(PluginHost::IStateControl::SUSPENDED) + , _sinks() + { + } + ~StateControlImplementation() override = default; + StateControlImplementation(const StateControlImplementation&) = delete; + StateControlImplementation& operator=(const StateControlImplementation&) = delete; + + BEGIN_INTERFACE_MAP(StateControlImplementation) + INTERFACE_ENTRY(PluginHost::IStateControl) + END_INTERFACE_MAP + + uint32_t Configure(PluginHost::IShell* /* framework */) override { return Core::ERROR_NONE; } + PluginHost::IStateControl::state State() const override { return _state; } + + uint32_t Request(const PluginHost::IStateControl::command command) override + { + std::lock_guard guard(_adminLock); + _state = (command == PluginHost::IStateControl::SUSPEND) + ? PluginHost::IStateControl::SUSPENDED + : PluginHost::IStateControl::RESUMED; + for (auto* sink : _sinks) { + sink->StateChange(_state); + } + return Core::ERROR_NONE; + } + void Register(PluginHost::IStateControl::INotification* notification) override + { + std::lock_guard guard(_adminLock); + notification->AddRef(); + _sinks.push_back(notification); + } + void Unregister(PluginHost::IStateControl::INotification* notification) override + { + std::lock_guard guard(_adminLock); + for (auto it = _sinks.begin(); it != _sinks.end(); ++it) { + if (*it == notification) { + (*it)->Release(); + _sinks.erase(it); + break; + } + } + } + + private: + mutable std::mutex _adminLock; + PluginHost::IStateControl::state _state; + std::vector _sinks; + }; + + SERVICE_REGISTRATION(StateControlImplementation, 1, 0) + +} // namespace Plugin +} // namespace Thunder diff --git a/Tests/statemachine/statecontrol_oop/StateControlOOPPlugin.cpp b/Tests/statemachine/statecontrol_oop/StateControlOOPPlugin.cpp new file mode 100644 index 000000000..6b9017129 --- /dev/null +++ b/Tests/statemachine/statecontrol_oop/StateControlOOPPlugin.cpp @@ -0,0 +1,67 @@ +#include "Module.h" + +namespace Thunder { +namespace Plugin { + + // OOP variant: the shell spawns an out-of-process implementation that owns + // IStateControl and aggregates it so the framework's + // _handler->QueryInterface() resolves across the process + // boundary. + // + // CAVEAT (read before relying on this): the implementation runs in a child + // process, so the test process CANNOT read its request log. Only State() + // (marshalled) and the outbound StateControlStateChange notification are + // observable in the parent. The in-process tests that assert on + // g_stateControlRequests / g_stateControlSinkCount therefore do NOT transfer. + // For the "framework never drives SUSPEND" question, in-process is the + // working harness; this skeleton is here for the spawn/connection path only. + // + // BUILD ASSUMPTIONS to confirm against your tree: + // - IStateControl proxy-stubs are available on the proxyStubPath the runtime + // is initialized with. + // - HOSTING_COMPROCESS points at a usable child host (ThunderPlugin). + // - "StateControlImplementation" matches the SERVICE_REGISTRATION name below. + class StateControlOOPPlugin : public PluginHost::IPlugin { + public: + StateControlOOPPlugin() + : _connectionId(0) + , _impl(nullptr) + { + } + ~StateControlOOPPlugin() override = default; + StateControlOOPPlugin(const StateControlOOPPlugin&) = delete; + StateControlOOPPlugin& operator=(const StateControlOOPPlugin&) = delete; + + BEGIN_INTERFACE_MAP(StateControlOOPPlugin) + INTERFACE_ENTRY(PluginHost::IPlugin) + INTERFACE_AGGREGATE(PluginHost::IStateControl, _impl) + END_INTERFACE_MAP + + const string Initialize(PluginHost::IShell* shell) override + { + _impl = shell->Root(_connectionId, Core::infinite, _T("StateControlImplementation")); + return (_impl != nullptr) ? string() : _T("could not spawn StateControlImplementation"); + } + void Deinitialize(PluginHost::IShell* shell) override + { + if (_impl != nullptr) { + RPC::IRemoteConnection* connection = shell->RemoteConnection(_connectionId); + _impl->Release(); + _impl = nullptr; + if (connection != nullptr) { + connection->Terminate(); + connection->Release(); + } + } + } + string Information() const override { return string(); } + + private: + uint32_t _connectionId; + PluginHost::IStateControl* _impl; + }; + + SERVICE_REGISTRATION(StateControlOOPPlugin, 1, 0) + +} // namespace Plugin +} // namespace Thunder diff --git a/Tests/statemachine/statecontrol_oop/StateControlTests.cpp b/Tests/statemachine/statecontrol_oop/StateControlTests.cpp new file mode 100644 index 000000000..5994b79d4 --- /dev/null +++ b/Tests/statemachine/statecontrol_oop/StateControlTests.cpp @@ -0,0 +1,22 @@ +// =========================================================================== +// IStateControl wiring characterization. Answers "can the framework-driven +// Suspend/Resume helpers go away" by pinning that the only live state-control +// path is the Composit notification wiring, and that no framework path drives +// SUSPEND into a plugin. +// =========================================================================== + + + + + +// OPEN: auto-resume-on-activate pin. The Service issues Request(RESUME) on +// activate only when Resumed() is true. I could not confirm from the source how +// Resumed() is set via PluginConfig in your tree (a startmode::RESUMED value, or +// a separate Resumed/StartupOrder flag on Plugin::Config). Tell me which, and +// this becomes: +// +// TEST_F(StateMachineBaseline, AutoResumeIssuedWhenConfiguredResumed) { +// // configure a second callsign with Resumed == true, activate it, then: +// ASSERT_EQ(g_stateControlRequests.size(), 1u); +// EXPECT_EQ(g_stateControlRequests[0], (int)PluginHost::IStateControl::RESUME); +// } From 3c89704a846125537a37b385595e0d177e8a2a50 Mon Sep 17 00:00:00 2001 From: Bram Oosterhuis Date: Mon, 22 Jun 2026 19:52:17 +0200 Subject: [PATCH 26/26] refactor the cmake coverage logic to modern standards --- CMakeLists.txt | 38 ++- cmake/common/CodeCoverage.cmake | 420 ++++++++------------------------ 2 files changed, 131 insertions(+), 327 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 9aa20630c..e27418c63 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -137,15 +137,35 @@ if(APPLE) set(CMAKE_INSTALL_RPATH_USE_LINK_PATH TRUE) endif() -if (ENABLE_CODE_COVERAGE) - include (CodeCoverage) - append_coverage_compiler_flags() - set(COVERAGE_EXCLUDES - "${CMAKE_CURRENT_BINARY_DIR}/Tests/unit*" - "${CMAKE_CURRENT_BINARY_DIR}/Tests/unit/core/*") - setup_target_for_coverage_gcovr_html(NAME "coverage") - setup_target_for_coverage_gcovr_xml(NAME "coverage-report") - add_definitions(-DWITH_CODE_COVERAGE=1) +include(CMakeDependentOption) + +cmake_dependent_option(ENABLE_CODE_COVERAGE "Enable gcov/gcovr code coverage instrumentation" OFF + "CMAKE_BUILD_TYPE STREQUAL Debug" OFF) + +if(ENABLE_CODE_COVERAGE) + message(STATUS "Code coverage instrumentation enabled") + + include(CodeCoverage) + + append_coverage_compiler_flags() + + set(COVERAGE_EXCLUDES + "${CMAKE_CURRENT_BINARY_DIR}/Tests/unit*" + "${CMAKE_CURRENT_BINARY_DIR}/Tests/unit/core/*" + "${CMAKE_CURRENT_BINARY_DIR}/Tests/statemachine/*" + ) + setup_target_for_coverage_gcovr_html( + NAME "coverage-html-report" + GCOVR_ARGS --gcov-ignore-parse-errors=negative_hits.warn_once_per_file + EXCLUDE_THROW_BRANCHES ON + EXCLUDE_UNREACHABLE_BRANCHES ON + ) + setup_target_for_coverage_gcovr_xml( + NAME "coverage-xml-report" + GCOVR_ARGS --gcov-ignore-parse-errors=negative_hits.warn_once_per_file + EXCLUDE_THROW_BRANCHES ON + EXCLUDE_UNREACHABLE_BRANCHES ON + ) endif() if(BUILD_TESTS) diff --git a/cmake/common/CodeCoverage.cmake b/cmake/common/CodeCoverage.cmake index e5a71b596..0b2a4dccc 100644 --- a/cmake/common/CodeCoverage.cmake +++ b/cmake/common/CodeCoverage.cmake @@ -66,6 +66,20 @@ # - Make all add_custom_target()s VERBATIM to auto-escape wildcard characters # in EXCLUDEs, and remove manual escaping from gcovr targets # +# 2026-06-19, Bram Oosterhuis +# - Require CMake 3.15, drop all legacy version guards +# - Remove lcov/genhtml support, gcovr only +# - Remove Fortran support +# - Remove Clang version check +# - Remove include(CMakeParseArguments), built-in since CMake 3.5 +# - Replace CMAKE_COMPILER_IS_GNUCXX with modern CMAKE_CXX_COMPILER_ID check +# - Remove COVERAGE_LCOVR_EXCLUDES and COVERAGE_GCOVR_EXCLUDES legacy variables +# - Add GCOVR_ARGS parameter to both gcovr targets for passing extra gcovr flags +# - Add EXCLUDE_THROW_BRANCHES / EXCLUDE_UNREACHABLE_BRANCHES +# - Deduplicate the html/xml gcovr targets into one shared implementation +# - Pass the discovered GCOV_PATH to gcovr via --gcov-executable so the gcov used +# matches the compiler that wrote the .gcno files +# # USAGE: # # 1. Copy this file into your cmake modules path. @@ -84,352 +98,122 @@ # setup_target_for_coverage_*(). # Example: # set(COVERAGE_EXCLUDES -# '${PROJECT_SOURCE_DIR}/src/dir1/*' -# '/path/to/my/src/dir2/*') +# "${PROJECT_SOURCE_DIR}/src/dir1/*" +# "/path/to/my/src/dir2/*") # Or, use the EXCLUDE argument to setup_target_for_coverage_*(). # Example: -# setup_target_for_coverage_lcov( +# setup_target_for_coverage_gcovr_html( # NAME coverage -# EXECUTABLE testrunner # EXCLUDE "${PROJECT_SOURCE_DIR}/src/dir1/*" "/path/to/my/src/dir2/*") # -# 4.a NOTE: With CMake 3.4+, COVERAGE_EXCLUDES or EXCLUDE can also be set -# relative to the BASE_DIRECTORY (default: PROJECT_SOURCE_DIR) -# Example: -# set(COVERAGE_EXCLUDES "dir1/*") -# setup_target_for_coverage_gcovr_html( -# NAME coverage -# EXECUTABLE testrunner -# BASE_DIRECTORY "${PROJECT_SOURCE_DIR}/src" -# EXCLUDE "dir2/*") -# -# 5. Use the functions described below to create a custom make target which -# runs your test executable and produces a code coverage report. -# -# 6. Build a Debug build: -# cmake -DCMAKE_BUILD_TYPE=Debug .. -# make -# make my_coverage_target -# +# 5. Build a Debug build and run your tests, then: +# cmake --build . --target coverage-html-report -include(CMakeParseArguments) +cmake_minimum_required(VERSION 3.15) # Check prereqs -find_program( GCOV_PATH gcov ) -find_program( LCOV_PATH NAMES lcov lcov.bat lcov.exe lcov.perl) -find_program( GENHTML_PATH NAMES genhtml genhtml.perl genhtml.bat ) -find_program( GCOVR_PATH gcovr PATHS ${CMAKE_SOURCE_DIR}/scripts/test) -find_program( CPPFILT_PATH NAMES c++filt ) - -if(NOT GCOV_PATH) - message(FATAL_ERROR "gcov not found! Aborting...") -endif() # NOT GCOV_PATH - -if("${CMAKE_CXX_COMPILER_ID}" MATCHES "(Apple)?[Cc]lang") - if("${CMAKE_CXX_COMPILER_VERSION}" VERSION_LESS 3) - message(FATAL_ERROR "Clang version must be 3.0.0 or greater! Aborting...") - endif() -elseif(NOT CMAKE_COMPILER_IS_GNUCXX) - if("${CMAKE_Fortran_COMPILER_ID}" MATCHES "[Ff]lang") - # Do nothing; exit conditional without error if true - elseif("${CMAKE_Fortran_COMPILER_ID}" MATCHES "GNU") - # Do nothing; exit conditional without error if true - else() - message(FATAL_ERROR "Compiler is not GNU gcc! Aborting...") - endif() -endif() - -set(COVERAGE_COMPILER_FLAGS "-g -fprofile-arcs -ftest-coverage" - CACHE INTERNAL "") - -set(CMAKE_Fortran_FLAGS_COVERAGE - ${COVERAGE_COMPILER_FLAGS} - CACHE STRING "Flags used by the Fortran compiler during coverage builds." - FORCE ) -set(CMAKE_CXX_FLAGS_COVERAGE - ${COVERAGE_COMPILER_FLAGS} - CACHE STRING "Flags used by the C++ compiler during coverage builds." - FORCE ) -set(CMAKE_C_FLAGS_COVERAGE - ${COVERAGE_COMPILER_FLAGS} - CACHE STRING "Flags used by the C compiler during coverage builds." - FORCE ) -set(CMAKE_EXE_LINKER_FLAGS_COVERAGE - "" - CACHE STRING "Flags used for linking binaries during coverage builds." - FORCE ) -set(CMAKE_SHARED_LINKER_FLAGS_COVERAGE - "" - CACHE STRING "Flags used by the shared libraries linker during coverage builds." - FORCE ) -mark_as_advanced( - CMAKE_Fortran_FLAGS_COVERAGE - CMAKE_CXX_FLAGS_COVERAGE - CMAKE_C_FLAGS_COVERAGE - CMAKE_EXE_LINKER_FLAGS_COVERAGE - CMAKE_SHARED_LINKER_FLAGS_COVERAGE ) +find_program(GCOV_PATH gcov REQUIRED) +find_program(GCOVR_PATH gcovr PATHS ${CMAKE_SOURCE_DIR}/scripts/test REQUIRED) -if(NOT CMAKE_BUILD_TYPE STREQUAL "Debug") - message(WARNING "Code coverage results with an optimised (non-Debug) build may be misleading") -endif() # NOT CMAKE_BUILD_TYPE STREQUAL "Debug" - -if(CMAKE_C_COMPILER_ID STREQUAL "GNU" OR CMAKE_Fortran_COMPILER_ID STREQUAL "GNU") - link_libraries(gcov) +if(NOT CMAKE_CXX_COMPILER_ID STREQUAL "GNU" AND NOT CMAKE_CXX_COMPILER_ID MATCHES "Clang") + message(FATAL_ERROR "Code coverage requires GCC or Clang. Aborting...") endif() -# Defines a target for running and collection code coverage information -# Builds dependencies, runs the given executable and outputs reports. -# NOTE! The executable should always have a ZERO as exit code otherwise -# the coverage generation will not complete. -# -# setup_target_for_coverage_lcov( -# NAME testrunner_coverage # New target name -# EXECUTABLE testrunner -j ${PROCESSOR_COUNT} # Executable in PROJECT_BINARY_DIR -# DEPENDENCIES testrunner # Dependencies to build first -# BASE_DIRECTORY "../" # Base directory for report -# # (defaults to PROJECT_SOURCE_DIR) -# EXCLUDE "src/dir1/*" "src/dir2/*" # Patterns to exclude (can be relative -# # to BASE_DIRECTORY, with CMake 3.4+) -# NO_DEMANGLE # Don't demangle C++ symbols -# # even if c++filt is found -# ) -function(setup_target_for_coverage_lcov) - - set(options NO_DEMANGLE) - set(oneValueArgs BASE_DIRECTORY NAME) - set(multiValueArgs EXCLUDE EXECUTABLE EXECUTABLE_ARGS DEPENDENCIES LCOV_ARGS GENHTML_ARGS) - cmake_parse_arguments(Coverage "${options}" "${oneValueArgs}" "${multiValueArgs}" ${ARGN}) - - if(NOT LCOV_PATH) - message(FATAL_ERROR "lcov not found! Aborting...") - endif() # NOT LCOV_PATH +# Helper to collect and normalise exclude patterns into -e arguments for gcovr. +# Usage: _gcovr_collect_excludes( [exclude...]) +function(_gcovr_collect_excludes out_var base_dir) + set(excludes "") + foreach(exclude IN LISTS COVERAGE_EXCLUDES ARGN) + get_filename_component(exclude "${exclude}" ABSOLUTE BASE_DIR "${base_dir}") + list(APPEND excludes "${exclude}") + endforeach() + list(REMOVE_DUPLICATES excludes) - if(NOT GENHTML_PATH) - message(FATAL_ERROR "genhtml not found! Aborting...") - endif() # NOT GENHTML_PATH + set(exclude_args "") + foreach(exclude IN LISTS excludes) + list(APPEND exclude_args "-e" "${exclude}") + endforeach() - # Set base directory (as absolute path), or default to PROJECT_SOURCE_DIR - if(${Coverage_BASE_DIRECTORY}) - get_filename_component(BASEDIR ${Coverage_BASE_DIRECTORY} ABSOLUTE) + set(${out_var} "${exclude_args}" PARENT_SCOPE) +endfunction() + +# setup_target_for_coverage_gcovr_html / setup_target_for_coverage_gcovr_xml +# NAME # cmake target name +# EXCLUDE "src/dir1/*" ... # patterns to exclude (absolute or relative to BASE_DIRECTORY) +# GCOVR_ARGS --some-flag ... # extra flags forwarded verbatim to gcovr +# BASE_DIRECTORY # source root for gcovr -r (defaults to PROJECT_SOURCE_DIR) +# EXECUTABLE [args...] # test executable to run before collecting coverage +# DEPENDENCIES ... # cmake targets to build before running +# EXCLUDE_THROW_BRANCHES ON # drop exception-unwind branches from the report +# EXCLUDE_UNREACHABLE_BRANCHES ON # drop compiler-unreachable branches from the report +# +# Shared implementation behind both wrappers. is "html" or "xml"; +# everything except the output flags, the output path and the html +# make_directory step is identical between the two. +function(_setup_target_for_coverage_gcovr format) + set(one_value_args BASE_DIRECTORY NAME EXCLUDE_THROW_BRANCHES EXCLUDE_UNREACHABLE_BRANCHES) + set(multi_value_args DEPENDENCIES EXCLUDE EXECUTABLE EXECUTABLE_ARGS GCOVR_ARGS) + cmake_parse_arguments(PARSE_ARGV 1 Coverage "" "${one_value_args}" "${multi_value_args}") + + if(Coverage_BASE_DIRECTORY) + get_filename_component(basedir "${Coverage_BASE_DIRECTORY}" ABSOLUTE) else() - set(BASEDIR ${PROJECT_SOURCE_DIR}) + set(basedir "${PROJECT_SOURCE_DIR}") endif() - # Collect excludes (CMake 3.4+: Also compute absolute paths) - set(LCOV_EXCLUDES "") - foreach(EXCLUDE ${Coverage_EXCLUDE} ${COVERAGE_EXCLUDES} ${COVERAGE_LCOV_EXCLUDES}) - if(CMAKE_VERSION VERSION_GREATER 3.4) - get_filename_component(EXCLUDE ${EXCLUDE} ABSOLUTE BASE_DIR ${BASEDIR}) - endif() - list(APPEND LCOV_EXCLUDES "${EXCLUDE}") - endforeach() - list(REMOVE_DUPLICATES LCOV_EXCLUDES) + _gcovr_collect_excludes(exclude_args "${basedir}" ${Coverage_EXCLUDE}) - # Conditional arguments - if(CPPFILT_PATH AND NOT ${Coverage_NO_DEMANGLE}) - set(GENHTML_EXTRA_ARGS "--demangle-cpp") + set(extra_args "") + if(Coverage_EXCLUDE_THROW_BRANCHES) + list(APPEND extra_args --exclude-throw-branches) endif() - - # Setup target - add_custom_target(${Coverage_NAME} - - # Cleanup lcov - COMMAND ${LCOV_PATH} ${Coverage_LCOV_ARGS} --gcov-tool ${GCOV_PATH} -directory . -b ${BASEDIR} --zerocounters - # Create baseline to make sure untouched files show up in the report - COMMAND ${LCOV_PATH} ${Coverage_LCOV_ARGS} --gcov-tool ${GCOV_PATH} -c -i -d . -b ${BASEDIR} -o ${Coverage_NAME}.base - - # Run tests - COMMAND ${Coverage_EXECUTABLE} ${Coverage_EXECUTABLE_ARGS} - - # Capturing lcov counters and generating report - COMMAND ${LCOV_PATH} ${Coverage_LCOV_ARGS} --gcov-tool ${GCOV_PATH} --directory . -b ${BASEDIR} --capture --output-file ${Coverage_NAME}.capture - # add baseline counters - COMMAND ${LCOV_PATH} ${Coverage_LCOV_ARGS} --gcov-tool ${GCOV_PATH} -a ${Coverage_NAME}.base -a ${Coverage_NAME}.capture --output-file ${Coverage_NAME}.total - # filter collected data to final coverage report - COMMAND ${LCOV_PATH} ${Coverage_LCOV_ARGS} --gcov-tool ${GCOV_PATH} --remove ${Coverage_NAME}.total ${LCOV_EXCLUDES} --output-file ${Coverage_NAME}.info - - # Generate HTML output - COMMAND ${GENHTML_PATH} ${GENHTML_EXTRA_ARGS} ${Coverage_GENHTML_ARGS} -o ${Coverage_NAME} ${Coverage_NAME}.info - - # Set output files as GENERATED (will be removed on 'make clean') - BYPRODUCTS - ${Coverage_NAME}.base - ${Coverage_NAME}.capture - ${Coverage_NAME}.total - ${Coverage_NAME}.info - ${Coverage_NAME} # report directory - - WORKING_DIRECTORY ${PROJECT_BINARY_DIR} - DEPENDS ${Coverage_DEPENDENCIES} - VERBATIM # Protect arguments to commands - COMMENT "Resetting code coverage counters to zero.\nProcessing code coverage counters and generating report." - ) - - # Show where to find the lcov info report - add_custom_command(TARGET ${Coverage_NAME} POST_BUILD - COMMAND ; - COMMENT "Lcov code coverage info report saved in ${Coverage_NAME}.info." - ) - - # Show info where to find the report - add_custom_command(TARGET ${Coverage_NAME} POST_BUILD - COMMAND ; - COMMENT "Open ./${Coverage_NAME}/index.html in your browser to view the coverage report." - ) - -endfunction() # setup_target_for_coverage_lcov - -# Defines a target for running and collection code coverage information -# Builds dependencies, runs the given executable and outputs reports. -# NOTE! The executable should always have a ZERO as exit code otherwise -# the coverage generation will not complete. -# -# setup_target_for_coverage_gcovr_xml( -# NAME ctest_coverage # New target name -# EXECUTABLE ctest -j ${PROCESSOR_COUNT} # Executable in PROJECT_BINARY_DIR -# DEPENDENCIES executable_target # Dependencies to build first -# BASE_DIRECTORY "../" # Base directory for report -# # (defaults to PROJECT_SOURCE_DIR) -# EXCLUDE "src/dir1/*" "src/dir2/*" # Patterns to exclude (can be relative -# # to BASE_DIRECTORY, with CMake 3.4+) -# ) -function(setup_target_for_coverage_gcovr_xml) - - set(options NONE) - set(oneValueArgs BASE_DIRECTORY NAME) - set(multiValueArgs EXCLUDE EXECUTABLE EXECUTABLE_ARGS DEPENDENCIES) - cmake_parse_arguments(Coverage "${options}" "${oneValueArgs}" "${multiValueArgs}" ${ARGN}) - - if(NOT GCOVR_PATH) - message(FATAL_ERROR "gcovr not found! Aborting...") - endif() # NOT GCOVR_PATH - - # Set base directory (as absolute path), or default to PROJECT_SOURCE_DIR - if(${Coverage_BASE_DIRECTORY}) - get_filename_component(BASEDIR ${Coverage_BASE_DIRECTORY} ABSOLUTE) - else() - set(BASEDIR ${PROJECT_SOURCE_DIR}) + if(Coverage_EXCLUDE_UNREACHABLE_BRANCHES) + list(APPEND extra_args --exclude-unreachable-branches) endif() - # Collect excludes (CMake 3.4+: Also compute absolute paths) - set(GCOVR_EXCLUDES "") - foreach(EXCLUDE ${Coverage_EXCLUDE} ${COVERAGE_EXCLUDES} ${COVERAGE_GCOVR_EXCLUDES}) - if(CMAKE_VERSION VERSION_GREATER 3.4) - get_filename_component(EXCLUDE ${EXCLUDE} ABSOLUTE BASE_DIR ${BASEDIR}) - endif() - list(APPEND GCOVR_EXCLUDES "${EXCLUDE}") - endforeach() - list(REMOVE_DUPLICATES GCOVR_EXCLUDES) - - # Combine excludes to several -e arguments - set(GCOVR_EXCLUDE_ARGS "") - foreach(EXCLUDE ${GCOVR_EXCLUDES}) - list(APPEND GCOVR_EXCLUDE_ARGS "-e") - list(APPEND GCOVR_EXCLUDE_ARGS "${EXCLUDE}") - endforeach() - - add_custom_target(${Coverage_NAME} - # Run tests - ${Coverage_EXECUTABLE} ${Coverage_EXECUTABLE_ARGS} - - # Running gcovr - COMMAND ${GCOVR_PATH} --xml - -r ${BASEDIR} ${GCOVR_EXCLUDE_ARGS} - --object-directory=${PROJECT_BINARY_DIR} - -o ${Coverage_NAME}.xml - BYPRODUCTS ${Coverage_NAME}.xml - WORKING_DIRECTORY ${PROJECT_BINARY_DIR} - DEPENDS ${Coverage_DEPENDENCIES} - VERBATIM # Protect arguments to commands - COMMENT "Running gcovr to produce Cobertura code coverage report." - ) - - # Show info where to find the report - add_custom_command(TARGET ${Coverage_NAME} POST_BUILD - COMMAND ; - COMMENT "Cobertura code coverage report saved in ${Coverage_NAME}.xml." - ) -endfunction() # setup_target_for_coverage_gcovr_xml - -# Defines a target for running and collection code coverage information -# Builds dependencies, runs the given executable and outputs reports. -# NOTE! The executable should always have a ZERO as exit code otherwise -# the coverage generation will not complete. -# -# setup_target_for_coverage_gcovr_html( -# NAME ctest_coverage # New target name -# EXECUTABLE ctest -j ${PROCESSOR_COUNT} # Executable in PROJECT_BINARY_DIR -# DEPENDENCIES executable_target # Dependencies to build first -# BASE_DIRECTORY "../" # Base directory for report -# # (defaults to PROJECT_SOURCE_DIR) -# EXCLUDE "src/dir1/*" "src/dir2/*" # Patterns to exclude (can be relative -# # to BASE_DIRECTORY, with CMake 3.4+) -# ) -function(setup_target_for_coverage_gcovr_html) - - set(options NONE) - set(oneValueArgs BASE_DIRECTORY NAME) - set(multiValueArgs EXCLUDE EXECUTABLE EXECUTABLE_ARGS DEPENDENCIES) - cmake_parse_arguments(Coverage "${options}" "${oneValueArgs}" "${multiValueArgs}" ${ARGN}) - - if(NOT GCOVR_PATH) - message(FATAL_ERROR "gcovr not found! Aborting...") - endif() # NOT GCOVR_PATH - - # Set base directory (as absolute path), or default to PROJECT_SOURCE_DIR - if(${Coverage_BASE_DIRECTORY}) - get_filename_component(BASEDIR ${Coverage_BASE_DIRECTORY} ABSOLUTE) + set(pre_commands "") + if(format STREQUAL "html") + set(output "${PROJECT_BINARY_DIR}/${Coverage_NAME}") + set(format_args --html --html-details -o "${output}/index.html") + set(byproduct "${output}/index.html") + set(pre_commands COMMAND ${CMAKE_COMMAND} -E make_directory "${output}") + elseif(format STREQUAL "xml") + set(output "${PROJECT_BINARY_DIR}/${Coverage_NAME}.xml") + set(format_args --xml -o "${output}") + set(byproduct "${output}") else() - set(BASEDIR ${PROJECT_SOURCE_DIR}) + message(FATAL_ERROR "_setup_target_for_coverage_gcovr: unknown format '${format}'") endif() - # Collect excludes (CMake 3.4+: Also compute absolute paths) - set(GCOVR_EXCLUDES "") - foreach(EXCLUDE ${Coverage_EXCLUDE} ${COVERAGE_EXCLUDES} ${COVERAGE_GCOVR_EXCLUDES}) - if(CMAKE_VERSION VERSION_GREATER 3.4) - get_filename_component(EXCLUDE ${EXCLUDE} ABSOLUTE BASE_DIR ${BASEDIR}) - endif() - list(APPEND GCOVR_EXCLUDES "${EXCLUDE}") - endforeach() - list(REMOVE_DUPLICATES GCOVR_EXCLUDES) - - # Combine excludes to several -e arguments - set(GCOVR_EXCLUDE_ARGS "") - foreach(EXCLUDE ${GCOVR_EXCLUDES}) - list(APPEND GCOVR_EXCLUDE_ARGS "-e") - list(APPEND GCOVR_EXCLUDE_ARGS "${EXCLUDE}") - endforeach() - add_custom_target(${Coverage_NAME} - # Run tests - ${Coverage_EXECUTABLE} ${Coverage_EXECUTABLE_ARGS} - - # Create folder - COMMAND ${CMAKE_COMMAND} -E make_directory ${PROJECT_BINARY_DIR}/${Coverage_NAME} - - # Running gcovr - COMMAND ${GCOVR_PATH} --html --html-details - -r ${BASEDIR} ${GCOVR_EXCLUDE_ARGS} + COMMAND ${Coverage_EXECUTABLE} ${Coverage_EXECUTABLE_ARGS} + ${pre_commands} + COMMAND ${GCOVR_PATH} + --gcov-executable "${GCOV_PATH}" + -r "${basedir}" + ${exclude_args} + ${extra_args} + ${Coverage_GCOVR_ARGS} --object-directory=${PROJECT_BINARY_DIR} - -o ${Coverage_NAME}/index.html - - BYPRODUCTS ${PROJECT_BINARY_DIR}/${Coverage_NAME} # report directory - WORKING_DIRECTORY ${PROJECT_BINARY_DIR} + ${format_args} + BYPRODUCTS "${byproduct}" + WORKING_DIRECTORY "${PROJECT_BINARY_DIR}" DEPENDS ${Coverage_DEPENDENCIES} - VERBATIM # Protect arguments to commands - COMMENT "Running gcovr to produce HTML code coverage report." + VERBATIM + COMMENT "Running gcovr ${format} report -> ${output}" ) +endfunction() - # Show info where to find the report - add_custom_command(TARGET ${Coverage_NAME} POST_BUILD - COMMAND ; - COMMENT "Open ./${Coverage_NAME}/index.html in your browser to view the coverage report." - ) +function(setup_target_for_coverage_gcovr_html) + _setup_target_for_coverage_gcovr(html ${ARGN}) +endfunction() -endfunction() # setup_target_for_coverage_gcovr_html +function(setup_target_for_coverage_gcovr_xml) + _setup_target_for_coverage_gcovr(xml ${ARGN}) +endfunction() function(append_coverage_compiler_flags) - add_compile_options(${COVERAGE_COMPILER_FLAGS}) - add_link_options(${COVERAGE_COMPILER_FLAGS}) - message(STATUS "Appending code coverage compiler flags: ${COVERAGE_COMPILER_FLAGS}") -endfunction() # append_coverage_compiler_flags + add_compile_options(--coverage) + add_link_options(--coverage) + message(STATUS "Appending code coverage compiler flags") +endfunction()