Refactor router code #728

tewinget · 2019-07-22T21:31:17Z

This PR refactors a lot of the functionality that is "kitchen sink"ed into router.cpp into separate classes.

This PR will be squashed before merge.

There are also some minor behavior changes along with this refactor as I came across behavior that was either incorrect or sub-optimal.

llarp/link/i_link_manager.hpp

michael-loki · 2019-07-22T23:25:26Z

llarp/link/link_manager.cpp

+  {
+    {
+      util::Lock l(&_mutex);
+      if (stopping) return nullptr;


you don't need to lock to check an atomic

michael-loki · 2019-07-22T23:26:20Z

llarp/link/link_manager.cpp

+  LinkManager::SendTo(const RouterID& remote, const llarp_buffer_t& buf)
+  {
+    {
+      util::Lock l(&_mutex);


michael-loki · 2019-07-22T23:27:13Z

llarp/link/link_manager.cpp

+    if (!stopping)
+    {
+      LogInfo("stopping links");
+      stopping.store(true);


stopping = true;

llarp/link/link_manager.cpp

michael-loki · 2019-07-22T23:34:10Z

llarp/router/outbound_message_handler.cpp

+    {
+      DoCallback(msg.second, SendStatus::Success);
+    }
+    DoCallback(msg.second, SendStatus::Congestion);


is this what you mean? we always call the congestion func rn

I'm not sure what you mean by this, but I don't see any congestion function anywhere in the old SendToOrQueue codepath.

When I reviewed the function looked like https://github.com/loki-project/loki-network/blob/1d6f010ea9056ff4deee38cd97de3d47727095bb/llarp/router/outbound_message_handler.cpp#L162-L169

michael-loki · 2019-07-22T23:34:44Z

llarp/router/outbound_message_handler.hpp

+  struct Logic;
+  enum class SessionResult;
+
+  struct OutboundMessageHandler : public IOutboundMessageHandler


michael-loki · 2019-07-22T23:35:39Z

llarp/router/outbound_session_maker.cpp

+
+    LogInfo("session with ", router, " established");
+
+    if (not _rcLookup->RemoteIsAllowed(router))


I actually prefer the not token over !

Does MSVC support it yet (without #include <ciso646>)?

Actually good question. I'm not sure. you just need iso646.h (since c99) but msvc is weird and is a c++ exclusive compiler 🤷‍♀

update on this? Also, perhaps make format can change to the preferred style? Not sure, haven't really used it, but I imagine it'd be cool if it could.

llarp/router/outbound_session_maker.cpp

michael-loki · 2019-07-22T23:40:45Z

llarp/router/outbound_session_maker.cpp

+  }
+
+  void
+  Init(LinkManager *linkManager,


I hate init methods. why not a constructor which does this?

majestrate · 2019-07-23T13:22:07Z

llarp/router/rc_lookup_handler.cpp

+        [&](const RouterContact &rc) {
+          if(HavePendingLookup(rc.pubkey))
+            return;
+          GetRC(rc.pubkey, nullptr);


GetRC calls stuff that acquires a lock that is held while visiting.

That's...gonna be fun to resolve.

majestrate · 2019-07-23T13:31:47Z

llarp/router/rc_lookup_handler.cpp

+  }
+
+  void
+  RCLookupHandler::Init(llarp_dht_context *dht,


this should be the constructor.

See comment above, not changed.

majestrate · 2019-07-23T13:32:06Z

llarp/link/link_manager.cpp

+  }
+
+  void
+  LinkManager::Init(IOutboundSessionMaker *sessionMaker)


this should ALSO be the constructor.

So for objects which need to point to one another you would end up mixing and matching this style and in-constructor init of pointers. Ideally that situation wouldn't occur, but it's non-trivial to avoid. In order to have consistency among these new classes I opted for this style.

i am segfaulting in my local testing because a timer is called with a member being nullptr and i assumed it's happpening before calling init

The Inits are run during Router::FromConfig. If that's not early enough then the lack of segfault before was just masking something using Router before it was fully configured/initialized.

After further discussion, leaving it as-is.

Init functions like this are evils in API design :( but okay

I can definitely agree with arguments against doing it this way. It's definitely something to revisit in the future.

majestrate · 2019-07-23T13:32:20Z

llarp/router/outbound_session_maker.cpp

+  }
+
+  void
+  OutboundSessionMaker::Init(ILinkManager *linkManager,


also this should be a constructor. :>

See comment above, not changed.

majestrate · 2019-07-23T13:32:31Z

llarp/router/outbound_message_handler.cpp

+  }
+
+  void
+  OutboundMessageHandler::Init(ILinkManager* linkManager,


also a constructor

See comment above, not changed.

majestrate · 2019-07-23T13:34:32Z

llarp/link/link_manager.cpp

+                 false
+               );
+
+    auto sz = connectedRouters.size();


this should be const

majestrate · 2019-07-23T13:36:17Z

llarp/link/link_manager.hpp

+    std::unordered_map< RouterID, llarp_time_t, RouterID::Hash >
+        m_PersistingSessions GUARDED_BY(_mutex);
+
+    IOutboundSessionMaker *_sessionMaker;


should this be a const * ?

only if I change it to be set in the constructor as @michael-loki suggested, but in case / because some of these things need to point to one another I opted to set all of their pointers in an Init function. Naturally the pointers are private to at least mostly mitigate the risk of accidentally changing them rather than using them.

I suppose I could make these pointers const and cast the const away in the Init function, but that also feels messy. Thoughts?

not casting const away, no change.

majestrate · 2019-07-23T13:37:35Z

llarp/router/outbound_message_handler.cpp

+      case SessionResult::NoLink:
+        OnNoLink(router);
+        break;
+      default:


this should probably error on default case "just in case"

I guess, but that would be a very interesting case.

Added LogError and abort.

majestrate · 2019-07-23T13:38:07Z

llarp/router/outbound_message_handler.hpp

+      std::unordered_map< RouterID, MessageQueue, RouterID::Hash >
+          outboundMessageQueue GUARDED_BY(_mutex);
+
+      ILinkManager* _linkManager;


should this also be a const *?

See comment above, not changed.

majestrate · 2019-07-23T13:39:42Z

llarp/router/outbound_session_maker.cpp

+        }
+        else
+        {
+          //TODO: shouldn't happen, how to handle?


maybe abort() ? probably log error

added LogError but not abort.

majestrate · 2019-07-23T13:40:27Z

llarp/router/outbound_session_maker.hpp

+      ILinkManager *_linkManager;
+      I_RCLookupHandler *_rcLookup;
+      std::shared_ptr< Logic > _logic;
+      llarp_nodedb *_nodedb;


See comment above, not changed.

majestrate · 2019-07-23T13:40:48Z

llarp/router/outbound_session_maker.hpp

+                          RouterID::Hash >
+          pendingCallbacks GUARDED_BY(_mutex);
+
+      ILinkManager *_linkManager;


* const (i kept on saying const * >.>)

which the compiler corrected several times as a result...

majestrate · 2019-07-23T13:42:30Z

llarp/router/rc_lookup_handler.cpp

+    // update nodedb if required
+    if (rc.IsPublicRouter())
+    {
+      _nodedb->UpdateAsyncIfNewer(rc);


this doesn't update the inserted timestamp so it either should be InsertAsync or make UpdateAsyncIfNewer change insertion timestamps.

I think changing UpdateAsyncIfNewer is cleaner, which do you prefer?

changing UpdateAsyncIfNewer works fine

majestrate · 2019-07-23T13:44:08Z

llarp/router/rc_lookup_handler.cpp

+
+      while (not callbackQueue.empty())
+      {
+        callbackQueue.front()(router, rc, result);


what happens if the callback eventually wants to acquire the RCLookupHandler lock again?

fair point. I don't particularly like the idea of collecting the callbacks in a local container first (and then unlocking) but that's probably for the best.

majestrate · 2019-07-23T13:44:35Z

llarp/router/rc_lookup_handler.hpp

+
+      mutable util::Mutex _mutex; // protects pendingCallbacks, whitelistRouters
+
+      llarp_dht_context *_dht = nullptr;


See comment above, not changed.

michael-loki · 2019-07-24T22:20:01Z

llarp/link/link_manager.cpp

+  }
+
+  void
+  LinkManager::Init(IOutboundSessionMaker *sessionMaker)


Init functions like this are evils in API design :( but okay

michael-loki · 2019-07-24T22:22:58Z

llarp/router/i_outbound_session_maker.hpp

+  };
+
+  using RouterCallback =
+      std::function< void(const RouterID, const SessionResult) >;


const RouterID&?

Yea, will change now.

michael-loki · 2019-07-24T22:23:48Z

llarp/router/outbound_message_handler.cpp

+  void
+  OutboundMessageHandler::OnConnectFailure(const RouterID &router)
+  {
+    FinalizeRequest(router, SendStatus::Timeout);


is a connect failure always a timeout?

when this func is called, yes. Changing func name.

michael-loki · 2019-07-24T22:26:59Z

llarp/router/outbound_message_handler.cpp

+    {
+      DoCallback(msg.second, SendStatus::Success);
+    }
+    DoCallback(msg.second, SendStatus::Congestion);


When I reviewed the function looked like https://github.com/loki-project/loki-network/blob/1d6f010ea9056ff4deee38cd97de3d47727095bb/llarp/router/outbound_message_handler.cpp#L162-L169

michael-loki · 2019-07-24T22:30:08Z

llarp/router/outbound_session_maker.cpp

+
+    LinkLayer_ptr link = _linkManager->GetCompatibleLink(rc);
+
+    if(link == nullptr)


if(!link)?

michael-loki · 2019-07-24T22:51:03Z

llarp/router/rc_lookup_handler.cpp

+        {
+          collectedCallbacks.push_back(callbackQueue.front());
+          callbackQueue.pop();
+        }


I think this could be simplified to be:

std::vector< RCRequestCallback > collectedCallbacks; { util::Lock l(&_mutex); auto itr = pendingCallbacks.find(router); if(itr != pendingCallbacks.end()) { auto &callbackQueue = itr->second; if(callbackQueue.size()) { collectedCallbacks.reserve(callbackQueue.size()); } collectedCallbacks.assign(callbackQueue.begin(), callbackQueue.end()); pendingCallbacks.erase(itr); } } // lock for(const auto &callback : collectedCallbacks) { callback(router, rc, result); }

yeah, probably. neat.

majestrate · 2019-07-25T14:28:24Z

llarp/router/outbound_message_handler.cpp

@@ -25,8 +25,6 @@ namespace llarp
      return false;
    }

-    buf.sz = buf.cur - buf.base;


majestrate · 2019-07-25T17:30:02Z

llarp/router/outbound_session_maker.cpp

+  {
+    // TODO: do we want to keep it
+
+    const auto &router = RouterID(session->GetPubKey());


const RouterID router{session->GetPubKey()};

majestrate · 2019-07-25T17:31:14Z

llarp/router/outbound_session_maker.cpp

+
+    CreatePendingSession(router);
+
+    LogWarn("Creating session establish attempt to ", router, " .");


this is a stray LogWarn

majestrate · 2019-07-25T17:31:24Z

llarp/router/outbound_session_maker.cpp

+  {
+    // TODO: retry/num attempts
+
+    LogWarn("Session establish attempt to ", RouterID(session->GetPubKey()),


nvm this is fine

majestrate · 2019-07-25T17:37:30Z

llarp/router/rc_lookup_handler.hpp

+    llarp_dht_context *_dht = nullptr;
+    llarp_nodedb *_nodedb   = nullptr;
+    std::shared_ptr< llarp::thread::ThreadPool > _threadpool;
+    service::Context *_hiddenServiceContext;


this should set to nullptr like the other bare pointers

majestrate · 2019-07-25T17:37:38Z

llarp/router/rc_lookup_handler.hpp

+    llarp_nodedb *_nodedb   = nullptr;
+    std::shared_ptr< llarp::thread::ThreadPool > _threadpool;
+    service::Context *_hiddenServiceContext;
+    ILinkManager *_linkManager;


also this too

majestrate

squash it

majestrate · 2019-07-25T18:06:35Z

100 comments and a 3 way merge conflict ain't one :>

This commit refactors functionality from the Router class into separate, dedicated classes. There are a few behavior changes that came as a result of discussion on what the correct behavior should be. In addition, many things Router was previously doing can now be provided callback functions to alert the calling point when the asynchronous action completes, successfully or otherwise.

majestrate · 2019-07-25T18:23:44Z

YOLO!

michael-loki reviewed Jul 22, 2019

View reviewed changes

majestrate reviewed Jul 23, 2019

View reviewed changes

michael-loki reviewed Jul 24, 2019

View reviewed changes

majestrate reviewed Jul 25, 2019

View reviewed changes

tewinget force-pushed the refactor-router-code branch from 83034e4 to fbd5746 Compare July 25, 2019 17:44

majestrate approved these changes Jul 25, 2019

View reviewed changes

tewinget force-pushed the refactor-router-code branch from fbd5746 to 3f69228 Compare July 25, 2019 18:04

tewinget force-pushed the refactor-router-code branch from 3f69228 to baf8019 Compare July 25, 2019 18:11

majestrate merged commit f7e05ad into oxen-io:master Jul 25, 2019


		LogInfo("session with ", router, " established");

		if (not _rcLookup->RemoteIsAllowed(router))


		mutable util::Mutex _mutex; // protects pendingCallbacks, whitelistRouters

		llarp_dht_context *_dht = nullptr;


		LinkLayer_ptr link = _linkManager->GetCompatibleLink(rc);

		if(link == nullptr)


		CreatePendingSession(router);

		LogWarn("Creating session establish attempt to ", router, " .");

Refactor router code #728

Refactor router code #728

Conversation

tewinget commented Jul 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment