face: Implementing close operation and many related fixes in TcpFace and TcpChannel

Change-Id: Ib6b751e80454e149bf94f3867663d5e705cbf4a0
refs: #1250, #1248
diff --git a/daemon/face/face.hpp b/daemon/face/face.hpp
index ed78504..e2aa62d 100644
--- a/daemon/face/face.hpp
+++ b/daemon/face/face.hpp
@@ -27,6 +27,14 @@
 class Face : noncopyable, public enable_shared_from_this<Face>
 {
 public:
+  /**
+   * \brief Face-related error
+   */
+  struct Error : public std::runtime_error
+  {
+    Error(const std::string& what) : std::runtime_error(what) {}
+  };
+
   Face();
   
   virtual
@@ -51,6 +59,15 @@
   /// send a Data
   virtual void
   sendData(const Data& data) = 0;
+
+  /**
+   * \brief Close the face
+   *
+   * This terminates all communication on the face and cause
+   * onFail() method event to be invoked
+   */
+  virtual void
+  close() = 0;
   
   /** \brief Get whether underlying communicate is up
    *  In this base class this property is always true.
diff --git a/daemon/face/stream-face.hpp b/daemon/face/stream-face.hpp
index 529e3ef..65ca82e 100644
--- a/daemon/face/stream-face.hpp
+++ b/daemon/face/stream-face.hpp
@@ -17,8 +17,24 @@
 public:
   typedef T protocol;
 
+  /**
+   * \brief Create instance of StreamFace
+   */
   StreamFace(const shared_ptr<typename protocol::socket>& socket);
 
+  virtual
+  ~StreamFace();
+
+  // from Face
+  virtual void
+  sendInterest(const Interest& interest);
+
+  virtual void
+  sendData(const Data& data);
+
+  virtual void
+  close();
+  
 protected:
   void
   handleSend(const boost::system::error_code& error,
@@ -28,17 +44,25 @@
   handleReceive(const boost::system::error_code& error,
                 std::size_t bytes_recvd);
 
+  void
+  keepFaceAliveUntilAllHandlersExecuted(const shared_ptr<Face>& face);
+  
 protected:
   shared_ptr<typename protocol::socket> m_socket;
-
+  
 private:
   uint8_t m_inputBuffer[MAX_NDN_PACKET_SIZE];
   std::size_t m_inputBufferSize;
 
+#ifdef _DEBUG
+  typename protocol::endpoint m_localEndpoint;
+#endif
+  
   NFD_LOG_INCLASS_DECLARE();
 };
 
-NFD_LOG_INCLASS_TEMPLATE_DEFINE(StreamFace, "StreamFace");
+// All inherited classes must use
+// NFD_LOG_INCLASS_TEMPLATE_SPECIALIZATION_DEFINE(StreamFace, <specialization-parameter>, "Name");
 
 template <class T>
 inline
@@ -49,6 +73,58 @@
                           bind(&StreamFace<T>::handleReceive, this, _1, _2));
 }
 
+template <class T>
+inline
+StreamFace<T>::~StreamFace()
+{
+}
+
+
+template <class T>
+inline void
+StreamFace<T>::sendInterest(const Interest& interest)
+{
+  m_socket->async_send(boost::asio::buffer(interest.wireEncode().wire(),
+                                           interest.wireEncode().size()),
+                       bind(&StreamFace<T>::handleSend, this, _1, interest.wireEncode()));
+  
+  // anything else should be done here?
+}
+
+template <class T>
+inline void
+StreamFace<T>::sendData(const Data& data)
+{
+  m_socket->async_send(boost::asio::buffer(data.wireEncode().wire(),
+                                           data.wireEncode().size()),
+                       bind(&StreamFace<T>::handleSend, this, _1, data.wireEncode()));
+  
+  // anything else should be done here?
+}
+
+template <class T>
+inline void
+StreamFace<T>::close()
+{
+  if (!m_socket->is_open())
+    return;
+  
+  NFD_LOG_INFO("[id:" << this->getId()
+               << ",endpoint:" << m_socket->local_endpoint()
+               << "] Close connection");
+  
+  
+  boost::asio::io_service& io = m_socket->get_io_service();
+  m_socket->close();
+  // after this, handleSend/handleReceive will be called with set error code
+
+  // ensure that if that Face object is alive at least until all pending
+  // methods are dispatched
+  io.post(bind(&StreamFace<T>::keepFaceAliveUntilAllHandlersExecuted,
+               this, this->shared_from_this()));
+
+  onFail("Close connection");
+}
 
 template <class T>
 inline void
