boost asio: Is it thread safe to call tcp::socket::async_read_some() when handler is protected by a strand

Question

I'm struggle to full understand Boost ASIO and strands. I was under the impression that the call to socket::async_read_some() was safe as long as the handler was wrapped in a strand. This appears not to be the case since the code eventually throws an exception.

In my situation a third party library is making the Session::readSome() calls. I'm using a reactor pattern with the ASIO layer under the third party library. When data arrives on the socket the 3rd party is called to do the read. The pattern is used since it is necessary to abort the read operation at any time and have the 3rd party library error out and return its thread. The third party expected a blocking read so the code mimics it with a conditional variable.

Given the example below what is the proper way to do this? Do I need to wrap the async_read_some() call in a dispatch() or post() so it runs through a strand too?

Note: Compiler is c++14 ;-(

Example representative code:

Session::Session (ba::io_context& ioContext): 
     m_sessionStrand ( ioContext.get_executor() ),
     m_socket ( m_sessionStrand )
{}

int32_t Session::readSome (unsigned char* pBuffer, uint32_t bufferSizeToRead, boost::system::error_code& errorCode)
{
   // The 3d party expects a synchronous read so we mimic the behavior 
   // with a async_read and then wait for the results. With this pattern 
   // we can unblock the read elsewhere - for or example calling close on the socket -
   // and still give the 3d party the illusion of a synchronous read.  
   // In such a cases the 3rd party will receive an error code 
   // on the read and return it's thread.

   // Nothing to do
   if ( bufferSizeToRead == 0) return 0;

   // Create a mutable buffer
   ba::mutable_buffer buffer (pBuffer, bufferSizeToRead);
   std::size_t result = 0;
   errorCode.clear();

   // Setup conditional 
   m_readerPause.exchange(true);

   auto readHandler = [&result, &errorCode, self=shared_from_this()](boost::system::error_code ec, std::size_t bytesRead)
   {      
      result = bytesRead;
      errorCode = ec;
      
      // Signal that we got results
      std::unique_lock<std::mutex> lock{m_readerMutex};
      m_readerPause.exchange(false);
      m_readerPauseCV.notify_all();
   };

   m_socket.async_read_some(buffer, ba::bind_executor (m_sessionStrand, readHandler));

   // We pause the 3rd party read thread until we get the read results back - or an error occurs
   {
        std::unique_lock<std::mutex> lock{m_readerMutex};
        m_readerPauseCV.wait (lock, [this]{ return !m_readerPause.load(std::memory_order_acquire); } );
    }

   return result;
}

The exception occurs in epoll_reactor.ipp. There is a race condition between the read and closing the socket.

void epoll_reactor::start_op(int op_type, socket_type descriptor,
    epoll_reactor::per_descriptor_data& descriptor_data, reactor_op* op,
    bool is_continuation, bool allow_speculative)
{
  if (!descriptor_data)
  {
    op->ec_ = boost::asio::error::bad_descriptor;
    post_immediate_completion(op, is_continuation);
    return;
  }

  mutex::scoped_lock descriptor_lock(descriptor_data->mutex_);

  if (descriptor_data->shutdown_)  //!! SegFault here: descriptor_data == NULL*
  {
    post_immediate_completion(op, is_continuation);
    return;
  }
...
}

Thanks in advance for any insights in the proper way to handle this situation using ASIO.

Answer 1

The strand doesn't "protect" the handler. Instead, it protects some shared state (which you control) by synchronizing handler execution. It's exactly like a mutex for async execution.

According to this logic all code running on the strand can touch the shared resources, and conversely, code not guaranteed to be on the strand can not be allowed to touch them.

In your code, the shared resources consist of at least buffer, result, m_socket. It would be more complete to include the m_sessionStrand, m_readerPauseCV, m_readerMutex, m_readerPause but all of these are implicitly threadsafe the way they are used¹.

Your code looks to do things safely in these regards. However it makes a few unfortunate detours that make it harder than necessary to check/reason about the code:

1. it uses more (local) shared state to communicate results from the handler

2. it doesn't make explicit what the mutex and/or the strand protect

3. it employs both a mutex and a strand which conceptually compete for the same responsibility

4. it employs both a condition and an atomic bool, which again compete for the same responsibility

5. it does manual strand binding, which muddies the expectations about what the native executor for the m_socket object is expected to be

6. the initial read is not protected. This means that if Session::readSome is invoked from a "wild" thread, it will use member functions without synchronizing with any other operations that may be pending on the m_socket.

7. the atomic_bool mutations are spelled in Very Convoluted Ways(TM), which serve to show you (presumably) understand the memory model, but make the code harder to review without tangible merit. Clearly, the blocking synchronization will (far) outweigh any benefit of explicit memory acquisition order. I suggest to at least "normalize" the spelling as atomic_bool was explicitly designed to afford:

   //m_readerPause.exchange(true);
   m_readerPause = true;
   

   and

   m_readerPauseCV.wait(lock, [this] { return !m_readerPause; });
   

8. since you are emulating blocking IO, there is no merit capturing shared_from_this() in the lambda. Lifetime should be guaranteed by the calling party any ways.

   Interestingly, you didn't show this capture, which is required for the lambda to compile, assuming you didn't use global variables.

9. Kudos for explicitly clearing the error_code output variable. This is oft forgotten. Technically, you did forget about with the (questionable?) early exit when (bufferSizeToRead == 0)... You might have a slightly unorthodox caller contract where this makes sense.

   To be generic I'd suggest to perform the zero-length read as it might behave differently depending on the transport connected.

10. Last, but not least, m_socket.[async_]read_some is rarely what you require on application protocol level. I'll leave this one to you, as you might have this exceptional edge-case scenario.

Simplifying

Conceptually, I'd like to write:

int32_t Session::readSome(unsigned char* buf, uint32_t size, error_code& ec) {
    ec.clear();
    size_t result = 0;

    std::tie(ec, result) = m_socket
            .async_read_some(ba::buffer(buf, size),
                             ba::as_tuple(ba::use_future))
            .get();
    return result;
}

This uses futures to get the blocking behaviour while being cancelable. Sadly, contrary to expectation there is currently a limitation that prevents combining as_tuple and use_future.

So, we have to either ignore partial success scenarios (significant result when !ec):

int32_t Session::readSome(unsigned char* buf, uint32_t size, error_code& ec) try {
    ec.clear();
    return m_socket
        .async_read_some(ba::buffer(buf, size), ba::use_future)
        .get();
} catch (boost::system::system_error const& se) {
    ec = se.code();
    return 0;
}

I suspect that member-async_read_some doesn't have a partial success mode. However, let's still give it thought, seeing that I warned before that async_read_some is rarely what you need anyways:

int32_t Session::readSome(unsigned char* buf, uint32_t size, error_code& ec) {
    std::promise<std::tuple<size_t, error_code> > p;

    m_socket.async_read_some(ba::buffer(buf, size), [&p](error_code ec_, size_t n_) { p.set_value({n_, ec_}); });

    size_t result;
    std::tie(result, ec) = p.get_future().get();
    return result;
}

Still considerably easier.

Interim Result

Self contained example with the current approach:

Live On Coliru

#include <boost/asio.hpp>
namespace ba = boost::asio;
using ba::ip::tcp;
using boost::system::error_code;

using CharT = /*unsigned*/ char; // for ease of output...

struct Session : std::enable_shared_from_this<Session> {
    tcp::socket m_socket;

    Session(ba::any_io_executor ex) : m_socket(make_strand(ex)) {
        m_socket.connect({{}, 7878});
    }

    int32_t readSome(CharT* buf, uint32_t size, error_code& ec) {
        std::promise<std::tuple<size_t, error_code>> p;

        m_socket.async_read_some(ba::buffer(buf, size), [&p](error_code ec_, size_t n_) {
            p.set_value({n_, ec_});
        });

        size_t result;
        std::tie(result, ec) = p.get_future().get();
        return result;
    }
};

#include <iomanip>
#include <iostream>
int main() {
    ba::thread_pool ioc;

    auto s = std::make_shared<Session>(ioc.get_executor());

    error_code ec;
    CharT data[10];
    while (auto n = s->readSome(data, 10, ec))
        std::cout << "Received " << quoted(std::string(data, n)) << " (" << ec.message() << ")\n";

    ioc.join();
}

Testing with

g++ -std=c++14 -O2 -Wall -pedantic -pthread main.cpp
for resp in FOO LONG_BAR_QUX_RESPONSE; do nc -tln 7878 -w 0 <<< $resp; done&
set -x
sleep .2; ./a.out
sleep .2; ./a.out

Prints

+ sleep .2
+ ./a.out
Received "FOO
" (Success)
+ sleep .2
+ ./a.out
Received "LONG_BAR_Q" (Success)
Received "UX_RESPONS" (Success)
Received "E
" (Success)

External Synchronization (Cancellation?)

Now, code not show implies that other operations may act on m_socket, if at least only to cancel operations in flight³. If this situation arises you have add the missing synchronization, either using the mutex or the strand.

I suggest not introducing the competing synchronization mechanism, even though not "incorrect". It will

• lead to simpler code
• allow you to solidify your understanding of the use of the strand.

So, let's make sure that the operation runs on the strand:

int32_t readSome(CharT* buf, uint32_t size, error_code& ec) {
    std::promise<size_t> p;

    post(m_socket.get_executor(), [&] {
        m_socket.async_read_some(ba::buffer(buf, size),
             [&](error_code ec_, size_t n_) { ec = ec_; p.set_value(n_); });
    });

    return p.get_future().get();
}

void cancel() {
    post(m_socket.get_executor(),
         [self = shared_from_this()] { self->m_socket.cancel(); });
}

See it Live On Coliru

Exercising Cancellation

int main() {
    ba::thread_pool ioc(1);

    auto s = std::make_shared<Session>(ioc.get_executor());

    std::thread th([&] {
        std::this_thread::sleep_for(5s);
        s->cancel();
    });

    error_code ec;
    CharT data[10];
    do {
        auto n = s->readSome(data, 10, ec);
        std::cout << "Received " << quoted(std::string(data, n)) << " (" << ec.message() << ")\n";
    } while (!ec);

    ioc.join();
    th.join();
}

Again, Live On Coliru

---

¹ Technically in a multi-thread situation you need to notify the CV under the lock to allow for fair scheduling, i.e. to prevent waiter starvation. However your scenario is so isolated that you can get away with being somewhat sloppy.

² by default tcp::socket type-erases the executor with any_io_executor, but you could use basic_stream_socket<tcp, strand<io_context::executor_type> > to remove that cost if your executor type is statically known

³ Of course, POSIX sockets include full duplex scenarios, where read and write operations can be in flight simultaneoulsy.

UPDATE: redirect_error

Just re-discovered redirect_error which allows something close to as_tuple:

auto readSome(CharT* buf, uint32_t size, error_code& ec) {
    return m_socket
        .async_read_some(ba::buffer(buf, size),
                         ba::redirect_error(ba::use_future, ec))
        .get();
}

void cancel() { m_socket.cancel(); }

This only suffices when readSome and cancel are guaranteed to be invoked on the strand.