c++c++11boostboost-asio

Any better way to improve the code to keep the underlying memory blocks used by aiso::async_send remains valid


I once wrote the the code snippet below to asynchronously send tcp data.

void Conversation::do_write(std::string str)
{
  auto conversation_ptr(shared_from_this());
  m_socket.async_send(asio::buffer(str), [this, conversation_ptr](std::error_code error, size_t length ){
    if (!error)
    {
        std::cout << "data write successfully" << std::endl;
    }
    else
    {
        std::cout << "error in writing data: " << error.message() << std::endl;
    }
  });
}

Later, I found there is something wrong with the code above though nobody pointed it out before. As per the documentation, which says that the underlying memory blocks are retained by the caller, which must guarantee that they remain valid until the handler is called.

basic_stream_socket::async_send (1 of 2 overloads)

Start an asynchronous send.

 template<
     typename ConstBufferSequence,
     typename WriteToken = default_completion_token_t<executor_type>>
 DEDUCED async_send(
     const ConstBufferSequence & buffers,
     WriteToken && token = default_completion_token_t< executor_type >());

This function is used to asynchronously send data on the stream socket. It is an initiating function for an asynchronous operation, and always returns immediately.

Parameters

  • buffers

    One or more data buffers to be sent on the socket. Although the buffers object may be copied as necessary, ownership of the underlying memory blocks is retained by the caller, which must guarantee that they remain valid until the completion handler is called.

To keep the context alive, I rewrite the code as blow.

//Note: the passed string would be modified to avoid copying
void Conversation::do_write(std::string& str)  //modification1: using the reference
{
    auto conversation_ptr(shared_from_this());
    //modification2 using shared pointer to keep the content to be sent alive when the handler is called
    std::shared_ptr<std::string> content_ptr=std::make_shared<std::string>(std::move(str));
    m_socket.async_send(asio::buffer(*content_ptr), [this, conversation_ptr, content_ptr](std::error_code error, size_t length ){
    if (!error)
    {
        std::cout << "data write successfully" << std::endl;
    }
    else
    {
        std::cout << "error in writing data: " << error.message() << std::endl;
    }
  });
}

Is there any better way? How do I improve the code above?


Solution

  • What you did works. However, I'd take str by value, since you're "sinking" its data. Let the caller decide whether the side-effect of clearing the argument's value is desired.

    std::string message = "Hello\n";
    psession1->do_write(message); // copy
    psession2->do_write(std::move(message)); // move
    

    Next up, it's more customary to make the data member of the class, since the class that contains the socket already has to guarantee the correct life-time anyways.

    Your Conversation class may already inherit from std::enable_shared_from_this for the purpose of guaranteeing the lifetime, no need to duplicate the effort for any extra state

    To top it all off: in many situations you have to watch for concurrent write requests anyways, and you will have a member like std::deque<std::string> instead to keep track of pending writes.

    That way you ensure reference stability of the buffers involved, can avoid overlapping write operations and unnecessary dynamic allocation and reference-counting.

    For examples of this you can scan any of my Asio answers for such queues, just picking the first that I see: How to safely write to a socket from multiple threads?