Merge pull request #759 from michael-loki/fix_metrics_thread_safety

Fix metrics thread safety

include/tuntap.h

@@ -147,7 +147,7 @@ extern "C"
     int flags; /* ifr.ifr_flags on Unix */
     char if_name[IF_NAMESIZE];
 #if defined(Windows)
-	int idx; /* needed to set ipv6 address */
+    int idx; /* needed to set ipv6 address */
 #endif
 #if defined(FreeBSD)
     int mode;

llarp/ev/ev_libuv.cpp

@@ -633,16 +633,17 @@ namespace libuv
   Loop::CloseAll()
   {
     llarp::LogInfo("Closing all handles");
-    uv_walk(m_Impl.get(),
-            [](uv_handle_t* h, void*) {
-              if(uv_is_closing(h))
-                return;
-              if(h->data && uv_is_active(h))
-              {
-                static_cast< glue* >(h->data)->Close();
-              }
-            },
-            nullptr);
+    uv_walk(
+        m_Impl.get(),
+        [](uv_handle_t* h, void*) {
+          if(uv_is_closing(h))
+            return;
+          if(h->data && uv_is_active(h))
+          {
+            static_cast< glue* >(h->data)->Close();
+          }
+        },
+        nullptr);
   }
 
   void

llarp/path/path.cpp

@@ -103,8 +103,8 @@ namespace llarp
     }
 
     bool
