0

I recently got bonked pretty hard in a code review, implementing an ASIO UDP socket within an interface adapter; it seems that there was another input UDP socket implemented and both input and output were assumed to be on the same thread. So, I'm wondering why the ASIO socket libraries don't maintain a static thread (socket context) and use that for each socket? What is the motivation and the trade-offs to consider in employing the Proactor pattern?

Edit / Addendum

After seeing some of the comments about my questions being unclear, I'm adding this code snippet based upon the class definition that I was told didn't follow the Proactor pattern:

class InterfaceAdapter{
  public:
    typedef std::vector<MsgFragment> MsgPackets;
    InterfaceAdapter() :
      mySocket(myContext) {}
    void sendDataToSystem(const DataStruct& originalData);
  private:
    asio::io_context myContext;
    asio::ip::udp::socket mySocket;
    MsgPackets transformData(const DataStruct& originalData);
    void sendPackets(const MsgPackets& msgs);
};

Apparently I needed to use a globally-scoped asio::io_context instead of having it as a private member & using it to default-construct the socket?

jhill515
  • 894
  • 8
  • 26
  • 1
    Can you clarify the question.It's not clear whether "another UDP socket...same thread" refers to the implementation deemed "correct" or the one you made. Also, if one was deemed correct, do you know why? What is your `io_service` (`io_context` in recent versions) setup (on how many threads does it run? how many separate contexts do you use etc) – sehe Feb 21 '19 at 10:13
  • 2
    Oh and the Proactor Design Pattern _is_ the default. Unless you go out of your way to run a service on multiple threads, all async operations run on the same thread. – sehe Feb 21 '19 at 10:14
  • Thank you for asking about that. I added an addendum to the question that should help clarify the comment I received. Apparently there is an input ASIO socket whose context I should be sharing. Granted, this is a new library for me (long history of working in "Not Invented Here" dev teams), so admittedly my understanding is limited. – jhill515 Feb 22 '19 at 10:02
  • Did my answer answer your question? If so please mark it as "accepted" – Superlokkus Feb 26 '19 at 16:17

1 Answers1

2

Your questions is not really clear and mixing or implying things, and I assume that is also the cause of your problem:

Boost ASIO is a proactor pattern, asynchronous handlers are executed, usually after the finish of another handler i.e. callback. This can be done concurrently if the user chooses so by running boost::asio::io_context::run on more than 1 thread.

Boost ASIO gives that freedom to you, it makes no sense for this library to restrict itself to this corner case without any motivation. Static or global variables i.e. threads are also considered broadly as extreme bad style.

However your question suggests that your program i.e. architecture is designed to be single threaded, and the code you wrote was using ASIO as one would if using multiple threads (which should not have any significant overhead at runtime anyway), or your reviewer also misunderstood the boost ASIO semantics. Without your code and specific reasons this remains speculation tough.

Addendum to your addendum: No I does not have to be global, I assume the reviewers point is that you have your own asio::io_context, which you usually don't need, because your class seems to be just about sending packets, so for it, it should be indifferent on which io_context it runs on. Thats the reason boost sockets and alike just take a reference to an io_context, I do this my self, for example a RTP class I wrote. There you can see I just store a reference to the io_context which the overall RTSP Videostreaming server manages.

However I fear your company/reviewer doesn't use boost asio otherwise and your adapter might be the first one using boost asio internal but is not intended to leak this implementation detail out. Then it depends on how your class is used: Will there usually be only one instance the whole program life time, like this? Then it could manage it's own io_context, but I assume you will rather create several instances of it. Imagine the boost tcp connection i.e. socket creating threads and everything just for itself, for every tcp connection a server has, that would be stupid.

So the solution then would be either also just take a io_context& in your constructor, or if then want to avoid the boost details, another class designed by you, which the creator of InterfaceAdapter has to keep, and reuses every time he creates a new interface adapter. However should that not be possible, than you should really refactor your whole program, but that would be the moment where mediocre would start to use globals. But then don't make your InterfaceAdapter or a boost::io_context global, but something like class my_io_singleton which still has to be given into your InterfaceAdpater, so that one better day, your code will be easily refactored.

Update 2 The next thing could off rail you again so I advise you to read it only after reading the above part and after you had done some more implementation with boost asio, as it is not really important nor relevant for your case: To be fair, there a rare occasions where boost io_contexts seem to be singleton like, I stumbled over myself, but that are just comfort functions build in by asio, which one could argue might be better left out. But they can simply be ignored.

Superlokkus
  • 4,731
  • 1
  • 25
  • 57
  • Thank you for addressing that. I edited the question to include the class definition which received the comment "You're not following the Proactor Pattern." From what I can understand, `asio::io_context` seems singleton-like, at least when I read the documentation, but the review comment I got claims otherwise. – jhill515 Feb 22 '19 at 10:04
  • It is not a singleton. You can have many (though that's rarely useful for simple applications. So yes, you do share `io_contexts` in an application) – sehe Feb 22 '19 at 10:18