From 2816ad4d8d503318f864f67e975c12491110b2ed Mon Sep 17 00:00:00 2001 From: Alexander Bock Date: Fri, 6 May 2022 00:32:25 -0700 Subject: [PATCH] Make the connection a shared_ptr and let the LuaScriptTopic return lambda keep a copy of it (closes #1940) --- modules/server/include/connection.h | 20 ++++++++----- .../server/include/topics/luascripttopic.h | 3 -- modules/server/include/topics/topic.h | 7 ++--- modules/server/servermodule.cpp | 2 +- modules/server/src/connection.cpp | 29 +++++++++---------- modules/server/src/topics/luascripttopic.cpp | 3 +- modules/server/src/topics/topic.cpp | 8 ++--- 7 files changed, 34 insertions(+), 38 deletions(-) diff --git a/modules/server/include/connection.h b/modules/server/include/connection.h index 344f3fb1aa..ec5257f420 100644 --- a/modules/server/include/connection.h +++ b/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 { public: - Connection( - std::unique_ptr s, - std::string address, - bool authorized = false, - const std::string& password = "" - ); + Connection(std::unique_ptr s, std::string address, + bool authorized = false, const std::string& password = ""); void handleMessage(const std::string& message); void sendMessage(const std::string& message); diff --git a/modules/server/include/topics/luascripttopic.h b/modules/server/include/topics/luascripttopic.h index 2a40372a4c..969fc1b68c 100644 --- a/modules/server/include/topics/luascripttopic.h +++ b/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; diff --git a/modules/server/include/topics/topic.h b/modules/server/include/topics/topic.h index da695e9ed9..067f97b952 100644 --- a/modules/server/include/topics/topic.h +++ b/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, 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; }; } // namespace openspace diff --git a/modules/server/servermodule.cpp b/modules/server/servermodule.cpp index 03789f5344..734c1ce246 100644 --- a/modules/server/servermodule.cpp +++ b/modules/server/servermodule.cpp @@ -216,7 +216,7 @@ void ServerModule::handleConnection(std::shared_ptr 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 }); } } diff --git a/modules/server/src/connection.cpp b/modules/server/src/connection.cpp index 311afe9ebd..2380e2153d 100644 --- a/modules/server/src/connection.cpp +++ b/modules/server/src/connection.cpp @@ -75,10 +75,8 @@ namespace { namespace openspace { -Connection::Connection(std::unique_ptr s, - std::string address, - bool authorized, - const std::string& password) +Connection::Connection(std::unique_ptr 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 = std::unique_ptr(_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()) { diff --git a/modules/server/src/topics/luascripttopic.cpp b/modules/server/src/topics/luascripttopic.cpp index 4f0b03529f..ec038f29dd 100644 --- a/modules/server/src/topics/luascripttopic.cpp +++ b/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; } }; diff --git a/modules/server/src/topics/topic.cpp b/modules/server/src/topics/topic.cpp index a4a9cb4594..cced85e1cc 100644 --- a/modules/server/src/topics/topic.cpp +++ b/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, size_t topicId) { + _connection = std::move(connection); _topicId = topicId; }