SPE-2622: Moved session mutex inside Account Session methods. Prevents performing request under locked mutex.

Prevents deadlock when UI interacts with Session queue while requests are performing slowly.

src/slic3r/GUI/UserAccountCommunication.cpp

@@ -242,8 +242,6 @@ void UserAccountCommunication::set_username(const std::string& username)
 {
     m_username = username;
     {
-        // We don't need mutex lock here, as credentials are guarded by own mutex in m_session
-        //std::lock_guard<std::mutex> lock(m_session_mutex);
         if (is_secret_store_ok()) {
             std::string tokens;
             if (m_remember_session) {
@@ -292,36 +290,28 @@ void UserAccountCommunication::set_remember_session(bool b)
 
 std::string UserAccountCommunication::get_access_token()
 {
-    {
+
-        // We don't need mutex lock here, as credentials are guarded by own mutex in m_session
+    return m_session->get_access_token();
-        //std::lock_guard<std::mutex> lock(m_session_mutex);
+
-        return m_session->get_access_token();
-    }
 }
 
 std::string UserAccountCommunication::get_shared_session_key()
 {
-    {
+
-        // We don't need mutex lock here, as credentials are guarded by own mutex in m_session
+    return m_session->get_shared_session_key();
-        //std::lock_guard<std::mutex> lock(m_session_mutex);
+
-        return m_session->get_shared_session_key();
-    }
 }
 
 void UserAccountCommunication::set_polling_enabled(bool enabled)
 {
-    {
+    return m_session->set_polling_action(enabled ? UserAccountActionID::USER_ACCOUNT_ACTION_CONNECT_PRINTER_MODELS : UserAccountActionID::USER_ACCOUNT_ACTION_DUMMY);
-        std::lock_guard<std::mutex> lock(m_session_mutex);
-        return m_session->set_polling_action(enabled ? UserAccountActionID::USER_ACCOUNT_ACTION_CONNECT_PRINTER_MODELS : UserAccountActionID::USER_ACCOUNT_ACTION_DUMMY);
-    }
 }
 
 void UserAccountCommunication::on_uuid_map_success()
 {
-    {
+
-        std::lock_guard<std::mutex> lock(m_session_mutex);
+    return m_session->set_polling_action(UserAccountActionID::USER_ACCOUNT_ACTION_CONNECT_STATUS);
-        return m_session->set_polling_action(UserAccountActionID::USER_ACCOUNT_ACTION_CONNECT_STATUS);
+    
-    }
 }
 
 // Generates and stores Code Verifier - second call deletes previous one.
@@ -371,13 +361,10 @@ bool UserAccountCommunication::is_logged()
 }
 void UserAccountCommunication::do_login()
 {
-    {
+    if (!m_session->is_initialized()) {
-        std::lock_guard<std::mutex> lock(m_session_mutex);
+        login_redirect();
-        if (!m_session->is_initialized()) {
+    } else { 
-            login_redirect();
+        m_session->enqueue_test_with_refresh();
-        } else { 
-            m_session->enqueue_test_with_refresh();
-        }
     }
     wakeup_session_thread();
 }
@@ -389,85 +376,64 @@ void UserAccountCommunication::do_logout()
 
 void UserAccountCommunication::do_clear()
 {
-    {
+    m_session->clear();
-        std::lock_guard<std::mutex> lock(m_session_mutex);
-        m_session->clear();
-    }
     set_username({});
     m_token_timer->Stop();
 }
 
 void UserAccountCommunication::on_login_code_recieved(const std::string& url_message)
 {
-    {
+    const std::string code = get_code_from_message(url_message);
-        std::lock_guard<std::mutex> lock(m_session_mutex);
+    m_session->init_with_code(code, m_code_verifier);
-        const std::string code = get_code_from_message(url_message);
-        m_session->init_with_code(code, m_code_verifier);
-    }
     wakeup_session_thread();
 }
 
 void UserAccountCommunication::enqueue_connect_printer_models_action()
 {
-    {
+    if (!m_session->is_initialized()) {
-        std::lock_guard<std::mutex> lock(m_session_mutex);
+        BOOST_LOG_TRIVIAL(error) << "Connect Printer Models connection failed - Not Logged in.";
-        if (!m_session->is_initialized()) {
+        return;
-            BOOST_LOG_TRIVIAL(error) << "Connect Printer Models connection failed - Not Logged in.";
-            return;
-        }
-        m_session->enqueue_action(UserAccountActionID::USER_ACCOUNT_ACTION_CONNECT_PRINTER_MODELS, nullptr, nullptr, {});
     }
+    m_session->enqueue_action(UserAccountActionID::USER_ACCOUNT_ACTION_CONNECT_PRINTER_MODELS, nullptr, nullptr, {});
     wakeup_session_thread();
 }
 
 void UserAccountCommunication::enqueue_connect_status_action()
 {
-    {
+    if (!m_session->is_initialized()) {
-        std::lock_guard<std::mutex> lock(m_session_mutex);
+        BOOST_LOG_TRIVIAL(error) << "Connect Status endpoint connection failed - Not Logged in.";
-        if (!m_session->is_initialized()) {
+        return;
-            BOOST_LOG_TRIVIAL(error) << "Connect Status endpoint connection failed - Not Logged in.";
-            return;
-        }
-        m_session->enqueue_action(UserAccountActionID::USER_ACCOUNT_ACTION_CONNECT_STATUS, nullptr, nullptr, {});
     }
+    m_session->enqueue_action(UserAccountActionID::USER_ACCOUNT_ACTION_CONNECT_STATUS, nullptr, nullptr, {});
     wakeup_session_thread();
 }
 void UserAccountCommunication::enqueue_test_connection()
 {
-    {
+    if (!m_session->is_initialized()) {
-        std::lock_guard<std::mutex> lock(m_session_mutex);
+        BOOST_LOG_TRIVIAL(error) << "Connect Printers endpoint connection failed - Not Logged in.";
-        if (!m_session->is_initialized()) {
+        return;
-            BOOST_LOG_TRIVIAL(error) << "Connect Printers endpoint connection failed - Not Logged in.";
-            return;
-        }
-        m_session->enqueue_test_with_refresh();
     }
+    m_session->enqueue_test_with_refresh();
     wakeup_session_thread();
 }
 
 void UserAccountCommunication::enqueue_avatar_action(const std::string& url)
 {
-    {
+    if (!m_session->is_initialized()) {
-        std::lock_guard<std::mutex> lock(m_session_mutex);
+        BOOST_LOG_TRIVIAL(error) << "Connect Printers endpoint connection failed - Not Logged in.";
-        if (!m_session->is_initialized()) {
+        return;
-            BOOST_LOG_TRIVIAL(error) << "Connect Printers endpoint connection failed - Not Logged in.";
-            return;
-        }
-        m_session->enqueue_action(UserAccountActionID::USER_ACCOUNT_ACTION_AVATAR, nullptr, nullptr, url);
     }
+    m_session->enqueue_action(UserAccountActionID::USER_ACCOUNT_ACTION_AVATAR, nullptr, nullptr, url);
     wakeup_session_thread();
 }
 
 void UserAccountCommunication::enqueue_printer_data_action(const std::string& uuid)
 {
-    {
+    if (!m_session->is_initialized()) {
-        std::lock_guard<std::mutex> lock(m_session_mutex);
+        BOOST_LOG_TRIVIAL(error) << "Connect Printers endpoint connection failed - Not Logged in.";
-        if (!m_session->is_initialized()) {
+        return;
-            BOOST_LOG_TRIVIAL(error) << "Connect Printers endpoint connection failed - Not Logged in.";
-            return;
-        }
-        m_session->enqueue_action(UserAccountActionID::USER_ACCOUNT_ACTION_CONNECT_DATA_FROM_UUID, nullptr, nullptr, uuid);
     }
+    m_session->enqueue_action(UserAccountActionID::USER_ACCOUNT_ACTION_CONNECT_DATA_FROM_UUID, nullptr, nullptr, uuid);   
     wakeup_session_thread();
 }
    
@@ -479,18 +445,15 @@ void UserAccountCommunication::request_refresh()
 
 void UserAccountCommunication::enqueue_refresh()
 {
-    {
+    if (!m_session->is_initialized()) {
-        std::lock_guard<std::mutex> lock(m_session_mutex);
+        BOOST_LOG_TRIVIAL(error) << "Connect Printers endpoint connection failed - Not Logged in.";
-        if (!m_session->is_initialized()) {
+        return;
-            BOOST_LOG_TRIVIAL(error) << "Connect Printers endpoint connection failed - Not Logged in.";
-            return;
-        }
-        if (m_session->is_enqueued(UserAccountActionID::USER_ACCOUNT_ACTION_REFRESH_TOKEN)) {
-            BOOST_LOG_TRIVIAL(debug) << "User Account: Token refresh already enqueued, skipping...";
-            return;
-        }
-        m_session->enqueue_refresh({});
     }
+    if (m_session->is_enqueued(UserAccountActionID::USER_ACCOUNT_ACTION_REFRESH_TOKEN)) {
+        BOOST_LOG_TRIVIAL(debug) << "User Account: Token refresh already enqueued, skipping...";
+        return;
+    }
+    m_session->enqueue_refresh({});
     wakeup_session_thread();
 }
 
@@ -512,10 +475,7 @@ void UserAccountCommunication::init_session_thread()
                 continue;
             }
             m_thread_wakeup = false;
-            {
+            m_session->process_action_queue();
-                std::lock_guard<std::mutex> lock(m_session_mutex);
-                m_session->process_action_queue();
-            }
         }
     });
 }

src/slic3r/GUI/UserAccountCommunication.hpp

@@ -81,7 +81,6 @@ public:
 private:
     std::unique_ptr<UserAccountSession>     m_session;
     std::thread                             m_thread;
-    std::mutex                              m_session_mutex;
     std::mutex                              m_thread_stop_mutex;
     std::condition_variable                 m_thread_stop_condition;
     bool                                    m_thread_stop { false };

src/slic3r/GUI/UserAccountSession.cpp

@@ -35,6 +35,7 @@ wxDEFINE_EVENT(EVT_UA_ENQUEUED_REFRESH, SimpleEvent);
 void UserActionPost::perform(/*UNUSED*/ wxEvtHandler* evt_handler, /*UNUSED*/ const std::string& access_token, UserActionSuccessFn success_callback, UserActionFailFn fail_callback, const std::string& input) const
 {
     std::string url = m_url;
+    BOOST_LOG_TRIVIAL(debug) << __FUNCTION__ << " " << url; 
     auto http = Http::post(std::move(url));
     if (!input.empty())
         http.set_post_body(input);
@@ -53,6 +54,7 @@ void UserActionPost::perform(/*UNUSED*/ wxEvtHandler* evt_handler, /*UNUSED*/ co
 void UserActionGetWithEvent::perform(wxEvtHandler* evt_handler, const std::string& access_token, UserActionSuccessFn success_callback, UserActionFailFn fail_callback, const std::string& input) const
 {
     std::string url = m_url + input;
+    BOOST_LOG_TRIVIAL(debug) << __FUNCTION__ << " " << url; 
     auto http = Http::get(std::move(url));
     if (!access_token.empty()) {
         http.header("Authorization", "Bearer " + access_token);
@@ -82,75 +84,98 @@ void UserActionGetWithEvent::perform(wxEvtHandler* evt_handler, const std::strin
     http.perform_sync(HttpRetryOpt::default_retry());
 }
 
-bool UserAccountSession::is_enqueued(UserAccountActionID action_id) const {
+bool UserAccountSession::is_enqueued(UserAccountActionID action_id) const 
-    return std::any_of(
+{
-        std::begin(m_priority_action_queue), std::end(m_priority_action_queue),
+    {
-        [action_id](const ActionQueueData& item) { return item.action_id == action_id; }
+        std::lock_guard<std::mutex> lock(m_session_mutex);
-    );
+        return std::any_of(
+            std::begin(m_priority_action_queue), std::end(m_priority_action_queue),
+            [action_id](const ActionQueueData& item) { return item.action_id == action_id; }
+        );
+    }
 }
 
-
 void UserAccountSession::process_action_queue()
 {
-    if (!m_proccessing_enabled)
+    {
-        return;
+        std::lock_guard<std::mutex> lock(m_session_mutex);
-    if (m_priority_action_queue.empty() && m_action_queue.empty()) {
+        if (!m_proccessing_enabled)
-        // update printers periodically
+            return;
-        enqueue_action(m_polling_action, nullptr, nullptr, {});
+        if (m_priority_action_queue.empty() && m_action_queue.empty()) {
-    }
+            // update printers periodically
-    // priority queue works even when tokens are empty or broken
+            enqueue_action_inner(m_polling_action, nullptr, nullptr, {});
-    while (!m_priority_action_queue.empty()) {
-        std::string access_token;
-        {
-            std::lock_guard<std::mutex> lock(m_credentials_mutex);
-            access_token = m_access_token;
         }
-        m_actions[m_priority_action_queue.front().action_id]->perform(p_evt_handler, access_token, m_priority_action_queue.front().success_callback, m_priority_action_queue.front().fail_callback, m_priority_action_queue.front().input);
+    }
-        if (!m_priority_action_queue.empty())
+    process_action_queue_inner();
+}
+void UserAccountSession::process_action_queue_inner()
+{
+    bool call_priority = false;
+    bool call_standard = false;
+    ActionQueueData selected_data;
+    {
+        std::lock_guard<std::mutex> lock(m_session_mutex);
+    
+        // priority queue works even when tokens are empty or broken
+        if (!m_priority_action_queue.empty()) {
+            // Do a copy here even its costly. We need to get data outside m_session_mutex protected code to perform background operation over it.
+            selected_data = m_priority_action_queue.front();
             m_priority_action_queue.pop_front();
-    }
+            call_priority = true;
-    // regular queue has to wait until priority fills tokens
+        } else if (this->is_initialized() && !m_action_queue.empty()) {
-    if (!this->is_initialized())
+            // regular queue has to wait until priority fills tokens
-        return;
+            // Do a copy here even its costly. We need to get data outside m_session_mutex protected code to perform background operation over it.
-    while (!m_action_queue.empty()) {
+            selected_data = m_action_queue.front();
-        std::string access_token;
-        {
-            std::lock_guard<std::mutex> lock(m_credentials_mutex);
-            access_token = m_access_token;
-        }
-        m_actions[m_action_queue.front().action_id]->perform(p_evt_handler, access_token, m_action_queue.front().success_callback, m_action_queue.front().fail_callback, m_action_queue.front().input);
-        if (!m_action_queue.empty())
             m_action_queue.pop();
+            call_standard = true;
+        }
     }
+    if (call_priority || call_standard) {
+        m_actions[selected_data.action_id]->perform(p_evt_handler, get_access_token(), selected_data.success_callback, selected_data.fail_callback, selected_data.input);
+        process_action_queue_inner();
+    }        
 }
 
 void UserAccountSession::enqueue_action(UserAccountActionID id, UserActionSuccessFn success_callback, UserActionFailFn fail_callback, const std::string& input)
 {
-    m_proccessing_enabled = true;
+    {
-    m_action_queue.push({ id, success_callback, fail_callback, input });
+        std::lock_guard<std::mutex> lock(m_session_mutex);
+        enqueue_action_inner(id, success_callback, fail_callback, input);
+    }
 }
 
+// called from m_session_mutex protected code only!
+void UserAccountSession::enqueue_action_inner(UserAccountActionID id, UserActionSuccessFn success_callback, UserActionFailFn fail_callback, const std::string& input)
+{
+        m_proccessing_enabled = true;
+        m_action_queue.push({ id, success_callback, fail_callback, input });
+}
 
 void UserAccountSession::init_with_code(const std::string& code, const std::string& code_verifier)
 {
-    // Data we have       
+    {
-    const std::string REDIRECT_URI = "prusaslicer://login";
+        std::lock_guard<std::mutex> lock(m_session_mutex);
-    std::string post_fields = "code=" + code +
+        // Data we have       
-        "&client_id=" + client_id() +
+        const std::string REDIRECT_URI = "prusaslicer://login";
-        "&grant_type=authorization_code" +
+        std::string post_fields = "code=" + code +
-        "&redirect_uri=" + REDIRECT_URI +
+            "&client_id=" + client_id() +
-        "&code_verifier="+ code_verifier;
+            "&grant_type=authorization_code" +
+            "&redirect_uri=" + REDIRECT_URI +
+            "&code_verifier="+ code_verifier;
 
-    m_proccessing_enabled = true;
+        m_proccessing_enabled = true;
-    // fail fn might be cancel_queue here
+        // fail fn might be cancel_queue here
-    m_priority_action_queue.push_back({ UserAccountActionID::USER_ACCOUNT_ACTION_CODE_FOR_TOKEN
+        m_priority_action_queue.push_back({ UserAccountActionID::USER_ACCOUNT_ACTION_CODE_FOR_TOKEN
-        , std::bind(&UserAccountSession::token_success_callback, this, std::placeholders::_1)
+            , std::bind(&UserAccountSession::token_success_callback, this, std::placeholders::_1)
-        , std::bind(&UserAccountSession::code_exchange_fail_callback, this, std::placeholders::_1)
+            , std::bind(&UserAccountSession::code_exchange_fail_callback, this, std::placeholders::_1)
-        , post_fields });
+            , post_fields });
+    }
 }
 
 void UserAccountSession::token_success_callback(const std::string& body)
 {
+    // No need to use lock m_session_mutex here
+
     BOOST_LOG_TRIVIAL(debug) << "Access token refreshed";
     // Data we need
     std::string access_token, refresh_token, shared_session_key;
@@ -204,21 +229,24 @@ void UserAccountSession::token_success_callback(const std::string& body)
 
 void UserAccountSession::code_exchange_fail_callback(const std::string& body)
 {
+    
     BOOST_LOG_TRIVIAL(debug) << "Access token refresh failed, body: " << body;
     clear();
     cancel_queue();
     // Unlike refresh_fail_callback, no event was triggered so far, do it. (USER_ACCOUNT_ACTION_CODE_FOR_TOKEN does not send events)
-    wxQueueEvent(p_evt_handler, new UserAccountFailEvent(EVT_UA_RESET, std::move(body)));
+    wxQueueEvent(p_evt_handler, new UserAccountFailEvent(EVT_UA_RESET, body));
 }
 
 void UserAccountSession::enqueue_test_with_refresh()
 {
-    // on test fail - try refresh
+    {
-    m_proccessing_enabled = true;
+        std::lock_guard<std::mutex> lock(m_session_mutex);
-    m_priority_action_queue.push_back({ UserAccountActionID::USER_ACCOUNT_ACTION_TEST_ACCESS_TOKEN, nullptr, std::bind(&UserAccountSession::enqueue_refresh, this, std::placeholders::_1), {} });
+        // on test fail - try refresh
+        m_proccessing_enabled = true;
+        m_priority_action_queue.push_back({ UserAccountActionID::USER_ACCOUNT_ACTION_TEST_ACCESS_TOKEN, nullptr, std::bind(&UserAccountSession::enqueue_refresh, this, std::placeholders::_1), {} });
+    }
 }
 
-
 void UserAccountSession::enqueue_refresh(const std::string& body)
 {
     wxQueueEvent(p_evt_handler, new SimpleEvent(EVT_UA_ENQUEUED_REFRESH));
@@ -230,11 +258,13 @@ void UserAccountSession::enqueue_refresh(const std::string& body)
                       "&client_id=" + client_id() +
                       "&refresh_token=" + m_refresh_token;
     }
-
+    {
-    m_priority_action_queue.push_back({ UserAccountActionID::USER_ACCOUNT_ACTION_REFRESH_TOKEN
+        std::lock_guard<std::mutex> lock(m_session_mutex);
-        , std::bind(&UserAccountSession::token_success_callback, this, std::placeholders::_1)
+        m_priority_action_queue.push_back({ UserAccountActionID::USER_ACCOUNT_ACTION_REFRESH_TOKEN
-        , std::bind(&UserAccountSession::refresh_fail_callback, this, std::placeholders::_1)
+            , std::bind(&UserAccountSession::token_success_callback, this, std::placeholders::_1)
-        , post_fields });
+            , std::bind(&UserAccountSession::refresh_fail_callback, this, std::placeholders::_1)
+            , post_fields });
+    }
 }
 
 void UserAccountSession::refresh_fail_callback(const std::string& body)
@@ -244,15 +274,17 @@ void UserAccountSession::refresh_fail_callback(const std::string& body)
     // No need to notify UI thread here
     // backtrace: load tokens -> TEST_TOKEN fail (access token bad) -> REFRESH_TOKEN fail (refresh token bad)
     // USER_ACCOUNT_ACTION_TEST_ACCESS_TOKEN triggers EVT_UA_FAIL, we need also RESET
-    wxQueueEvent(p_evt_handler, new UserAccountFailEvent(EVT_UA_RESET, std::move(body)));
+    wxQueueEvent(p_evt_handler, new UserAccountFailEvent(EVT_UA_RESET, body));
-
 }
 
 void UserAccountSession::cancel_queue()
 {
-    m_priority_action_queue.clear();
+    {
-    while (!m_action_queue.empty()) {
+        std::lock_guard<std::mutex> lock(m_session_mutex);
-        m_action_queue.pop();
+        m_priority_action_queue.clear();
+        while (!m_action_queue.empty()) {
+            m_action_queue.pop();
+        }
     }
 }
 

src/slic3r/GUI/UserAccountSession.hpp

@@ -143,7 +143,10 @@ public:
             m_refresh_token.clear();
             m_shared_session_key.clear();
         }
-        m_proccessing_enabled = false;
+        {
+            std::lock_guard<std::mutex> lock(m_session_mutex);
+            m_proccessing_enabled = false;
+        }
     }
 
     // Functions that automatically enable action queu processing
@@ -152,8 +155,8 @@ public:
     // Special enques, that sets callbacks.
     void enqueue_test_with_refresh();
     void enqueue_refresh(const std::string& body);
-
     void process_action_queue();
+
     bool is_initialized() const {
         std::lock_guard<std::mutex> lock(m_credentials_mutex);
         return !m_access_token.empty() || !m_refresh_token.empty();
@@ -177,21 +180,22 @@ public:
     }
 
     //void set_polling_enabled(bool enabled) {m_polling_action = enabled ? UserAccountActionID::USER_ACCOUNT_ACTION_CONNECT_PRINTER_MODELS : UserAccountActionID::USER_ACCOUNT_ACTION_DUMMY; }
-    void set_polling_action(UserAccountActionID action) { m_polling_action = action; }
+    void set_polling_action(UserAccountActionID action) { 
+        std::lock_guard<std::mutex> lock(m_session_mutex);
+        m_polling_action = action; 
+    }
 private:
-
     void refresh_fail_callback(const std::string& body);
     void cancel_queue();
     void code_exchange_fail_callback(const std::string& body);
     void token_success_callback(const std::string& body);
     std::string client_id() const { return Utils::ServiceConfig::instance().account_client_id(); }
+    void process_action_queue_inner();
 
-    // false prevents action queu to be processed - no communication is done
+    // called from m_session_mutex protected code only
-    // sets to true by init_with_code or enqueue_action call
+    void enqueue_action_inner(UserAccountActionID id, UserActionSuccessFn success_callback, UserActionFailFn fail_callback, const std::string& input);
-    bool        m_proccessing_enabled {false}; 
+
-    // action when woken up on idle - switches between USER_ACCOUNT_ACTION_CONNECT_PRINTER_MODELS and USER_ACCOUNT_ACTION_CONNECT_STATUS
+   
-    // set to USER_ACCOUNT_ACTION_DUMMY to switch off polling
-    UserAccountActionID m_polling_action;
 
     // Section of following vars is guarded by this mutex
     mutable std::mutex m_credentials_mutex;
@@ -201,8 +205,21 @@ private:
     long long m_next_token_timeout;
     // End of section guarded by m_credentials_mutex
 
+
+     // Section of following vars is guarded by this mutex
+    mutable std::mutex m_session_mutex;
+
     std::queue<ActionQueueData>                                    m_action_queue;
-    std::deque<ActionQueueData>                                    m_priority_action_queue;
+    std::deque<ActionQueueData>                                    m_priority_action_queue; 
+     // false prevents action queue to be processed - no communication is done
+    // sets to true by init_with_code or enqueue_action call
+    bool        m_proccessing_enabled {false}; 
+    // action when woken up on idle - switches between USER_ACCOUNT_ACTION_CONNECT_PRINTER_MODELS and USER_ACCOUNT_ACTION_CONNECT_STATUS
+    // set to USER_ACCOUNT_ACTION_DUMMY to switch off polling
+    UserAccountActionID m_polling_action;
+
+     // End of section guarded by m_session_mutex
+    
     std::map<UserAccountActionID, std::unique_ptr<UserAction>>     m_actions;
 
     wxEvtHandler* p_evt_handler;