-    Path::HandleLRSM(uint64_t status,
-                     std::array< EncryptedFrame, 8 >& frames, AbstractRouter* r)
+    Path::HandleLRSM(uint64_t status, std::array< EncryptedFrame, 8 >& frames,
+                     AbstractRouter* r)
     {
       uint64_t currentStatus = status;
 

llarp/path/path_context.cpp

@@ -164,14 +164,15 @@ namespace llarp
     HopHandler_ptr
     PathContext::GetByUpstream(const RouterID& remote, const PathID_t& id)
     {
-      auto own = MapGet(m_OurPaths, id,
-                        [](const PathSet_ptr) -> bool {
-                          // TODO: is this right?
-                          return true;
-                        },
-                        [remote, id](PathSet_ptr p) -> HopHandler_ptr {
-                          return p->GetByUpstream(remote, id);
-                        });
+      auto own = MapGet(
+          m_OurPaths, id,
+          [](const PathSet_ptr) -> bool {
+            // TODO: is this right?
+            return true;
+          },
+          [remote, id](PathSet_ptr p) -> HopHandler_ptr {
+            return p->GetByUpstream(remote, id);
+          });
       if(own)
         return own;
 

llarp/path/path_context.hpp

@@ -107,6 +107,7 @@ namespace llarp
 
         void
         ForEach(std::function< void(const TransitHop_ptr&) > visit)
+            LOCKS_EXCLUDED(first)
         {
           util::Lock lock(&first);
           for(const auto& item : second)

llarp/router/outbound_message_handler.hpp

@@ -26,7 +26,7 @@ namespace llarp
 
     bool
     QueueMessage(const RouterID &remote, const ILinkMessage *msg,
-                 SendStatusHandler callback) override;
+                 SendStatusHandler callback) override LOCKS_EXCLUDED(_mutex);
 
     util::StatusObject
     ExtractStatus() const override;
@@ -72,7 +72,8 @@ namespace llarp
     SendIfSession(const RouterID &remote, const Message &msg);
 
     void
-    FinalizeRequest(const RouterID &router, SendStatus status);
+    FinalizeRequest(const RouterID &router, SendStatus status)
+        LOCKS_EXCLUDED(_mutex);
 
     mutable util::Mutex _mutex;  // protects outboundMessageQueue
 

llarp/router/outbound_session_maker.hpp

@@ -35,13 +35,16 @@ namespace llarp
     OnConnectTimeout(ILinkSession *session) override;
 
     void
-    CreateSessionTo(const RouterID &router, RouterCallback on_result) override;
+    CreateSessionTo(const RouterID &router, RouterCallback on_result) override
+        LOCKS_EXCLUDED(_mutex);
 
     void
-    CreateSessionTo(const RouterContact &rc, RouterCallback on_result) override;
+    CreateSessionTo(const RouterContact &rc, RouterCallback on_result) override
+        LOCKS_EXCLUDED(_mutex);
 
     bool
-    HavePendingSessionTo(const RouterID &router) const override;
+    HavePendingSessionTo(const RouterID &router) const override
+        LOCKS_EXCLUDED(_mutex);
 
     void
     ConnectToRandomRouters(int numDesired, llarp_time_t now) override;
@@ -56,10 +59,11 @@ namespace llarp
 
    private:
     void
-    DoEstablish(const RouterID &router);
+    DoEstablish(const RouterID &router) LOCKS_EXCLUDED(_mutex);
 
     void
-    GotRouterContact(const RouterID &router, const RouterContact &rc);
+    GotRouterContact(const RouterID &router, const RouterContact &rc)
+        LOCKS_EXCLUDED(_mutex);
 
     void
     InvalidRouter(const RouterID &router);
@@ -75,10 +79,11 @@ namespace llarp
     VerifyRC(const RouterContact rc);
 
     void
-    CreatePendingSession(const RouterID &router);
+    CreatePendingSession(const RouterID &router) LOCKS_EXCLUDED(_mutex);
 
     void
-    FinalizeRequest(const RouterID &router, const SessionResult type);
+    FinalizeRequest(const RouterID &router, const SessionResult type)
+        LOCKS_EXCLUDED(_mutex);
 
     mutable util::Mutex _mutex;  // protects pendingSessions, pendingCallbacks
 

llarp/router/rc_lookup_handler.hpp

@@ -31,25 +31,29 @@ namespace llarp
     ~RCLookupHandler() = default;
 
     void
-    AddValidRouter(const RouterID &router) override;
+    AddValidRouter(const RouterID &router) override LOCKS_EXCLUDED(_mutex);
 
     void
-    RemoveValidRouter(const RouterID &router) override;
+    RemoveValidRouter(const RouterID &router) override LOCKS_EXCLUDED(_mutex);
 
     void
-    SetRouterWhitelist(const std::vector< RouterID > &routers) override;
+    SetRouterWhitelist(const std::vector< RouterID > &routers) override
+        LOCKS_EXCLUDED(_mutex);
 
     void
-    GetRC(const RouterID &router, RCRequestCallback callback) override;
+    GetRC(const RouterID &router, RCRequestCallback callback) override
+        LOCKS_EXCLUDED(_mutex);
 
     bool
-    RemoteIsAllowed(const RouterID &remote) const override;
+    RemoteIsAllowed(const RouterID &remote) const override
+        LOCKS_EXCLUDED(_mutex);
 
     bool
     CheckRC(const RouterContact &rc) const override;
 
     bool
-    GetRandomWhitelistRouter(RouterID &router) const override;
+    GetRandomWhitelistRouter(RouterID &router) const override
+        LOCKS_EXCLUDED(_mutex);
 
     bool
     CheckRenegotiateValid(RouterContact newrc, RouterContact oldrc) override;
@@ -74,14 +78,14 @@ namespace llarp
                           const std::vector< RouterContact > &results);
 
     bool
-    HavePendingLookup(RouterID remote) const;
+    HavePendingLookup(RouterID remote) const LOCKS_EXCLUDED(_mutex);
 
     bool
     RemoteInBootstrap(const RouterID &remote) const;
 
     void
     FinalizeRequest(const RouterID &router, const RouterContact *const rc,
-                    RCRequestResult result);
+                    RCRequestResult result) LOCKS_EXCLUDED(_mutex);
 
     mutable util::Mutex _mutex;  // protects pendingCallbacks, whitelistRouters
 

llarp/service/endpoint_state.hpp

@@ -40,7 +40,7 @@ namespace llarp
       hooks::Backend_ptr m_OnDown;
       hooks::Backend_ptr m_OnReady;
 
-      util::Mutex m_InboundTrafficQueueMutex;
+      util::Mutex m_InboundTrafficQueueMutex;  // protects m_InboundTrafficQueue
       /// ordered queue for inbound hidden service traffic
       RecvPacketQueue_t m_InboundTrafficQueue
           GUARDED_BY(m_InboundTrafficQueueMutex);
@@ -55,7 +55,7 @@ namespace llarp
       std::string m_NetNS;
       bool m_BundleRC = false;
 
-      util::Mutex m_SendQueueMutex;
+      util::Mutex m_SendQueueMutex;  // protects m_SendQueue
       std::deque< SendEvent_t > m_SendQueue GUARDED_BY(m_SendQueueMutex);
 
       PendingTraffic m_PendingTraffic;

