Documentation is not clear if boost::asio::spawn can be used with multiple threads

[johnwason]

I have been testing out boost::asio::spawn that results in a boost::asio::yield_context and it appears that if I have more than one thread in the thread pool for io_service it will eventually result in an access violation. This is being caused by io_service resuming the coroutine from a different thread. From what I have read this is correct behavior, however the documentation on spawn and yield_context do not mention the thread safety problem. It would be very helpful if the documentation discussed this issue in more detail to prevent this misunderstanding.

[chriskohlhoff]

It's supposed to work. Can you supply a test case?

[johnwason]

According to the coroutine documentation coroutines can only be resumed from the same thread that started them. It also states that io_service.run() can only be called from one thread.

http://www.crystalclearsoftware.com/soc/coroutine/coroutine/threads.html

[chriskohlhoff]

That isn't the documentation for the current boost.coroutine library, but a very old and early prototype developed for Google Summer of Code.

[johnwason]

I will try to figure out a way to isolate the problem. I have the equivalent program using handlers that does not have an issue. The one that uses yield_context crashes after around 80,000 repeated calls in a while loop so the only thing I can think of that causes it is the coroutine thread switching. This is the code that I use to adapt the handler function to use yield_context.

virtual void async_pingpong(std::string p,boost::function<void (std::string, RR_SHARED_PTR<RobotRaconteur::RobotRaconteurException>) > rr_handler, int32_t rr_timeout=RR_TIMEOUT_INFINITE);

#ifdef ROBOTRACONTEUR_USE_ASIO_SPAWN
std::string async_pingpong(std::string p,boost::asio::yield_context& rr_yield, int32_t rr_timeout=RR_TIMEOUT_INFINITE)
{
    boost::asio::detail::async_result_init<boost::asio::yield_context, void(boost::tuple<std::string,RR_SHARED_PTR<RobotRaconteur::RobotRaconteurException> > )> init(BOOST_ASIO_MOVE_CAST(boost::asio::yield_context)(rr_yield));
    async_pingpong(p, boost::bind(&boost::asio::handler_type<boost::asio::yield_context, void(boost::tuple<std::string,RR_SHARED_PTR<RobotRaconteur::RobotRaconteurException> >) >::type::operator(),init.handler,boost::bind(&boost::make_tuple<std::string,RR_SHARED_PTR<RobotRaconteur::RobotRaconteurException> >,_1,_2)),rr_timeout);
    boost::tuple<std::string, RR_SHARED_PTR<RobotRaconteur::RobotRaconteurException> > ret=init.result.get();
    if (ret.get<1>()) throw *ret.get<1>();
    return ret.get<0>();
}
#endif

The program itself is used to measure transaction performance. It is possible I messed up the async_result_init block but I would expect that to cause problems immediately.

[chriskohlhoff]

The use of a non-const reference for rr_yield as well as BOOST_ASIO_MOVE_CAST(boost::asio::yield_context)(rr_yield) is highly suspicious and if you're using C++11 likely to lead to a crash. You should either use an rvalue reference (via BOOST_ASIO_MOVE_ARG if you like) in conjunction with the cast, or use a const reference and no cast.

[johnwason]

I changed rr_yield to be a copy arg but am still getting the same access violation at iteration=43363. I will do some digging to see if I can recreate the problem but the software runs fine using handlers.

[johnwason]

Ok, I figured out the problem. If the handler is called before init.result.get() it results in an instant access violation. This is why it is intermittent. Here is a test program that causes the access violation (I used MSVC 2010)

#include <boost/asio.hpp>
#include <boost/asio/spawn.hpp>
#include <boost/bind.hpp>
#include <boost/thread.hpp>

using namespace boost;
using namespace boost::asio;

io_service io1;

void func2(boost::function<void(int32_t)> handler)
{
    io1.post(boost::bind(handler,10));
}

void func(const yield_context& y)
{
    boost::asio::detail::async_result_init<boost::asio::yield_context, void(int32_t )> init(BOOST_ASIO_MOVE_CAST(boost::asio::yield_context)(y));
    func2(boost::bind(&boost::asio::handler_type<boost::asio::yield_context, void( int32_t)>::type::operator(),init.handler,_1));
    this_thread::sleep(posix_time::milliseconds(10000));
    int32_t ret=init.result.get();

}

int main(int argc, char* argv[])
{

    io_service::work w(io1);

    for (int i=0; i<10; i++) thread(bind(&io_service::run,&io1));

    spawn(io1,&func);


    this_thread::sleep(posix_time::milliseconds(100000));

}

[chriskohlhoff]

Aha, yes that's because func2's handler isn't executing in the yield context's strand.

I think you could write the whole thing much cleaner with a small function object adapter. Let me mock something up to illustrate...

[chriskohlhoff]

virtual void async_pingpong(std::string p,
  boost::function<void (std::string, RR_SHARED_PTR<RobotRaconteur::RobotRaconteurException>) > rr_handler,
  int32_t rr_timeout=RR_TIMEOUT_INFINITE);

#ifdef ROBOTRACONTEUR_USE_ASIO_SPAWN
template <typename Handler>
struct pingpong_adapter
{
  Handler h_;

  explicit pingpong_adapter(Handler h) : h_(h) {}

  void operator()(std::string s, RR_SHARED_PTR<RobotRaconteur::RobotRaconteurException> p)
  {
    // This is the key bit you are missing...
    using asio::asio_handler_invoke;
    asio_handler_invoke(boost::bind(h_, boost::make_tuple(s, p)), boost::addressof(h_)); 
  }
};