@@ -59,14 +135,43 @@
     if (error == boost::system::errc::operation_canceled) // when socket is closed by someone
       return;
 
-    NFD_LOG_WARN("[id:" << this->getId()
-                 << ",endpoint:" << m_socket->local_endpoint()
-                 << "] Send operation failed, closing socket: "
-                 << error.category().message(error.value()));
+    if (!m_socket->is_open())
+      {
+        onFail("Connection closed");
+        return;
+      }
 
-    onFail("Send operation failed, closing socket: " +
-           error.category().message(error.value()));
+    if (error == boost::asio::error::eof)
+      {
+        NFD_LOG_INFO("[id:" << this->getId()
+                     << ",endpoint:" << m_socket->local_endpoint()
+                     << "] Connection closed");
+      }
+    else
+      {
+        NFD_LOG_WARN("[id:" << this->getId()
+                     << ",endpoint:" << m_socket->local_endpoint()
+                     << "] Send operation failed, closing socket: "
+                     << error.category().message(error.value()));
+      }
+
+    boost::asio::io_service& io = m_socket->get_io_service();
     m_socket->close();
+    
+    // ensure that if that Face object is alive at least until all pending
+    // methods are dispatched
+    io.post(bind(&StreamFace<T>::keepFaceAliveUntilAllHandlersExecuted,
+                 this, this->shared_from_this()));
+    
+    if (error == boost::asio::error::eof)
+      {
+        onFail("Connection closed");
+      }
+    else
+      {
+        onFail("Send operation failed, closing socket: " +
+               error.category().message(error.value()));
+      }
     return;
   }
 
@@ -81,25 +186,55 @@
 StreamFace<T>::handleReceive(const boost::system::error_code& error,
                              std::size_t bytes_recvd)
 {
-  NFD_LOG_TRACE("[id:" << this->getId()
-                << ",endpoint:" << m_socket->local_endpoint()
-                << "] Received: " << bytes_recvd << " bytes");
-
   if (error || bytes_recvd == 0) {
     if (error == boost::system::errc::operation_canceled) // when socket is closed by someone
       return;
 
-    NFD_LOG_WARN("[id:" << this->getId()
-                 << ",endpoint:" << m_socket->local_endpoint()
-                 << "] Receive operation failed: "
-                 << error.category().message(error.value()));
+    // this should be unnecessary, but just in case
+    if (!m_socket->is_open())
+      {
+        onFail("Connection closed");
+        return;
+      }
+
+    if (error == boost::asio::error::eof)
+      {
+        NFD_LOG_INFO("[id:" << this->getId()
+                     << ",endpoint:" << m_socket->local_endpoint()
+                     << "] Connection closed");
+      }
+    else
+      {
+        NFD_LOG_WARN("[id:" << this->getId()
+                     << ",endpoint:" << m_socket->local_endpoint()
+                     << "] Receive operation failed: "
+                     << error.category().message(error.value()));
+      }
     
-    onFail("Receive operation failed, closing socket: " +
-           error.category().message(error.value()));
+    boost::asio::io_service& io = m_socket->get_io_service();
     m_socket->close();
+    
+    // ensure that if that Face object is alive at least until all pending
+    // methods are dispatched
+    io.post(bind(&StreamFace<T>::keepFaceAliveUntilAllHandlersExecuted,
+                 this, this->shared_from_this()));
+
+    if (error == boost::asio::error::eof)
+      {
+        onFail("Connection closed");
+      }
+    else
+      {
+        onFail("Receive operation failed, closing socket: " +
+               error.category().message(error.value()));
+      }
     return;
   }
 
+  NFD_LOG_TRACE("[id:" << this->getId()
+                << ",endpoint:" << m_socket->local_endpoint()
+                << "] Received: " << bytes_recvd << " bytes");
+
   m_inputBufferSize += bytes_recvd;
   // do magic
 
@@ -150,8 +285,15 @@
                      << "] Received input is invalid or too large to process, "
                      << "closing down the face");
         
-        onFail("Received input is invalid or too large to process, closing down the face");
+        boost::asio::io_service& io = m_socket->get_io_service();
         m_socket->close();
+    
+        // ensure that if that Face object is alive at least until all pending
+        // methods are dispatched
+        io.post(bind(&StreamFace<T>::keepFaceAliveUntilAllHandlersExecuted,
+                     this, this->shared_from_this()));
+        
+        onFail("Received input is invalid or too large to process, closing down the face");
         return;
       }
   }
@@ -175,6 +317,12 @@
                           bind(&StreamFace<T>::handleReceive, this, _1, _2));
 }
 
+template <class T>
+inline void
+StreamFace<T>::keepFaceAliveUntilAllHandlersExecuted(const shared_ptr<Face>& face)
+{
+}
+
 
 } // namespace nfd
 
diff --git a/daemon/face/tcp-channel.cpp b/daemon/face/tcp-channel.cpp
index 026c338..087a24c 100644
--- a/daemon/face/tcp-channel.cpp
+++ b/daemon/face/tcp-channel.cpp
@@ -111,19 +111,33 @@
                                        onConnectFailed));
 }
 
+size_t
+TcpChannel::size() const
+{
+  return m_channelFaces.size();
+}
 
 void
 TcpChannel::createFace(const shared_ptr<ip::tcp::socket>& socket,
                        const FaceCreatedCallback& onFaceCreated)
 {
-  shared_ptr<TcpFace> face = make_shared<TcpFace>(boost::cref(socket));
-  onFaceCreated(face);
-
   tcp::Endpoint remoteEndpoint = socket->remote_endpoint();
+
+  shared_ptr<TcpFace> face = make_shared<TcpFace>(boost::cref(socket));
+  face->onFail += bind(&TcpChannel::afterFaceFailed, this, remoteEndpoint);
+
+  onFaceCreated(face);
   m_channelFaces[remoteEndpoint] = face;
 }
 
 void
+TcpChannel::afterFaceFailed(tcp::Endpoint &remoteEndpoint)
+{
+  NFD_LOG_DEBUG("afterFaceFailed: " << remoteEndpoint);
+  m_channelFaces.erase(remoteEndpoint);
+}
+
+void
 TcpChannel::handleSuccessfulAccept(const boost::system::error_code& error,
                                    const shared_ptr<boost::asio::ip::tcp::socket>& socket,
                                    const FaceCreatedCallback& onFaceCreated,
diff --git a/daemon/face/tcp-channel.hpp b/daemon/face/tcp-channel.hpp
index 999f9c9..e94747f 100644
--- a/daemon/face/tcp-channel.hpp
+++ b/daemon/face/tcp-channel.hpp
@@ -52,8 +52,11 @@
   /**
    * \brief Enable listening on the local endpoint, accept connections,
    *        and create faces when remote host makes a connection
-   * \param backlog The maximum length of the queue of pending incoming
-   *        connections
+   * \param onFaceCreated  Callback to notify successful creation of the face
+   * \param onAcceptFailed Callback to notify when channel fails (accept call
+   *                       returns an error)
+   * \param backlog        The maximum length of the queue of pending incoming
+   *                       connections
    */
   void
   listen(const FaceCreatedCallback& onFaceCreated,
@@ -85,6 +88,12 @@
           const FaceCreatedCallback& onFaceCreated,
           const ConnectFailedCallback& onConnectFailed,
           const time::Duration& timeout = time::seconds(4));
+
+  /**
+   * \brief Get number of faces in the channel
+   */
+  size_t
+  size() const;  
   
 private:
   void
@@ -92,6 +101,9 @@
              const FaceCreatedCallback& onFaceCreated);
 
   void
+  afterFaceFailed(tcp::Endpoint &endpoint);
+
+  void
   handleSuccessfulAccept(const boost::system::error_code& error,
                          const shared_ptr<boost::asio::ip::tcp::socket>& socket,
                          const FaceCreatedCallback& onFaceCreated,
@@ -118,7 +130,7 @@
                           const FaceCreatedCallback& onFaceCreated,
                           const ConnectFailedCallback& onConnectFailed,
                           const shared_ptr<boost::asio::ip::tcp::resolver>& resolver);
-  
+
 private:
   boost::asio::io_service& m_ioService;
   tcp::Endpoint m_localEndpoint;
diff --git a/daemon/face/tcp-face.cpp b/daemon/face/tcp-face.cpp
index a609725..e867483 100644
--- a/daemon/face/tcp-face.cpp
+++ b/daemon/face/tcp-face.cpp
@@ -8,6 +8,9 @@
 
 namespace nfd {
 
+// The whole purpose of this file is to specialize the logger,
+// otherwise, everything could be put into the header file.
+
 NFD_LOG_INCLASS_TEMPLATE_SPECIALIZATION_DEFINE(StreamFace, TcpFace::protocol, "TcpFace");
 
 TcpFace::TcpFace(const shared_ptr<TcpFace::protocol::socket>& socket)
@@ -15,24 +18,5 @@
 {
 }
 
-void
-TcpFace::sendInterest(const Interest& interest)
-{
-  m_socket->async_send(boost::asio::buffer(interest.wireEncode().wire(),
-                                           interest.wireEncode().size()),
-                       bind(&TcpFace::handleSend, this, _1, interest.wireEncode()));
-
-  // anything else should be done here?
-}
-
-void
-TcpFace::sendData(const Data& data)
-{
-  m_socket->async_send(boost::asio::buffer(data.wireEncode().wire(),
-                                           data.wireEncode().size()),
-                       bind(&TcpFace::handleSend, this, _1, data.wireEncode()));
-
-  // anything else should be done here?
-}
 
 } // namespace nfd
