Fix race condition with concurrent teardown of ServiceCtx and re-connection of the preloaded Session Server.

serverManager/common/include/ISessionServerAppManager.h

@@ -52,6 +52,7 @@ class ISessionServerAppManager
     virtual bool setLogLevels(const service::LoggingLevels &logLevels) const = 0;
     virtual void restartServer(int serverId) = 0;
     virtual void onServerStartupTimeout(int serverId) = 0;
+    virtual void setShuttingDown() = 0;
 };
 } // namespace rialto::servermanager::common
 

serverManager/common/source/SessionServerAppManager.cpp

@@ -47,6 +47,11 @@ SessionServerAppManager::~SessionServerAppManager()
     m_eventThread.reset();
 }
 
+void SessionServerAppManager::setShuttingDown()
+{
+    m_isShuttingDown = true;
+}
+
 void SessionServerAppManager::preloadSessionServers(unsigned numOfPreloadedServers)
 {
     m_eventThread->add(
@@ -392,7 +397,7 @@ void SessionServerAppManager::handleAck(int serverId, int pingId, bool success)
 
 void SessionServerAppManager::shutdownAllSessionServers()
 {
-    m_isShuttingDown = true;
+    setShuttingDown();
     m_healthcheckService.reset();
     for (const auto &kSessionServer : m_sessionServerApps)
     {

serverManager/common/source/SessionServerAppManager.h

@@ -29,6 +29,7 @@
 #include "ISessionServerAppManager.h"
 #include "IStateObserver.h"
 #include "SessionServerAppFactory.h"
+#include <atomic>
 #include <memory>
 #include <set>
 #include <string>
@@ -62,6 +63,7 @@ class SessionServerAppManager : public ISessionServerAppManager
     bool setLogLevels(const service::LoggingLevels &logLevels) const override;
     void restartServer(int serverId) override;
     void onServerStartupTimeout(int serverId) override;
+    void setShuttingDown() override;
 
 private:
     bool connectSessionServer(const std::shared_ptr<ISessionServerApp> &sessionServer);
@@ -99,7 +101,7 @@ class SessionServerAppManager : public ISessionServerAppManager
     std::shared_ptr<service::IStateObserver> m_stateObserver;
     std::unique_ptr<IHealthcheckService> m_healthcheckService;
     const firebolt::rialto::ipc::INamedSocketFactory &m_namedSocketFactory;
-    bool m_isShuttingDown;
+    std::atomic_bool m_isShuttingDown;
 };
 } // namespace rialto::servermanager::common
 

serverManager/service/include/ServiceContext.h

@@ -24,6 +24,7 @@
 #include "IServiceContext.h"
 #include "ISessionServerAppManager.h"
 #include "IStateObserver.h"
+#include <chrono>
 #include <list>
 #include <memory>
 #include <string>
@@ -38,7 +39,7 @@ class ServiceContext : public IServiceContext
                    std::chrono::milliseconds sessionServerStartupTimeout, std::chrono::seconds healthcheckInterval,
                    unsigned numOfFailedPingsBeforeRecovery, unsigned int socketPermissions,
                    const std::string &socketOwner, const std::string &socketGroup);
-    virtual ~ServiceContext() = default;
+    ~ServiceContext() override;
 
     common::ISessionServerAppManager &getSessionServerAppManager() override;
 

serverManager/service/source/ServiceContext.cpp

@@ -38,6 +38,14 @@ ServiceContext::ServiceContext(const std::shared_ptr<IStateObserver> &stateObser
 {
 }
 
+ServiceContext::~ServiceContext()
+{
+    if (m_sessionServerAppManager)
+    {
+        m_sessionServerAppManager->setShuttingDown();
+    }
+}
+
 common::ISessionServerAppManager &ServiceContext::getSessionServerAppManager()
 {
     return *m_sessionServerAppManager;

tests/unittests/serverManager/mocks/SessionServerAppManagerMock.h

@@ -47,6 +47,7 @@ class SessionServerAppManagerMock : public ISessionServerAppManager
     MOCK_METHOD(bool, setLogLevels, (const service::LoggingLevels &), (const, override));
     MOCK_METHOD(void, restartServer, (int serverId), (override));
     MOCK_METHOD(void, onServerStartupTimeout, (int serverId), (override));
+    MOCK_METHOD(void, setShuttingDown, (), (override));
 };
 } // namespace rialto::servermanager::common
 

tests/unittests/serverManager/unittests/common/SessionServerAppManagerTests.cpp

@@ -319,6 +319,15 @@ TEST_F(SessionServerAppManagerTests, SessionServerShouldSkipRestart)
     sessionServerWillKillRunningApplication();
 }
 
+TEST_F(SessionServerAppManagerTests, SessionServerShouldSkipRestartWhenShuttingDown)
+{
+    sessionServerWillLaunch(firebolt::rialto::common::SessionServerState::INACTIVE);
+    ASSERT_TRUE(triggerInitiateApplication(firebolt::rialto::common::SessionServerState::INACTIVE));
+    triggerSetShuttingDown();
+    triggerRestartServer();
+    sessionServerWillKillRunningApplication();
+}
+
 TEST_F(SessionServerAppManagerTests, SessionServerShouldReportStartupTimeout)
 {
     sessionServerWillLaunch(firebolt::rialto::common::SessionServerState::INACTIVE);

tests/unittests/serverManager/unittests/common/SessionServerAppManagerTestsFixture.cpp

@@ -539,3 +539,8 @@ void SessionServerAppManagerTests::triggerOnServerStartupTimeout()
 {
     m_sut->onServerStartupTimeout(kServerId);
 }
+
+void SessionServerAppManagerTests::triggerSetShuttingDown()
+{
+    m_sut->setShuttingDown();
+}

tests/unittests/serverManager/unittests/common/SessionServerAppManagerTestsFixture.h

@@ -88,6 +88,7 @@ class SessionServerAppManagerTests : public testing::Test
     void triggerSendPingEvents();
     void triggerRestartServer();
     void triggerOnServerStartupTimeout();
+    void triggerSetShuttingDown();
 
 private:
     std::unique_ptr<rialto::servermanager::ipc::IController> m_controller;

tests/unittests/serverManager/unittests/service/ServerManagerServiceTests.cpp

@@ -19,6 +19,7 @@
 
 #include "RialtoLogging.h"
 #include "ServerManagerServiceTestsFixture.h"
+#include "ServiceContext.h"
 #include "gtest/gtest.h"
 
 namespace
@@ -27,6 +28,12 @@ const std::string kAppName{"YouTube"};
 const firebolt::rialto::common::SessionServerState kAppState{firebolt::rialto::common::SessionServerState::INACTIVE};
 const std::string kAppSocket{getenv("RIALTO_SOCKET_PATH")};
 const firebolt::rialto::common::AppConfig kAppConfig{kAppSocket};
+
+void createAndDestroyServiceContext()
+{
+    rialto::servermanager::service::ServiceContext
+        context{nullptr, {}, "", std::chrono::milliseconds{0}, std::chrono::seconds{0}, 0, 0, "", ""};
+}
 } // namespace
 
 TEST_F(ServerManagerServiceTests, initiateApplicationShouldReturnTrueIfOperationSucceeded)
@@ -82,3 +89,8 @@ TEST_F(ServerManagerServiceTests, registerLogHandlerShouldFailWhenPtrIsNull)
 {
     EXPECT_FALSE(triggerRegisterLogHandler(nullptr));
 }
+
+TEST(ServiceContextTests, DestructorShouldNotThrow)
+{
+    ASSERT_NO_THROW(createAndDestroyServiceContext());
+}