Skip to content

Fix network implementation for better error handling and multithreading

Béla requested to merge masodikbela/core:network-fix into master

According to the boost::asio documentation only "types such as io_context provide a stronger guarantee that it is safe to use a single object concurrently" which means our current implementation is not entirely thread safe, because for example its possible to use the _socket concurrently, if for example the update thread calls Send() while the network thread performs a Close() or Read().

New elements I used and reasons behind them:

  • _strand: This is only necessary if we want to use more threads for the networking, so it is possible now to have more threads calling run on the _context from the core::Application. Also I'm not entirely sure if its safe to post on the main context, because what if the object gets destroyed while there are active posts related to this (probably this can't happen because we use the shared_ptr for binding).
  • _sendQueue: Now to ensure that the same thread processes both read and send (or at least let asio decide when to call them making sure its thread safe) we use boost::asio::post on the connection's own strand, which will enqueue the HandleSend operation. To make sure that no multiple HandleSends are enqueued, we feed the packets into this _sendQueue, and only post the HandleSend if the _sendQueue was empty before.
  • New PostClose(), Close(): Calling Close() outside of the main thread was not thread safe too, since _socket is a single object and not guaranteed to be thread safe. Therefore following the same logic, I post a Close() onto the connection's _strand to enqueue it. To ensure no multiple *Close()*s are posted or no Close()s are called from the network thread, I set the _isClosing to true which is an std::atomic and safe to call simultaneously.
  • PostShutdown() and Shutdown(): Asio recommends the following: For portable behaviour with respect to graceful closure of a connected socket, call shutdown() before closing the socket. Calling to Close() first should only happen if an error occurred.

Also added error handling to HandleReadSize and HandleReadData, because it could be possible that an error happen between those and the HandleReadHeader call, since HandleReadHeader only enqueues those two. Added connection close if a write operation fails.

Remarks: I had to move the activation of the cryptation to the HandleReadHeader, because of the queue I implemented.

Edited by Béla

Merge request reports