diff --git a/daemon/face/tcp-face.hpp b/daemon/face/tcp-face.hpp
index cf3188c..af45c6c 100644
--- a/daemon/face/tcp-face.hpp
+++ b/daemon/face/tcp-face.hpp
@@ -22,13 +22,6 @@
   typedef boost::asio::ip::tcp protocol;
 
   TcpFace(const shared_ptr<protocol::socket>& socket);
-
-  // from Face
-  virtual void
-  sendInterest(const Interest& interest);
-
-  virtual void
-  sendData(const Data& data);
 };
 
 } // namespace nfd
diff --git a/daemon/mgmt/internal-face.cpp b/daemon/mgmt/internal-face.cpp
index 7af328f..32c03a1 100644
--- a/daemon/mgmt/internal-face.cpp
+++ b/daemon/mgmt/internal-face.cpp
@@ -90,10 +90,16 @@
 void
 InternalFace::sendData(const Data& data)
 {
-
 }
 
 void
+InternalFace::close()
+{
+  throw Error("Internal face cannot be closed");
+}
+
+
+void
 InternalFace::setInterestFilter(const Name& filter,
                                 OnInterest onInterest)
 {
diff --git a/daemon/mgmt/internal-face.hpp b/daemon/mgmt/internal-face.hpp
index 9539a9e..8d5805c 100644
--- a/daemon/mgmt/internal-face.hpp
+++ b/daemon/mgmt/internal-face.hpp
@@ -15,6 +15,13 @@
 class InternalFace : public Face, public AppFace
 {
 public:
+  /**
+   * \brief InternalFace-related error
+   */
+  struct Error : public Face::Error
+  {
+    Error(const std::string& what) : Face::Error(what) {}
+  };
 
   InternalFace();
 
@@ -26,6 +33,9 @@
   virtual void
   sendData(const Data& data);
 
+  virtual void
+  close();
+
   // Methods implementing AppFace interface. Do not invoke from forwarder.
 
   virtual void
diff --git a/tests/face/dummy-face.hpp b/tests/face/dummy-face.hpp
index c7b5d76..e78f4cc 100644
--- a/tests/face/dummy-face.hpp
+++ b/tests/face/dummy-face.hpp
@@ -31,6 +31,11 @@
   sendData(const Data& data)
   {
   }
+
+  virtual void
+  close()
+  {
+  }
 };
 
 } // namespace nfd
diff --git a/tests/face/tcp.cpp b/tests/face/tcp.cpp
index 66c0a69..d64d1ce 100644
--- a/tests/face/tcp.cpp
+++ b/tests/face/tcp.cpp
@@ -40,6 +40,8 @@
       bind(&EndToEndFixture::face1_onReceiveInterest, this, _1);
     m_face1->onReceiveData +=
       bind(&EndToEndFixture::face1_onReceiveData, this, _1);
+    m_face1->onFail += 
+      bind(&EndToEndFixture::face1_onFail, this);
 
     this->afterIo();
   }
@@ -69,6 +71,13 @@
   }
 
   void
+  face1_onFail()
+  {
+    m_face1.reset();
+    this->afterIo();
+  }
+
+  void
   channel2_onFaceCreated(const shared_ptr<TcpFace>& newFace)
   {
     BOOST_CHECK(!static_cast<bool>(m_face2));
@@ -77,6 +86,8 @@
       bind(&EndToEndFixture::face2_onReceiveInterest, this, _1);
     m_face2->onReceiveData +=
       bind(&EndToEndFixture::face2_onReceiveData, this, _1);
+    m_face2->onFail += 
+      bind(&EndToEndFixture::face2_onFail, this);
 
     this->afterIo();
   }
@@ -106,6 +117,13 @@
   }
 
   void