template <typename Handler>
pingpong_adapter<Handler> make_pingpong_adapter(Handler h)
{ 
  return pingpong_adapter<Handler>(h);
}

std::string async_pingpong(std::string p,boost::asio::yield_context rr_yield, int32_t rr_timeout=RR_TIMEOUT_INFINITE)
{
    boost::asio::detail::async_result_init<boost::asio::yield_context, void(boost::tuple<std::string,RR_SHARED_PTR<RobotRaconteur::RobotRaconteurException> > )> init(rr_yield);
    async_pingpong(p, make_pingpong_adapter(init,handler));
    boost::tuple<std::string, RR_SHARED_PTR<RobotRaconteur::RobotRaconteurException> > ret=init.result.get();
    if (ret.get<1>()) throw *ret.get<1>();
    return ret.get<0>();
}
#endif

[johnwason]

I added the asio_handler_invoke wrapper to my test case but still have the same problem.

#include <boost/asio.hpp>
#include <boost/asio/spawn.hpp>
#include <boost/bind.hpp>
#include <boost/thread.hpp>

using namespace boost;
using namespace boost::asio;

io_service io1;

void func3(boost::function<void(int32_t)> handler, int32_t val)
{
    asio::asio_handler_invoke(boost::bind(handler,val));
}

void func2(boost::function<void(int32_t)> handler)
{
    io1.post(boost::bind(&func3,handler,10));
}

void func(const yield_context& y)
{
    boost::asio::detail::async_result_init<boost::asio::yield_context, void(int32_t )> init(BOOST_ASIO_MOVE_CAST(boost::asio::yield_context)(y));
    func2(boost::bind(&boost::asio::handler_type<boost::asio::yield_context, void( int32_t)>::type::operator(),init.handler,_1));
    this_thread::sleep(posix_time::milliseconds(10000));
    int32_t ret=init.result.get();

}

int main(int argc, char* argv[])
{

    io_service::work w(io1);

    for (int i=0; i<10; i++) thread(bind(&io_service::run,&io1));

    spawn(io1,&func);


    this_thread::sleep(posix_time::milliseconds(100000));

}

[johnwason]

As I think about it using strand to wrap the async handlers won't work because the strand will exit after the async call is started and before the init.result.get is called.

[johnwason]

I don't see a clean way to notify the handler thread that coroutine is ready to be called because any code after result.init.get() will not be called until after the handler has been called. There is no way to notify after the thread has been paused and the context saved (at least that I see). Wrapping the code in a strand won't help because the strand will not be unlocked until after the handler is called.

[chriskohlhoff]

You missed an important detail of my mockup. This won't work for two reasons:

void func3(boost::function<void(int32_t)> handler, int32_t val)
{
    asio::asio_handler_invoke(boost::bind(handler,val));
}

• asio_handler_invoke is located using argument-dependent lookup, therefore you cannot call it using a qualified name, and you must also pass the additional argument of the addressof the handler.
• The hook can't see through the boost::function type anyway because it's a runtime wrapper.

Hence:

template <typename Handler>
struct pingpong_adapter
{
  Handler h_;

  explicit pingpong_adapter(Handler h) : h_(h) {}

  void operator()(std::string s, RR_SHARED_PTR<RobotRaconteur::RobotRaconteurException> p)
  {
    // This is the key bit you are missing...
    using asio::asio_handler_invoke;
    asio_handler_invoke(boost::bind(h_, boost::make_tuple(s, p)), boost::addressof(h_)); 
  }
};

[johnwason]

What should "handler" be in "async_pingpong(p, make_pingpong_adapter(init,handler));"?

[johnwason]

Ok, I realized it should be init.handler. This code does seem to work:

#include <boost/asio.hpp>
#include <boost/asio/spawn.hpp>
#include <boost/bind.hpp>
#include <boost/thread.hpp>

using namespace boost;
using namespace boost::asio;

io_service io1;

template <typename Handler>
struct test_adapter
{
  Handler h_;

  explicit test_adapter(Handler h) : h_(h) {}

  void operator()(int32_t d)
  {
    // This is the key bit you are missing...
    using asio::asio_handler_invoke;
    asio_handler_invoke(boost::bind(boost::function<void(int32_t)>(h_),d), boost::addressof(h_)); 
  }
};

template <typename Handler>
test_adapter<Handler> make_test_adapter(Handler h)
{ 
  return test_adapter<Handler>(h);
}

void func2(boost::function<void(int32_t)> handler)
{
    io1.post(boost::bind(handler,10));
}

void func(const yield_context& y)
{
    boost::asio::detail::async_result_init<boost::asio::yield_context, void(int32_t )> init(BOOST_ASIO_MOVE_CAST(boost::asio::yield_context)(y));
    func2(make_test_adapter(init.handler));
    this_thread::sleep(posix_time::milliseconds(1000));
    int32_t ret=init.result.get();

}

int main(int argc, char* argv[])
{

    io_service::work w(io1);

    for (int i=0; i<10; i++) thread(bind(&io_service::run,&io1));

    spawn(io1,&func);


    this_thread::sleep(posix_time::milliseconds(100000));

}

I think it would be very helpful to have a standard method for handling wrapping a custom operation to use yield_context. I suspect that it is something that is going to be done commonly and it wasn't obvious how to do so correctly.