Move server refresh list storage to class user

This fixes possible duplicated servers when refreshing too quickly

src/network/protocols/connect_to_server.cpp

@@ -182,19 +182,15 @@ void ConnectToServer::asynchronousUpdate()
         {
             if (!m_server)
             {
-                while (!ServersManager::get()->refresh(false))
+                auto server_list =
+                    ServersManager::get()->getWANRefreshRequest();
+                while (!server_list->m_list_updated)
                 {
                     if (ProtocolManager::lock()->isExiting())
                         return;
                     StkTime::sleep(1);
                 }
-                while (!ServersManager::get()->listUpdated())
+                auto& servers = server_list->m_servers;
-                {
-                    if (ProtocolManager::lock()->isExiting())
-                        return;
-                    StkTime::sleep(1);
-                }
-                auto servers = std::move(ServersManager::get()->getServers());
 
                 // Remove password protected servers
                 servers.erase(std::remove_if(servers.begin(), servers.end(), []

src/network/servers_manager.cpp

@@ -48,8 +48,6 @@
 #  include <net/if.h>
 #endif
 
-const int64_t SERVER_REFRESH_INTERVAL = 5000;
-
 static ServersManager* g_manager_singleton(NULL);
 
 // ============================================================================
@@ -71,7 +69,6 @@ void ServersManager::deallocate()
 // ----------------------------------------------------------------------------
 ServersManager::ServersManager()
 {
-    reset();
 }   // ServersManager
 
 // ----------------------------------------------------------------------------
@@ -83,7 +80,7 @@ ServersManager::~ServersManager()
 /** Returns a WAN update-list-of-servers request. It queries the
  *  STK server for an up-to-date list of servers.
  */
-std::shared_ptr<Online::XMLRequest> ServersManager::getWANRefreshRequest() const
+std::shared_ptr<ServerList> ServersManager::getWANRefreshRequest() const
 {
     // ========================================================================
     /** A small local class that triggers an update of the ServersManager
@@ -91,14 +88,17 @@ std::shared_ptr<Online::XMLRequest> ServersManager::getWANRefreshRequest() const
     class WANRefreshRequest : public Online::XMLRequest
     {
     private:
+        std::weak_ptr<ServerList> m_server_list;
         // Run the ip detect in separate thread, so it can be done parallel
         // with the wan server request (which takes few seconds too)
         std::thread m_ip_detect_thread;
     public:
-        WANRefreshRequest() : Online::XMLRequest(/*priority*/100)
+        WANRefreshRequest(std::shared_ptr<ServerList> server_list)
+        : Online::XMLRequest(/*priority*/100)
         {
             m_ip_detect_thread = std::thread(std::bind(
                 &NetworkConfig::detectIPType, NetworkConfig::get()));
+            m_server_list = server_list;
         }
         ~WANRefreshRequest()
         {
@@ -111,16 +111,55 @@ std::shared_ptr<Online::XMLRequest> ServersManager::getWANRefreshRequest() const
             Online::XMLRequest::afterOperation();
             if (m_ip_detect_thread.joinable())
                 m_ip_detect_thread.join();
-            ServersManager::get()->setWanServers(isSuccess(), getXMLData());
+
+            auto server_list = m_server_list.lock();
+            if (!server_list)
+                return;
+
+            if (!isSuccess())
+            {
+                Log::error("ServersManager", "Could not refresh server list");
+                server_list->m_list_updated = true;
+                return;
+            }
+
+            const XMLNode *servers_xml = getXMLData()->getNode("servers");
+            for (unsigned int i = 0; i < servers_xml->getNumNodes(); i++)
+            {
+                const XMLNode* s = servers_xml->getNode(i);
+                assert(s);
+                const XMLNode* si = s->getNode("server-info");
+                assert(si);
+                int version = 0;
+                si->get("version", &version);
+                assert(version != 0);
+                if (version < stk_config->m_max_server_version ||
+                    version > stk_config->m_max_server_version)
+                {
+                    Log::verbose("ServersManager", "Skipping a server");
+                    continue;
+                }
+                std::shared_ptr<Server> ser = std::make_shared<Server>(*s);
+                if (ser->getAddress().isUnset() &&
+                    NetworkConfig::get()->getIPType() == NetworkConfig::IP_V4)
+                {
+                    Log::verbose("ServersManager",
+                        "Skipping an IPv6 only server");
+                    continue;
+                }
+                server_list->m_servers.emplace_back(ser);
+            }
+            server_list->m_list_updated = true;
         }   // afterOperation
         // --------------------------------------------------------------------
     };   // RefreshRequest
     // ========================================================================
 
-    auto request = std::make_shared<WANRefreshRequest>();
+    auto server_list = std::make_shared<ServerList>();
+    auto request = std::make_shared<WANRefreshRequest>(server_list);
     request->setApiURL(Online::API::SERVER_PATH, "get-all");
-
+    Online::RequestManager::get()->addRequest(request);
-    return request;
+    return server_list;
 }   // getWANRefreshRequest
 
 // ----------------------------------------------------------------------------
@@ -128,7 +167,7 @@ std::shared_ptr<Online::XMLRequest> ServersManager::getWANRefreshRequest() const
  *  to find LAN servers, and waits for a certain amount of time fr 
  *  answers.
  */
-std::shared_ptr<Online::XMLRequest> ServersManager::getLANRefreshRequest() const
+std::shared_ptr<ServerList> ServersManager::getLANRefreshRequest() const
 {
     /** A simple class that uses LAN broadcasts to find local servers.
      *  It is based on XML request, but actually does not use any of the
@@ -137,10 +176,17 @@ std::shared_ptr<Online::XMLRequest> ServersManager::getLANRefreshRequest() const
      */
     class LANRefreshRequest : public Online::XMLRequest
     {
+    private:
+        std::weak_ptr<ServerList> m_server_list;
     public:
 
         /** High priority for this request. */
-        LANRefreshRequest() : XMLRequest(/*priority*/100) {m_success = false;}
+        LANRefreshRequest(std::shared_ptr<ServerList> server_list)
+            : XMLRequest(/*priority*/100)
+        {
+            m_success = false;
+            m_server_list = server_list;
+        }
         // --------------------------------------------------------------------
         virtual ~LANRefreshRequest() {}
         // --------------------------------------------------------------------
@@ -159,6 +205,10 @@ std::shared_ptr<Online::XMLRequest> ServersManager::getLANRefreshRequest() const
         // --------------------------------------------------------------------
         virtual void operation() OVERRIDE
         {
+            auto server_list = m_server_list.lock();
+            if (!server_list)
+                return;
+
             ENetAddress addr = {};
             setIPv6Socket(UserConfigParams::m_ipv6_lan ? 1 : 0);
             NetworkConfig::get()->setIPType(UserConfigParams::m_ipv6_lan ?
@@ -169,6 +219,7 @@ std::shared_ptr<Online::XMLRequest> ServersManager::getLANRefreshRequest() const
                 setIPv6Socket(0);
                 m_success = true;
                 delete broadcast;
+                server_list->m_list_updated = true;
                 return;
             }
             const std::vector<SocketAddress> &all_bcast =
@@ -189,9 +240,7 @@ std::shared_ptr<Online::XMLRequest> ServersManager::getLANRefreshRequest() const
             // any local servers.
             uint64_t start_time = StkTime::getMonoTimeMs();
             const uint64_t DURATION = 1000;
-            const auto& servers = ServersManager::get()->getServers();
+            int cur_server_id = 0;
-            int cur_server_id = (int)servers.size();
-            assert(cur_server_id == 0);
             // Use a map with the server name as key to automatically remove
             // duplicated answers from a server (since we potentially do
             // multiple broadcasts). We can not use the sender ip address,
@@ -246,9 +295,11 @@ std::shared_ptr<Online::XMLRequest> ServersManager::getLANRefreshRequest() const
                 }   // if received_data
             }    // while still waiting
             setIPv6Socket(0);
-            m_success = true;
-            ServersManager::get()->setLanServers(servers_now);
             delete broadcast;
+            m_success = true;
+            for (auto& i : servers_now)
+                server_list->m_servers.emplace_back(i.second);
+            server_list->m_list_updated = true;
         }   // operation
         // --------------------------------------------------------------------
         /** This function is necessary, otherwise the XML- and HTTP-Request
@@ -258,95 +309,13 @@ std::shared_ptr<Online::XMLRequest> ServersManager::getLANRefreshRequest() const
     };   // LANRefreshRequest
     // ========================================================================
 
-    return std::make_shared<LANRefreshRequest>();
+    auto server_list = std::make_shared<ServerList>();
+    auto request = std::make_shared<LANRefreshRequest>(server_list);
+    Online::RequestManager::get()->addRequest(request);
+    return server_list;
 
 }   // getLANRefreshRequest
 
-// ----------------------------------------------------------------------------
-/** Takes a mapping of server name to server data (to avoid having the same 
- *  server listed more than once since the client will be doing multiple
- *  broadcasts to find a server), and converts this into a list of servers.
- *  \param servers Mapping of server name to Server object.
- */
-void ServersManager::setLanServers(const std::map<irr::core::stringw,
-                                                  std::shared_ptr<Server> >& servers)
-{
-    m_servers.clear();
-    for (auto i : servers) m_servers.emplace_back(i.second);
-    m_last_load_time.store(StkTime::getMonoTimeMs());
-    m_list_updated = true;
-
-}
-// ----------------------------------------------------------------------------
-/** Factory function to create either a LAN or a WAN update-of-server
- *  requests. The current list of servers is also cleared.
- */
-bool ServersManager::refresh(bool full_refresh)
-{
-    if ((int64_t)StkTime::getMonoTimeMs() - m_last_load_time.load()
-        < SERVER_REFRESH_INTERVAL)
-    {
-        // Avoid too frequent refreshing
-        return false;
-    }
-
-    cleanUpServers();
-    m_list_updated = false;
-
-    if (NetworkConfig::get()->isWAN())
-    {
-        Online::RequestManager::get()->addRequest(getWANRefreshRequest());
-    }
-    else
-    {
-        Online::RequestManager::get()->addRequest(getLANRefreshRequest());
-    }
-    return true;
-}   // refresh
-
-// ----------------------------------------------------------------------------
-/** Callback from the refresh request for wan servers.
- *  \param success If the refresh was successful.
- *  \param input The XML data describing the server.
- */
-void ServersManager::setWanServers(bool success, const XMLNode* input)
-{
-    if (!success)
-    {
-        Log::error("Server Manager", "Could not refresh server list");
-        m_list_updated = true;
-        return;
-    }
-
-    const XMLNode *servers_xml = input->getNode("servers");
-    for (unsigned int i = 0; i < servers_xml->getNumNodes(); i++)
-    {
-        const XMLNode* s = servers_xml->getNode(i);
-        assert(s);
-        const XMLNode* si = s->getNode("server-info");
-        assert(si);
-        int version = 0;
-        si->get("version", &version);
-        assert(version != 0);
-        if (version < stk_config->m_max_server_version ||
-            version > stk_config->m_max_server_version)
-        {
-            Log::verbose("ServersManager", "Skipping a server");
-            continue;
-        }
-        std::shared_ptr<Server> ser = std::make_shared<Server>(*s);
-        if (ser->getAddress().isUnset() &&
-            NetworkConfig::get()->getIPType() == NetworkConfig::IP_V4)
-        {
-            Log::verbose("ServersManager", "Skipping an IPv6 only server");
-            continue;
-        }
-        m_servers.emplace_back(ser);
-    }
-    m_last_load_time.store(StkTime::getMonoTimeMs());
-    m_list_updated = true;
-}   // refresh
-
 // ----------------------------------------------------------------------------
 /** Sets a list of default broadcast addresses which is used in case no valid
  *  broadcast address is found. This list includes default private network

src/network/servers_manager.hpp

@@ -36,18 +36,20 @@ class XMLNode;
  * \brief
  * \ingroup online
  */
+struct ServerList
+{
+    /** List of servers */
+    std::vector<std::shared_ptr<Server> > m_servers;
+    std::atomic_bool m_list_updated;
+    ServerList() { m_list_updated.store(false); }
+};
+
 class ServersManager
 {
 private:
-    /** List of servers */
-    std::vector<std::shared_ptr<Server> > m_servers;
-    
     /** List of broadcast addresses to use. */
     std::vector<SocketAddress> m_broadcast_address;
 
-    std::atomic<int64_t> m_last_load_time;
-
-    std::atomic_bool m_list_updated;
     // ------------------------------------------------------------------------
      ServersManager();
     // ------------------------------------------------------------------------
@@ -55,10 +57,6 @@ private:
     // ------------------------------------------------------------------------
     void setWanServers(bool success, const XMLNode* input);
     // ------------------------------------------------------------------------
-    std::shared_ptr<Online::XMLRequest> getWANRefreshRequest() const;
-    // ------------------------------------------------------------------------
-    std::shared_ptr<Online::XMLRequest> getLANRefreshRequest() const;
-    // ------------------------------------------------------------------------
     void setLanServers(const std::map<irr::core::stringw, 
                                       std::shared_ptr<Server> >& servers);
                                       
@@ -72,20 +70,11 @@ public:
     // ------------------------------------------------------------------------
     static void deallocate();
     // ------------------------------------------------------------------------
-    void cleanUpServers()                                { m_servers.clear(); }
-    // ------------------------------------------------------------------------
-    bool refresh(bool full_refresh);
-    // ------------------------------------------------------------------------
-    std::vector<std::shared_ptr<Server> >& getServers()   { return m_servers; }
-    // ------------------------------------------------------------------------
-    bool listUpdated() const                         { return m_list_updated; }
-    // ------------------------------------------------------------------------
     std::vector<SocketAddress> getBroadcastAddresses(bool ipv6);
     // ------------------------------------------------------------------------
-    void reset()
+    std::shared_ptr<ServerList> getWANRefreshRequest() const;
-    {
+    // ------------------------------------------------------------------------
-        m_last_load_time.store(-5000);
+    std::shared_ptr<ServerList> getLANRefreshRequest() const;
-        m_list_updated = false;
+
-    }
 };   // class ServersManager
 #endif // HEADER_SERVERS_MANAGER_HPP

src/states_screens/online/server_selection.cpp

@@ -67,26 +67,30 @@ ServerSelection::~ServerSelection()
 void ServerSelection::tearDown()
 {
     m_servers.clear();
-    ServersManager::get()->cleanUpServers();
     m_server_list_widget->clear();
+    m_server_list = nullptr;
 }   // tearDown
 
 // ----------------------------------------------------------------------------
 /** Requests the servers manager to update its list of servers, and disables
  *  the 'refresh' button (till the refresh was finished).
  */
-void ServerSelection::refresh(bool full_refresh)
+void ServerSelection::refresh()
 {
     // If the request was created (i.e. no error, and not re-requested within
     // 5 seconds), clear the list and display the waiting message:
-    if (ServersManager::get()->refresh(full_refresh))
+    if ((int64_t)StkTime::getMonoTimeMs() - m_last_load_time < 5000)
-    {
+        return;
+
     m_ip_warning_shown = false;
     m_server_list_widget->clear();
     m_reload_widget->setActive(false);
     m_refreshing_server = true;
     m_refresh_timer = 0.0f;
-    }
+    m_last_load_time = StkTime::getMonoTimeMs();
+    m_server_list = NetworkConfig::get()->isWAN() ?
+        ServersManager::get()->getWANRefreshRequest() :
+        ServersManager::get()->getLANRefreshRequest();
 }   // refresh
 
 // ----------------------------------------------------------------------------
@@ -137,6 +141,7 @@ void ServerSelection::beforeAddingWidget()
 void ServerSelection::init()
 {
     Screen::init();
+    m_last_load_time = -5000;
 
 #ifndef ENABLE_IPV6
     m_ipv6->setState(false);
@@ -190,9 +195,7 @@ void ServerSelection::init()
     
     m_server_list_widget->setIcons(m_icon_bank, row_height);
     m_sort_desc = false;
-    /** Triggers the loading of the server list in the servers manager. */
+    refresh();
-    ServersManager::get()->reset();
-    refresh(true);
     m_ipv6_only_without_nat64 = false;
     m_ip_warning_shown = false;
 }   // init
@@ -318,7 +321,7 @@ void ServerSelection::eventCallback(GUIEngine::Widget* widget,
     }
     else if (name == "reload")
     {
-        refresh(true);
+        refresh();
     }
     else if (name == "private_server" || name == "ipv6")
     {
@@ -334,7 +337,7 @@ void ServerSelection::eventCallback(GUIEngine::Widget* widget,
             m_ip_warning_shown = true;
             MessageQueue::add(MessageQueue::MT_ERROR, v4);
         }
-        copyFromServersManager();
+        copyFromServerList();
     }
     else if (name == m_server_list_widget->m_properties[GUIEngine::PROP_ID])
     {
@@ -372,14 +375,14 @@ void ServerSelection::onUpdate(float dt)
         sid->requestJoin();
     }
     
-    if (ServersManager::get()->getServers().empty() && !m_refreshing_server &&
+    if (m_server_list->m_list_updated && !m_refreshing_server &&
         !NetworkConfig::get()->isWAN())
     {
         m_refresh_timer += dt;
         
         if (m_refresh_timer > 10.0f)
         {
-            refresh(false);
+            refresh();
         }
     }
 
@@ -393,15 +396,15 @@ void ServerSelection::onUpdate(float dt)
 
     if (!m_refreshing_server) return;
 
-    if (ServersManager::get()->listUpdated())
+    if (m_server_list->m_list_updated)
     {
         m_refreshing_server = false;
-        if (!ServersManager::get()->getServers().empty())
+        if (!m_server_list->m_servers.empty())
         {
             int selection = m_server_list_widget->getSelectionID();
             std::string selection_str = m_server_list_widget
                 ->getSelectionInternalName();
-            copyFromServersManager();
+            copyFromServerList();
             // restore previous selection
             if (selection != -1 && selection_str != "loading")
                 m_server_list_widget->setSelectionID(selection);
@@ -425,9 +428,9 @@ void ServerSelection::onUpdate(float dt)
 }   // onUpdate
 
 // ----------------------------------------------------------------------------
-void ServerSelection::copyFromServersManager()
+void ServerSelection::copyFromServerList()
 {
-    m_servers = ServersManager::get()->getServers();
+    m_servers = m_server_list->m_servers;
     if (m_servers.empty())
         return;
     m_servers.erase(std::remove_if(m_servers.begin(), m_servers.end(),
@@ -456,7 +459,7 @@ void ServerSelection::copyFromServersManager()
             }), m_servers.end());
     }
     loadList();
-}   // copyFromServersManager
+}   // copyFromServerList
 
 // ----------------------------------------------------------------------------
 void ServerSelection::unloaded()

src/states_screens/online/server_selection.hpp

@@ -42,6 +42,7 @@ namespace irr
 }
 
 class Server;
+struct ServerList;
 
 /**
   * \brief ServerSelection
@@ -80,12 +81,14 @@ private:
     /** Load the servers into the main list.*/
     void loadList();
 
-    void copyFromServersManager();
+    void copyFromServerList();
 
-    void refresh(bool full_refresh);
+    void refresh();
 
     bool m_ipv6_only_without_nat64;
     bool m_ip_warning_shown;
+    int64_t m_last_load_time;
+    std::shared_ptr<ServerList> m_server_list;
 public:
     /** \brief implement callback from parent class GUIEngine::Screen */
     virtual void loadedFromFile() OVERRIDE;
@@ -110,7 +113,7 @@ public:
     /** \brief implement callback from parent class GUIEngine::Screen */
     virtual void onUpdate(float dt) OVERRIDE;
 
-    virtual void onTextUpdated() OVERRIDE         { copyFromServersManager(); }
+    virtual void onTextUpdated() OVERRIDE             { copyFromServerList(); }
 
     virtual bool onEnterPressed(const irr::core::stringw& text) OVERRIDE
                                                               { return false; }