views:

146

answers:

2

I've got a particularly ornery piece of network code. I'm using asio but that really doesn't matter for this question. I assume there is no way to unbind a socket other than closing it. The problem is that open(), bind(), and listen() can all throw a system_error. So I handled the code with a simple try/catch. The code as written in broken.

using namespace boost::asio;

class Thing
{
public:

   ip::tcp::endpoint m_address;

   ip::tcp::acceptor m_acceptor;

   /// connect should handle all of its exceptions internally.
   bool connect()
   {
      try
      {
         m_acceptor.open( m_address.protocol() );
         m_acceptor.set_option( tcp::acceptor::reuse_address(true) );

         m_acceptor.bind( m_address );
         m_acceptor.listen();

         m_acceptor.async_accept( /*stuff*/ );
      }
      catch( const boost::system::system_error& error )
      {
         assert(acceptor.is_open());
         m_acceptor.close();
         return false;
      }
      return true;
   }

   /// don't call disconnect unless connect previously succeeded.
   void disconnect()
   {
      // other stuff needed to disconnect is ommited
      m_acceptor.close();
   }
};

The error is if the socket fails to connect it will try to close it in the catch block and throw another system_error about closing an acceptor that has never been opened.

One solution is to add an if( acceptor.is_open() ) in the catch block but that tastes wrong. Kinda like mixing C-style error checking with c++ exceptions. If I where to go that route, I may as well use the non-throwing version of open().

boost::system::error_code error;
acceptor.open( address.protocol, error );
if( ! error )
{
    try
    {
       acceptor.set_option( tcp::acceptor::reuse_address(true) );

       acceptor.bind( address );
       acceptor.listen();

       acceptor.async_accept( /*stuff*/ );
    }
    catch( const boost::system::system_error& error )
    {
       assert(acceptor.is_open());
       acceptor.close();
       return false;
    }
}
return !error;

Is there an elegant way to handle these possible exceptions using RAII and try/catch blocks?

Am I just wrong headed in trying to avoid if( error condition ) style error handling when using exceptions?

+1  A: 

With try-catch you can take in account that system_error has an error_code that gives you the real reason. So you can test this error_code on the catch sentence.

To use RAI you will need to do the connect on the constructor and the disconnect on the destructor, but I don't know what is behind the

acceptor.async_accept( /*stuff*/ );

so maybe you need to keep out this part. Thing th;

{
 Connector conn(th); / connect on constructor
 // ... th.async_accept  
 // do some work while connected
}
 // disconnect on destructor

Connector will take care of whether the acceptor is open or not using a specific member variable is_open which is set just after the acceptor.open() succeed.

   Connector::Connector(...)
   : ...
   , is_open(false)
   {
     m_acceptor.open( m_address.protocol() );
     is_open=true;
     m_acceptor.set_option( tcp::acceptor::reuse_address(true) );

     m_acceptor.bind( m_address );
     m_acceptor.listen();

     m_acceptor.async_accept( /*stuff*/ );
   }

   Connector::~Connector(...)
   {
     // other stuff needed to disconnect is omitted
     if (is_open) m_acceptor.close();
   }
Vicente Botet Escriba
This is essentially moving the `if(is_open)` to `~Connector` instead of leaving it in the `connect()` function. The one advantage is it avoids code duplication if I need to open an acceptor elsewhere. In my particular situation I do not.
caspin
Well, the code has no try-catch block. You don't find that this is an advantage? You requested how RAI can simplify your code, Does may proposal make simpler?
Vicente Botet Escriba
+3  A: 

I would suggest just doing separate error handling for open, since there is different cleanup before and after:

bool connect()
{
  try {
     m_acceptor.open( m_address.protocol() );
  } catch( const boost::system::system_error& error ) {
     return false;
  }

  try {
     m_acceptor.set_option( tcp::acceptor::reuse_address(true) );

     m_acceptor.bind( m_address );
     m_acceptor.listen();

     m_acceptor.async_accept( /*stuff*/ );
  } catch( const boost::system::system_error& error ) {
     m_acceptor.close();
     return false;
  }
  return true;
}
DS
This includes two try-catch blocks which is expensive and is not in line with the RAI C++ idiom.
Vicente Botet Escriba