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?
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 fromstd::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?