Full multithreading safety with strands

[aaronalbers]

In my own ASIO based network libraries I spent days trying to track down a race condition and finally came across the following post:
https://stackoverflow.com/questions/36016135/asioasync-write-and-strand

That post informed me that my use of strands was only protecting the callback and not the invocation of the ASIO object methods. I see that same misuse of strands in websocketpp.

Example:

This was my initial understanding on how to use strands as you only need to wrap the callback:

if (config::enable_multithreading) {
  lib::asio::async_connect(
      tcon->get_raw_socket(),
      iterator,
      tcon->get_strand()->wrap(lib::bind(
          &type::handle_connect,
          this,
          tcon,
          con_timer,
          callback,
          lib::placeholders::_1
      ))
  );
} else {
  lib::asio::async_connect(
      tcon->get_raw_socket(),
      iterator,
      lib::bind(
          &type::handle_connect,
          this,
          tcon,
          con_timer,
          callback,
          lib::placeholders::_1
      )
  );
}

When in reality you also need to protect the call to async_connect() as it calls socket_.async_connect() and socket_.close() under the hood. Therefore in order to be safe we would actually need to do something like:

if (config::enable_multithreading) {
    lib::asio::dispatch(tcon->get_strand()->wrap([this, tcon, iterator, con_timer, callback]{
        lib::asio::async_connect(
            tcon->get_raw_socket(),
            iterator,
            tcon->get_strand()->wrap(lib::bind(
                &type::handle_connect,
                this,
                tcon,
                con_timer,
                callback,
                lib::placeholders::_1
            ))
        );
    }));
} else {
    lib::asio::async_connect(
        tcon->get_raw_socket(),
        iterator,
        lib::bind(
            &type::handle_connect,
            this,
            tcon,
            con_timer,
            callback,
            lib::placeholders::_1
        )
    );
}

This of course would have to be done for all ASIO objects (ie sockets, timers, etc)

[aaronalbers]

Here is a sample application that illustrates the problem (although it is contrived):

#include <iostream>
#include <vector>
#include <thread>
#include <string>
#include <websocketpp/config/asio_no_tls_client.hpp>
#include <websocketpp/client.hpp>

typedef websocketpp::client<websocketpp::config::asio_client> client;

void on_message(websocketpp::connection_hdl, client::message_ptr msg) {
    std::cout << msg->get_payload() << std::endl;
}

int main() {
   const unsigned _count = 4;
   std::vector<std::thread> _threads;
   asio::io_context _io_context;
   asio::io_context::work _work{_io_context};
   
   _threads.reserve(_count);
   for (unsigned n = 0; n != _count; ++n) {
      _threads.emplace_back([&] {
         while (true) {
            try {
               _io_context.run();
               break;
            } catch (...) {}
         }
      });
    }
   
   std::string uri = "ws://echo.websocket.org";
   
   client c;
   c.set_access_channels(websocketpp::log::alevel::all);
   c.clear_access_channels(websocketpp::log::alevel::frame_payload);
   c.set_error_channels(websocketpp::log::elevel::all);
   c.init_asio(&_io_context);
   
   c.set_message_handler(&on_message);
   
   websocketpp::lib::error_code ec;
   client::connection_ptr con = c.get_connection(uri, ec);
   if (ec) {
       std::cout << "could not create connection because: " << ec.message() << std::endl;
       return 0;
   }
   
   while (true) {
    c.connect(con);
   }

    return 0;
}

In the current state of the code you will get tons of:

[2020-03-03 15:10:57] [error] handle_connect error: Timer Expired
[2020-03-03 15:10:57] [error] handle_connect error: Timer Expired
[2020-03-03 15:10:57] [error] handle_connect error: Timer Expired
[2020-03-03 15:10:57] [error] handle_connect error: Timer Expired

Then within a few seconds it will segfault.

This exercises the race condition on:

    if (config::enable_multithreading) {
            m_resolver->async_resolve(
                query,
                tcon->get_strand()->wrap(lib::bind(
                    &type::handle_resolve,
                    this,
                    tcon,
                    dns_timer,
                    cb,
                    lib::placeholders::_1,
                    lib::placeholders::_2
                ))
            );
        } else {
            m_resolver->async_resolve(
                query,
                lib::bind(
                    &type::handle_resolve,
                    this,
                    tcon,
                    dns_timer,
                    cb,
                    lib::placeholders::_1,
                    lib::placeholders::_2
                )
            );
        }

Changing it to the following fixes the problem:

    if (config::enable_multithreading) {
            lib::asio::dispatch(tcon->get_strand()->wrap([this, query, tcon, dns_timer, cb]{
                m_resolver->async_resolve(
                    query,
                    tcon->get_strand()->wrap(lib::bind(
                        &type::handle_resolve,
                        this,
                        tcon,
                        dns_timer,
                        cb,
                        lib::placeholders::_1,
                        lib::placeholders::_2
                    ))
                );
            }));
        } else {
            m_resolver->async_resolve(
                query,
                lib::bind(
                    &type::handle_resolve,
                    this,
                    tcon,
                    dns_timer,
                    cb,
                    lib::placeholders::_1,
                    lib::placeholders::_2
                )
            );
        }

Now it will happily:

[2020-03-03 15:10:57] [error] handle_connect error: Timer Expired
[2020-03-03 15:10:57] [error] handle_connect error: Timer Expired
[2020-03-03 15:10:57] [error] handle_connect error: Timer Expired
[2020-03-03 15:10:57] [error] handle_connect error: Timer Expired

Until it runs out of memory. That could be caused by the rate of connections exceeding the rate of timeouts. Either way it is a separate issue all together.

[SirDifferential]

Could this issue be related to #607?