Make the connection a shared_ptr and let the LuaScriptTopic return lambda keep a copy of it (closes #1940)

modules/server/include/connection.h

@@ -39,14 +39,20 @@ using TopicId = size_t;
 
 class Topic;
 
-class Connection {
+// @TODO (abock, 2022-05-06) This is not really elegant as there is no need for a
+// Connection to be held by a shared_ptr, but there was a problem with the LuaScriptTopic
+// otherwise (issue #1940).
+// The problem there is that the LuaScriptTopic is keeping a copy of the _connection in
+// its lambda to return a script back to the caller. The script is only queued, so is
+// executed a bit longer. If the UI gets reloaded in between the creation of the lambda
+// and the execution, the _connection will be an invalid pointer and the program will
+// crash. Making this a shared_ptr circumvents that problem my having the lamdba retain
+// ownership of the _connection and keeping it alive until the message is sent. The
+// message doesn't go anywhere since noone is listening, but it's better than a crash.
+class Connection : public std::enable_shared_from_this<Connection> {
 public:
-    Connection(
-        std::unique_ptr<ghoul::io::Socket> s,
-        std::string address,
-        bool authorized = false,
-        const std::string& password = ""
-    );
+    Connection(std::unique_ptr<ghoul::io::Socket> s, std::string address,
+        bool authorized = false, const std::string& password = "");
 
     void handleMessage(const std::string& message);
     void sendMessage(const std::string& message);

modules/server/include/topics/luascripttopic.h

@@ -31,9 +31,6 @@ namespace openspace {
 
 class LuaScriptTopic : public Topic {
 public:
-    LuaScriptTopic() = default;
-    virtual ~LuaScriptTopic() = default;
-
     void handleJson(const nlohmann::json& json) override;
     bool isDone() const override;
 

modules/server/include/topics/topic.h

@@ -33,10 +33,9 @@ class Connection;
 
 class Topic {
 public:
-    Topic() {}
-    virtual ~Topic();
+    virtual ~Topic() = default;
 
-    void initialize(Connection* connection, size_t topicId);
+    void initialize(std::shared_ptr<Connection> connection, size_t topicId);
     nlohmann::json wrappedPayload(const nlohmann::json& payload) const;
     nlohmann::json wrappedError(std::string message = "Could not complete request.",
         int code = 500);
@@ -45,7 +44,7 @@ public:
 
 protected:
     size_t _topicId = 0;
-    Connection* _connection;
+    std::shared_ptr<Connection> _connection;
 };
 
 } // namespace openspace

modules/server/servermodule.cpp

@@ -216,7 +216,7 @@ void ServerModule::handleConnection(std::shared_ptr<Connection> connection) {
     messageString.reserve(256);
     while (connection->socket()->getMessage(messageString)) {
         std::lock_guard lock(_messageQueueMutex);
-        _messageQueue.push_back({ connection, std::move(messageString) });
+        _messageQueue.push_back({ connection, messageString });
     }
 }
 

modules/server/src/connection.cpp

@@ -75,10 +75,8 @@ namespace {
 
 namespace openspace {
 
-Connection::Connection(std::unique_ptr<ghoul::io::Socket> s,
-                       std::string address,
-                       bool authorized,
-                       const std::string& password)
+Connection::Connection(std::unique_ptr<ghoul::io::Socket> s, std::string address,
+                       bool authorized, const std::string& password)
     : _socket(std::move(s))
     , _address(std::move(address))
     , _isAuthorized(authorized)
@@ -121,14 +119,16 @@ void Connection::handleMessage(const std::string& message) {
         nlohmann::json j = nlohmann::json::parse(message.c_str());
         try {
             handleJson(j);
-        } catch (const std::domain_error& e) {
+        }
+        catch (const std::domain_error& e) {
             LERROR(fmt::format("JSON handling error from: {}. {}", message, e.what()));
         }
-        } catch (const std::out_of_range& e) {
-            LERROR(fmt::format("JSON handling error from: {}. {}", message, e.what()));
-        }
-        catch (const std::exception& e) {
-            LERROR(e.what());
+    }
+    catch (const std::out_of_range& e) {
+        LERROR(fmt::format("JSON handling error from: {}. {}", message, e.what()));
+    }
+    catch (const std::exception& e) {
+        LERROR(e.what());
     } catch (...) {
         if (!isAuthorized()) {
             _socket->disconnect();
@@ -177,10 +177,7 @@ void Connection::handleJson(const nlohmann::json& json) {
         // The topic id is not registered: Initialize a new topic.
         auto typeJson = json.find(MessageKeyType);
         if (typeJson == json.end() || !typeJson->is_string()) {
-            LERROR(fmt::format(
-                "A type must be specified (`{}`) as a string when "
-                "a new topic is initialized", MessageKeyType
-            ));
+            LERROR("Type must be specified as a string when a new topic is initialized");
             return;
         }
         std::string type = *typeJson;
@@ -191,7 +188,7 @@ void Connection::handleJson(const nlohmann::json& json) {
         }
 
         std::unique_ptr<Topic> topic = std::unique_ptr<Topic>(_topicFactory.create(type));
-        topic->initialize(this, topicId);
+        topic->initialize(shared_from_this(), topicId);
         topic->handleJson(*payloadJson);
         if (!topic->isDone()) {
             _topics.emplace(topicId, std::move(topic));
@@ -203,7 +200,7 @@ void Connection::handleJson(const nlohmann::json& json) {
             return;
         }
 
-        // Dispatch the message to the existing topic.
+        // Dispatch the message to the existing topic
         Topic& topic = *topicIt->second;
         topic.handleJson(*payloadJson);
         if (topic.isDone()) {

modules/server/src/topics/luascripttopic.cpp

@@ -182,7 +182,8 @@ void LuaScriptTopic::runScript(std::string script, bool shouldReturn) {
         callback = [this](ghoul::Dictionary data) {
             if (_connection) {
                 nlohmann::json j = data;
-                _connection->sendJson(wrappedPayload(j));
+                nlohmann::json payload = wrappedPayload(j);
+                _connection->sendJson(payload);
                 _waitingForReturnValue = false;
             }
         };

modules/server/src/topics/topic.cpp

@@ -30,12 +30,8 @@
 
 namespace openspace {
 
-Topic::~Topic() {
-    _connection = nullptr;
-}
-
-void Topic::initialize(Connection* connection, size_t topicId) {
-    _connection = connection;
+void Topic::initialize(std::shared_ptr<Connection> connection, size_t topicId) {
+    _connection = std::move(connection);
     _topicId = topicId;
 }