tcp: Deprecated on_error. on_connect may now be invoked with nullptr on failed (outgoing) connection

Author: AndreasAakesson

api/net/tcp/connection.hpp

@@ -65,12 +65,13 @@ class Connection : public std::enable_shared_from_this<Connection> {
   using WriteBuffer = Write_queue::WriteBuffer;
 
 public:
-  /** Called with the connection itself when it's been established. */
+  /** Called with the connection itself when it's been established. May be a nullptr if the connection failed. */
   using ConnectCallback         = delegate<void(Connection_ptr self)>;
   /**
    * @brief      Event when a connection has been established.
    *             This event lets you know when to start using the connection,
    *             and should always be assigned.
+   *             NOTE: The Connection_ptr will be a nullptr when an outgoing connection failed.
    *
    * @param[in]  callback  The callback
    *
@@ -137,18 +138,6 @@ class Connection : public std::enable_shared_from_this<Connection> {
    */
   inline Connection&            on_write(WriteCallback callback);
 
-  /** Called with the error encountered. */
-  using ErrorCallback           = delegate<void(const TCPException& err)>;
-  /**
-   * @brief      Event when a connection has experienced an error of any kind.
-   *             Pretty useless in it's current form, and only useful for printing.
-   *
-   * @param[in]  callback  The callback
-   *
-   * @return     This connection
-   */
-  inline Connection&            on_error(ErrorCallback callback);
-
   /** Called with the packet that got dropped and the reason why. */
   using PacketDroppedCallback   = delegate<void(const Packet&, Drop_reason)>;
   /**
@@ -231,7 +220,10 @@ class Connection : public std::enable_shared_from_this<Connection> {
      */
     Stream(Connection_ptr conn)
       : tcp{std::move(conn)}
-    {}
+    {
+      // stream for a nullptr makes no sense
+      Expects(tcp != nullptr);
+    }
 
     /**
      * @brief      Event when the stream is connected/established/ready to use.
@@ -788,6 +780,7 @@ class Connection : public std::enable_shared_from_this<Connection> {
   /**
    * @brief      Open the connection.
    *             Active determines whether the connection is active or passive.
+   *             May throw if no remote host, or state isnt valid for opening.
    *
    * @param[in]  active  Whether its an active (outgoing) or passive (listening)
    */
@@ -843,7 +836,6 @@ class Connection : public std::enable_shared_from_this<Connection> {
   /** Callbacks */
   ConnectCallback         on_connect_;
   DisconnectCallback      on_disconnect_;
-  ErrorCallback           on_error_;
   PacketDroppedCallback   on_packet_dropped_;
   RtxTimeoutCallback      on_rtx_timeout_;
   CloseCallback           on_close_;
@@ -998,15 +990,15 @@ class Connection : public std::enable_shared_from_this<Connection> {
   /*
     Invoke/signal the diffrent TCP events.
   */
-  void signal_connect()
-  { if(on_connect_) on_connect_(shared_from_this()); }
+  void signal_connect(const bool success = true)
+  {
+    if(on_connect_)
+      (success) ? on_connect_(shared_from_this()) : on_connect_(nullptr);
+  }
 
   void signal_disconnect(Disconnect::Reason&& reason)
   { on_disconnect_(shared_from_this(), Disconnect{reason}); }
 
-  void signal_error(TCPException error)
-  { if(on_error_) on_error_(std::forward<TCPException>(error)); }
-
   void signal_packet_dropped(const Packet& packet, Drop_reason reason)
   { if(on_packet_dropped_) on_packet_dropped_(packet, reason); }
 

api/net/tcp/connection.inc

@@ -24,11 +24,6 @@ inline Connection& Connection::on_write(WriteCallback cb) {
   return *this;
 }
 
-inline Connection& Connection::on_error(ErrorCallback callback) {
-  on_error_ = callback;
-  return *this;
-}
-
 inline Connection& Connection::on_packet_dropped(PacketDroppedCallback callback) {
   on_packet_dropped_ = callback;
   return *this;

src/net/tcp/connection.cpp

@@ -82,7 +82,6 @@ void Connection::reset_callbacks()
   on_disconnect_ = {this, &Connection::default_on_disconnect};
   on_connect_.reset();
   writeq.on_write(nullptr);
-  on_error_.reset();
   on_packet_dropped_.reset();
   on_rtx_timeout_.reset();
   on_close_.reset();
@@ -246,16 +245,10 @@ void Connection::writeq_reset() {
   rtx_timer.stop();
 }
 
-void Connection::open(bool active) {
-  try {
-    debug("<TCP::Connection::open> Trying to open Connection...\n");
-    state_->open(*this, active);
-  }
-  // No remote host, or state isnt valid for opening.
-  catch (const TCPException& e) {
-    debug("<TCP::Connection::open> Cannot open Connection. \n");
-    signal_error(e);
-  }
+void Connection::open(bool active)
+{
+  debug("<TCP::Connection::open> Trying to open Connection...\n");
+  state_->open(*this, active);
 }
 
 void Connection::close() {
@@ -269,8 +262,9 @@ void Connection::close() {
     if(is_state(Closed::instance()))
       signal_close();
   }
-  catch(const TCPException& err) {
-    signal_error(err);
+  catch(const TCPException&) {
+    // just ignore for now, it's kinda stupid its even throwing (i think)
+    // early return is_closing will probably prevent this from happening
   }
 }
 
@@ -883,7 +877,6 @@ void Connection::clean_up() {
 
   on_connect_.reset();
   on_disconnect_.reset();
-  on_error_.reset();
   on_packet_dropped_.reset();
   on_rtx_timeout_.reset();
   on_close_.reset();

src/net/tcp/connection_states.cpp

@@ -890,7 +890,7 @@ State::Result Connection::SynSent::handle(Connection& tcp, Packet_ptr in) {
   // 2. check RST
   if(UNLIKELY(in->isset(RST))) {
     if(in->isset(ACK)) {
-      tcp.signal_error(TCPException{"Connection reset."});
+      tcp.signal_connect(false);
       tcp.drop(*in, Drop_reason::RST);
       return CLOSED;
     } else {

test/net/integration/tcp/service.cpp

@@ -80,17 +80,16 @@ void OUTGOING_TEST_INTERNET(const HostAddress& address) {
     [port](auto ip_address) {
       CHECK(ip_address != 0, "Resolved host");
 
-      if(ip_address != 0) {
+      if(ip_address != 0)
+      {
         Inet4::stack<0>().tcp().connect({ip_address, port})
-          ->on_connect([](tcp::Connection_ptr conn) {
-              CHECK(true, "Connected");
-              conn->on_read(1024, [](tcp::buffer_t, size_t n) {
-                  CHECK(n > 0, "Received a response");
-                });
-            })
-          .on_error([](tcp::TCPException err) {
-              CHECK(false, "Error occured: %s", err.what());
-            });
+          ->on_connect([](tcp::Connection_ptr conn)
+          {
+            CHECKSERT(conn != nullptr, "Connected");
+            conn->on_read(1024, [](tcp::buffer_t, size_t n) {
+                CHECK(n > 0, "Received a response");
+              });
+          });
       }
     });
 }
@@ -102,6 +101,7 @@ void OUTGOING_TEST(tcp::Socket outgoing) {
   INFO("TEST", "Outgoing Connection (%s)", outgoing.to_string().c_str());
   Inet4::stack<0>().tcp().connect(outgoing, [](tcp::Connection_ptr conn)
   {
+    CHECKSERT(conn != nullptr, "Connection successfully established.");
     conn->on_read(small.size(), [](tcp::buffer_t buffer, size_t n)
     {
       CHECKSERT(std::string((char*)buffer.get(), n) == small, "Received SMALL");