4
\$\begingroup\$

I've wanted to write a C++11 class for making IPv4 TCP connections and figured that it probably could be done with just one static function using some shared_ptr and lambda "magic". The following code seems to work - but I'm not entirely sure that it's safe and completely bug-free:

void Client::connect(boost::asio::io_service& io_service,
                     const std::string& address,
                     const std::string& port,
                     const Callbacks& callbacks)
{
  using namespace boost::asio::ip;

  std::shared_ptr<tcp::socket> socket = std::make_shared<tcp::socket>(io_service);
  std::shared_ptr<tcp::resolver> resolver = std::make_shared<tcp::resolver>(io_service);
  tcp::resolver::query query(address, port);

  auto connectHandler = [socket, callbacks]
                        (const boost::system::error_code& errorCode, const tcp::resolver::iterator& it)
  {
    if (errorCode) {
      callbacks.onConnectionError("Could not connect: " + errorCode.message());
    } else if (it == tcp::resolver::iterator()) {
      callbacks.onConnectionError("Could not connect!");
    } else {
      callbacks.onConnected(std::move(*socket));
    }
  };

  auto resolveHandler = [socket, resolver, callbacks, connectHandler]
                        (const boost::system::error_code& errorCode, const tcp::resolver::iterator& it)
  {
    if (errorCode) {
      callbacks.onConnectionError("Could not resolve address: " + errorCode.message());
    } else {
      boost::asio::async_connect(*socket, it, tcp::resolver::iterator(), connectHandler);
    }
  };

  resolver->async_resolve(query, resolveHandler);
}

First of all, the Callbacks is just a struct with two callbacks: onConnected(tcp::socket) and onConnectionError(const std::string&).

We'll create a tcp::socket and a tcp::resolver inside shared_ptr since they need to "survive" this whole procedure and can't be copied (atleast not tcp::socket. It can however be moved, but not using C++11 lambdas (?)).

Then we'll just create our two callbacks. resolveHandler needs to capture the socket, callbacks and connectHandler so that those "survive". It also needs to capture the actual resolver, otherwise I got an "Operation canceled"-error. The connectHandler just needs to capture the socket and the callbacks.

If the resolve is successful we'll call async_connect and tell it to try all endpoints. If the connection was successful we'll call our onConnected-callback using std::move and a reference to the socket. If it was not successful we'll call the onConnectionError-callback with an error message.

Does this looks sane? Are there any improvements I could make? Also, I'm wondering if the std::move(*socket) is safe? If I'm not mistaken, this will just move the socket to the onConnected-function call and leave the invalidated socket in the shared_ptr (which is deleted when the lambda returns). Or am I wrong?

\$\endgroup\$
2
\$\begingroup\$

Looks good to me.
Given what is posted there is not much else to comment about.

  • Object life-span: Looks good. (Though I recommend some unit-tests).
  • Naming: Looks good.
  • Style: Looks good. (Though personally I don't yet like '{' on the same line as other stuff).

The only thing I may have done differently (but this is very subjective and I would not expect you to change if you prefer your technique).

using namespace boost::asio::ip;

// I may have used
namespace BIP = boost::asio::ip;

Then prefixed all the boost stuff with BIP::. Just so I don't pollute the namespace.

But as I say your technique is totally valid since you have bound the scope of the using declaration to the function. If I was code reviewing within your company I may have commented but I would not block your merge to mainline over it.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.