+  face2_onFail()
+  {
+    m_face2.reset();
+    this->afterIo();
+  }
+
+  void
   channel_onFaceCreated(const shared_ptr<TcpFace>& newFace)
   {
     m_faces.push_back(newFace);
@@ -230,7 +248,7 @@
   Scheduler scheduler(m_ioService); // to limit the amount of time the test may take
 
   EventId abortEvent =
-    scheduler.scheduleEvent(time::seconds(1),
+    scheduler.scheduleEvent(time::seconds(4),
                             bind(&EndToEndFixture::abortTestCase, this,
                                  "TcpChannel error: cannot connect or cannot accept connection"));
   
@@ -277,7 +295,7 @@
   
   m_ioRemaining = 4; // 2 connects and 2 accepts
   abortEvent = 
-    scheduler.scheduleEvent(time::seconds(1),
+    scheduler.scheduleEvent(time::seconds(4),
                             bind(&EndToEndFixture::abortTestCase, this,
                                  "TcpChannel error: cannot connect or cannot accept multiple connections"));
 
@@ -290,6 +308,61 @@
   BOOST_CHECK_EQUAL(m_faces.size(), 6);
 }
 
+
+BOOST_FIXTURE_TEST_CASE(FaceClosing, EndToEndFixture)
+{
+  TcpChannelFactory factory(m_ioService);
+  Scheduler scheduler(m_ioService); // to limit the amount of time the test may take
+
+  EventId abortEvent =
+    scheduler.scheduleEvent(time::seconds(4),
+                            bind(&EndToEndFixture::abortTestCase, this,
+                                 "TcpChannel error: cannot connect or cannot accept connection"));
+  
+  shared_ptr<TcpChannel> channel1 = factory.create("127.0.0.1", "20070");
+  shared_ptr<TcpChannel> channel2 = factory.create("127.0.0.1", "20071");
+  
+  channel1->listen(bind(&EndToEndFixture::channel1_onFaceCreated,   this, _1),
+                   bind(&EndToEndFixture::channel1_onConnectFailed, this, _1));
+  
+  channel2->connect("127.0.0.1", "20070",
+                    bind(&EndToEndFixture::channel2_onFaceCreated, this, _1),
+                    bind(&EndToEndFixture::channel2_onConnectFailed, this, _1),
+                    time::milliseconds(100)); // very short timeout
+
+  m_ioRemaining = 2;
+  BOOST_REQUIRE_NO_THROW(m_ioService.run());
+  m_ioService.reset();
+  scheduler.cancelEvent(abortEvent);
+
+  BOOST_CHECK_EQUAL(channel1->size(), 1);
+  BOOST_CHECK_EQUAL(channel2->size(), 1);
+
+  abortEvent =
+    scheduler.scheduleEvent(time::seconds(4),
+                            bind(&EndToEndFixture::abortTestCase, this,
+                                 "FaceClosing error: cannot properly close faces"));
+
+  BOOST_REQUIRE(static_cast<bool>(m_face1));
+  BOOST_CHECK(static_cast<bool>(m_face2));
+
+  m_ioRemaining = 2;
+  // just double check that we are calling the virtual method
+  static_pointer_cast<Face>(m_face1)->close();
+  
+  BOOST_REQUIRE_NO_THROW(m_ioService.run());
+  m_ioService.reset();
+  scheduler.cancelEvent(abortEvent);
+
+  // both faces should get closed
+  BOOST_CHECK(!static_cast<bool>(m_face1));
+  BOOST_CHECK(!static_cast<bool>(m_face2));
+  
+  BOOST_CHECK_EQUAL(channel1->size(), 0);
+  BOOST_CHECK_EQUAL(channel2->size(), 0);
+}
+
+
 BOOST_AUTO_TEST_SUITE_END()
 
 } // namespace nfd
diff --git a/tests/fw/forwarder.cpp b/tests/fw/forwarder.cpp
index 23d8828..27653bc 100644
--- a/tests/fw/forwarder.cpp
+++ b/tests/fw/forwarder.cpp
@@ -32,6 +32,11 @@
     m_ioService.stop();
   }
 
+  virtual void
+  close()
+  {
+  }
+
   void
   receiveInterest(const Interest& interest)
   {
diff --git a/tests/mgmt/internal-face.cpp b/tests/mgmt/internal-face.cpp
index 0c08190..52eaa23 100644
--- a/tests/mgmt/internal-face.cpp
+++ b/tests/mgmt/internal-face.cpp
@@ -96,6 +96,8 @@
   face->put(testData);
 
   BOOST_REQUIRE(didPutData);
+
+  BOOST_CHECK_THROW(face->close(), InternalFace::Error);
 }
 
 BOOST_FIXTURE_TEST_CASE(SendInterestHitEnd, InternalFaceFixture)