llarp/service/protocol.cpp

@@ -309,7 +309,8 @@ namespace llarp
 
         if(self->frame.T != self->msg->tag)
         {
-          LogError("convotag missmatch: ", self->frame.T, " != ", self->msg->tag);
+          LogError("convotag missmatch: ", self->frame.T,
+                   " != ", self->msg->tag);
           self->msg.reset();
           delete self;
           return;

llarp/util/metrics_core.cpp

@@ -432,11 +432,11 @@ namespace llarp
     struct PublisherSchedulerData
     {
       util::Mutex m_mutex;
-      thread::Scheduler::Handle m_handle;
-      std::set< const Category * > m_categories;
+      thread::Scheduler::Handle m_handle GUARDED_BY(m_mutex);
+      std::set< const Category * > m_categories GUARDED_BY(m_mutex);
 
-      bool m_default;
-      std::set< const Category * > m_nonDefaultCategories;
+      bool m_default GUARDED_BY(m_mutex);
+      std::set< const Category * > m_nonDefaultCategories GUARDED_BY(m_mutex);
 
       PublisherSchedulerData()
           : m_handle(thread::Scheduler::INVALID_HANDLE), m_default(false)
@@ -506,11 +506,9 @@ namespace llarp
       m_categories.erase(it);
       auto data = repeatIt->second;
 
-      {
-        util::Lock l(&data->m_mutex);
-        assert(data->m_categories.find(category) != data->m_categories.end());
-        data->m_categories.erase(category);
-      }
+      util::Lock l(&data->m_mutex);
+      assert(data->m_categories.find(category) != data->m_categories.end());
+      data->m_categories.erase(category);
 
       if(!data->m_default)
       {
@@ -526,7 +524,7 @@ namespace llarp
           assert(defaultIntervalIt != m_repeaters.end());
 
           auto &defaultRepeater = defaultIntervalIt->second;
-          util::Lock l(&defaultRepeater->m_mutex);
+          util::Lock lock(&defaultRepeater->m_mutex);
           defaultRepeater->m_nonDefaultCategories.erase(category);
         }
       }
@@ -547,6 +545,8 @@ namespace llarp
       assert(repeatIt != m_repeaters.end());
       auto data = repeatIt->second;
 
+      util::Lock l(&data->m_mutex);
+
       if(data->m_categories.empty())
       {
         assert(data->m_handle != thread::Scheduler::INVALID_HANDLE);
@@ -555,7 +555,6 @@ namespace llarp
       }
       else
       {
-        util::Lock l(&data->m_mutex);
         data->m_default = false;
         data->m_nonDefaultCategories.clear();
       }
@@ -593,9 +592,9 @@ namespace llarp
       if(repeatIt == m_repeaters.end())
       {
         data = std::make_shared< PublisherSchedulerData >();
+        util::Lock lock(&data->m_mutex);
         data->m_categories.insert(category);
         m_repeaters.emplace(interval, data);
-        util::Lock lock(&data->m_mutex);
         data->m_handle = m_scheduler.scheduleRepeat(
             interval, std::bind(&PublisherScheduler::publish, this, data));
       }
@@ -609,6 +608,7 @@ namespace llarp
       // If this isn't being added to the default schedule, then add to the set
       // of non-default categories in the default schedule.
 
+      util::Lock dataLock(&data->m_mutex);
       if(!data->m_default && m_defaultInterval != absl::Duration())
       {
         auto defaultIntervalIt = m_repeaters.find(m_defaultInterval);
@@ -703,6 +703,7 @@ namespace llarp
       util::Lock l(&m_mutex);
       for(auto &repeat : m_repeaters)
       {
+        util::Lock dataLock(&repeat.second->m_mutex);
         m_scheduler.cancelRepeat(repeat.second->m_handle, true);
       }
 

llarp/util/metrics_core.hpp

@@ -56,7 +56,7 @@ namespace llarp
 
      private:
       TaggedRecordsType m_records GUARDED_BY(m_mutex);
-      mutable util::Mutex m_mutex;
+      mutable util::Mutex m_mutex;  // protects m_records
 
       Collector(const Collector &) = delete;
       Collector &
@@ -280,7 +280,8 @@ namespace llarp
       CategoryMap m_categories GUARDED_BY(m_mutex);
       MetricMap m_metrics GUARDED_BY(m_mutex);
       bool m_defaultEnabled GUARDED_BY(m_mutex);
-      mutable util::Mutex m_mutex;
+      mutable util::Mutex m_mutex;  // protects m_stringmem, m_categories,
+                                    // m_metrics, m_defaultEnabled
 
       Registry(const Registry &) = delete;
       Registry &
@@ -374,16 +375,16 @@ namespace llarp
 
       Registry *m_registry;
       IdCollectors m_collectors;
-      CategoryCollectors m_categories;
+      CategoryCollectors m_categories GUARDED_BY(m_mutex);
 
-      mutable util::Mutex m_mutex;
+      mutable util::Mutex m_mutex;  // protects m_categories
 
       CollectorRepo(const CollectorRepo &) = delete;
       CollectorRepo &
       operator=(const CollectorRepo &) = delete;
 
       Collectors< Type > &
-      getCollectors(const Id &id)
+      getCollectors(const Id &id) SHARED_LOCKS_REQUIRED(m_mutex)
       {
         auto it = m_collectors.find(id);
 
@@ -406,7 +407,7 @@ namespace llarp
 
       template < TaggedRecords< Type > (Collectors< Type >::*func)() >
       std::vector< TaggedRecords< Type > >
-      collectOp(const Category *category)
+      collectOp(const Category *category) LOCKS_EXCLUDED(m_mutex)
       {
         absl::WriterMutexLock l(&m_mutex);
 
@@ -452,7 +453,7 @@ namespace llarp
       }
 
       Collector< Type > *
-      defaultCollector(const Id &id)
+      defaultCollector(const Id &id) LOCKS_EXCLUDED(m_mutex)
       {
         {
           absl::ReaderMutexLock l(&m_mutex);
@@ -476,14 +477,14 @@ namespace llarp
       }
 
       std::shared_ptr< Collector< Type > >
-      addCollector(const Id &id)
+      addCollector(const Id &id) LOCKS_EXCLUDED(m_mutex)
       {
         absl::WriterMutexLock l(&m_mutex);
         return getCollectors(id).add();
       }
 
       std::vector< std::shared_ptr< Collector< Type > > >
-      allCollectors(const Id &id)
+      allCollectors(const Id &id) LOCKS_EXCLUDED(m_mutex)
       {
         absl::ReaderMutexLock l(&m_mutex);
 
@@ -1134,7 +1135,8 @@ namespace llarp
       Repeaters m_repeaters GUARDED_BY(m_mutex);
       absl::Duration m_defaultInterval GUARDED_BY(m_mutex);
 
-      mutable util::Mutex m_mutex;
+      mutable util::Mutex
+          m_mutex;  // protected m_categories, m_repeaters, m_defaultInterval
 
       void
       publish(const std::shared_ptr< PublisherSchedulerData > &data) const;

llarp/util/metrics_types.hpp

@@ -200,56 +200,56 @@ namespace llarp
       }
 
       void
-      category(const Category *c)
+      category(const Category *c) LOCKS_EXCLUDED(m_mutex)
       {
         util::Lock l(&m_mutex);
         m_category = c;
       }
 
       const Category *
-      category() const
+      category() const LOCKS_EXCLUDED(m_mutex)
       {
         util::Lock l(&m_mutex);
         return m_category;
       }
 
       void
-      name(string_view n)
+      name(string_view n) LOCKS_EXCLUDED(m_mutex)
       {
         util::Lock l(&m_mutex);
         m_name = n;
       }
 
       string_view
-      name() const
+      name() const LOCKS_EXCLUDED(m_mutex)
       {
         util::Lock l(&m_mutex);
         return m_name;
       }
 
       void
-      type(Publication::Type t)
+      type(Publication::Type t) LOCKS_EXCLUDED(m_mutex)
       {
         util::Lock l(&m_mutex);
         m_type = t;
       }
 
       Publication::Type
-      type() const
+      type() const LOCKS_EXCLUDED(m_mutex)
       {
         util::Lock l(&m_mutex);
         return m_type;
       }
 
       void
-      format(const std::shared_ptr< Format > &f)
+      format(const std::shared_ptr< Format > &f) LOCKS_EXCLUDED(m_mutex)
       {
         util::Lock l(&m_mutex);
         m_format = f;
       }
 
       std::shared_ptr< Format >
-      format() const
+      format() const LOCKS_EXCLUDED(m_mutex)
       {
         util::Lock l(&m_mutex);
         return m_format;