Do I need to use a boost::strand in this server model?

Question

I've been coding this server for quite some time now. I am calling io_service::run from more than 1 thread, but I am not sure if I ned to use strand here. Since I am never calling async_read more than once. Though it is possible that I call async_write more than once if other connections need to send something to others. This is the code I have so far

void Session::RecvPacket()
{
    boost::asio::async_read(m_socket, boost::asio::buffer(m_recv_buffer, len),
        boost::bind(&Session::OnRead, this, boost::asio::placeholders::error,
        boost::asio::placeholders::bytes_transferred));

}

void Session::OnRead(const boost::system::error_code &e, size_t packet_length, size_t bytes_transferred)
{
    if (e || bytes_transferred != packet_length)
    {
        if (e == boost::asio::error::operation_aborted)
            return;

        Stop();
        return;
    }

    // do something and most likely send a packet back

    RecvPacket();
}
void Session::SendPacket(packet &p)
{
    boost::mutex::scoped_lock lock(send_mut);

    dword len = p.Lenght(); 

    m_crypt->makeheader(p.GetRaw().get(), static_cast<word>(len - 4));
    m_crypt->encrypt(p.GetRaw().get() + 4, len - 4);

    boost::asio::async_write(m_socket, boost::asio::buffer(p.GetRaw().get(), len),
        boost::bind(&Session::OnPacketSend, this, len, boost::asio::placeholders::error,
        boost::asio::placeholders::bytes_transferred, p.GetRaw()));
}

I am not sure wheter this is threadsafe or not. If only the send part is not threadsafe since I can send more than 1 packet at once (which is going to happen) should I only use a strand there?

Gz

Answer 1

Yes, you need a strand.

As noted in the tcp::socket documentation, concurrent calls on the same socket (i.e. shared object) is not thread safe. However, it is safe to have multiple asynchronous operations pending on an object. Thus, this is safe:

 thread_1                             | thread_2
--------------------------------------+---------------------------------------
socket.async_receive(...);            |
socket.async_write_some(...);         |

and this is safe:

 thread_1                             | thread_2
--------------------------------------+---------------------------------------
socket.async_receive(...);            |
                                      | socket.async_write_some(...);

but this is specified as not being safe:

 thread_1                             | thread_2
--------------------------------------+---------------------------------------
socket.async_receive(...);            | socket.async_write_some(...);
                                      |

Furthermore, the send_mut mutex in Session::SendPacket() provides no real safety to m_socket. The boost::asio::async_write() operation is a composed operation, made up of zero or more call's to the m_socket.async_write_some() function. Due to the lifespan of the lock and the behavior of asynchronous operations, the lock does not guarantee that the mutex will be held during any or all of the async_write_some() operations.

For more details on thread safety and strands, consider reading this answer.

---

Also, be aware that once an async_write() operation has been initiated on a stream, the program must ensure that no other write occurs on the stream until the initial operation's completion handler has been invoked:

The program must ensure that the stream performs no other write operations (such as async_write, the stream's async_write_some function, or any other composed operations that perform writes) until this operation completes.

If multiple writes need to be performed, then consider queuing up the writes, as shown in this answer. This type of solution may also make managing the lifespan of the packet object's underlying buffer easier, as the queue will own a copy of the data, meeting the async_write() operation's requirement that the buffer's underlying memory blocks remain valid until the completion handler is called.