chriskohlhoff / asio

Asio C++ Library
http://think-async.com/Asio
4.88k stars 1.21k forks source link

move constructor for thread_pool #1084

Open bowie7070 opened 2 years ago

bowie7070 commented 2 years ago

Hi,

I have some code that returns a thread_pool but copy elision is not possible. It seems that thread_pool does not have a move constructor. It is possible to support a move constructor on thread_pool?

It seems that thread_pool loses its move constructor by inheriting from boost::asio::noncopyable (indirectly through boost::asio::execution_context). The asio version of noncopyable uses approach of making the copy constructor/copy assignment private rather than deleting them.

namespace boost {
namespace asio {
namespace detail {

class noncopyable
{
protected:
  noncopyable() {}
  ~noncopyable() {}
private:
  noncopyable(const noncopyable&);
  const noncopyable& operator=(const noncopyable&);
};

} // namespace detail

using boost::asio::detail::noncopyable;

} // namespace asio
} // namespace boost

As I understand it this prevents the compiler from generating the move constructor and move assignment operator.

I see that thread_pool prevents copy construction with:

private:
  thread_pool(const thread_pool&) BOOST_ASIO_DELETED;
  thread_pool& operator=(const thread_pool&) BOOST_ASIO_DELETED;

Would it be appropriate to use this same pattern in execution_context?

bowie7070 commented 2 years ago

Having done some investigation the answer to my question is no, it is not appropriate to allow the compiler to generate move constructor/move assignment for execution_context. A default move constructor for execution_context would result in the service registry member being passed to delete more than once.

To allow move construction for execution_context would changing the definition of service_registry_ from:

asio::detail::service_registry* service_registry_;

to something like:

std::unique_ptr<asio::detail::service_registry> service_registry_;

And then making some of the member functions (shutdown() and destroy()) check for a null pointer and not do anything in that case.

However, it seems that support for older C++ standards is important, is that correct? If so, it would be necessary to change it in a way that is compatible with older standards.

bowie7070 commented 2 years ago

Further investigation indicates the problem is more complicated than I thought. The services held by the service registry get a reference to the execution context that owns them. Once there are some services that know about the execution context it seems there would not be a safe way to move the execution context.