face+transport: minor code cleanup
Change-Id: Ibe7d4b7fafdc4848ae9b6ae1e0db7e8d79ea1015
diff --git a/ndn-cxx/face.cpp b/ndn-cxx/face.cpp
index 53ed411..32aa630 100644
--- a/ndn-cxx/face.cpp
+++ b/ndn-cxx/face.cpp
@@ -24,7 +24,6 @@
#include "ndn-cxx/impl/face-impl.hpp"
#include "ndn-cxx/net/face-uri.hpp"
#include "ndn-cxx/security/signing-helpers.hpp"
-#include "ndn-cxx/util/random.hpp"
#include "ndn-cxx/util/time.hpp"
// NDN_LOG_INIT(ndn.Face) is declared in face-impl.hpp
@@ -59,7 +58,6 @@
: m_internalIoService(make_unique<boost::asio::io_service>())
, m_ioService(*m_internalIoService)
, m_internalKeyChain(make_unique<KeyChain>())
- , m_impl(make_shared<Impl>(*this))
{
construct(std::move(transport), *m_internalKeyChain);
}
@@ -67,7 +65,6 @@
Face::Face(boost::asio::io_service& ioService)
: m_ioService(ioService)
, m_internalKeyChain(make_unique<KeyChain>())
- , m_impl(make_shared<Impl>(*this))
{
construct(nullptr, *m_internalKeyChain);
}
@@ -76,7 +73,6 @@
: m_internalIoService(make_unique<boost::asio::io_service>())
, m_ioService(*m_internalIoService)
, m_internalKeyChain(make_unique<KeyChain>())
- , m_impl(make_shared<Impl>(*this))
{
construct(make_shared<TcpTransport>(host, port), *m_internalKeyChain);
}
@@ -84,7 +80,6 @@
Face::Face(shared_ptr<Transport> transport, KeyChain& keyChain)
: m_internalIoService(make_unique<boost::asio::io_service>())
, m_ioService(*m_internalIoService)
- , m_impl(make_shared<Impl>(*this))
{
construct(std::move(transport), keyChain);
}
@@ -92,14 +87,12 @@
Face::Face(shared_ptr<Transport> transport, boost::asio::io_service& ioService)
: m_ioService(ioService)
, m_internalKeyChain(make_unique<KeyChain>())
- , m_impl(make_shared<Impl>(*this))
{
construct(std::move(transport), *m_internalKeyChain);
}
Face::Face(shared_ptr<Transport> transport, boost::asio::io_service& ioService, KeyChain& keyChain)
: m_ioService(ioService)
- , m_impl(make_shared<Impl>(*this))
{
construct(std::move(transport), keyChain);
}
@@ -152,14 +145,15 @@
void
Face::construct(shared_ptr<Transport> transport, KeyChain& keyChain)
{
+ BOOST_ASSERT(m_impl == nullptr);
+ m_impl = make_shared<Impl>(*this, keyChain);
+
if (transport == nullptr) {
transport = makeDefaultTransport();
+ BOOST_ASSERT(transport != nullptr);
}
- BOOST_ASSERT(transport != nullptr);
m_transport = std::move(transport);
- m_nfdController = make_unique<nfd::Controller>(*this, keyChain);
-
IO_CAPTURE_WEAK_IMPL(post) {
impl->ensureConnected(false);
} IO_CAPTURE_WEAK_IMPL_END
@@ -167,12 +161,6 @@
Face::~Face() = default;
-shared_ptr<Transport>
-Face::getTransport()
-{
- return m_transport;
-}
-
PendingInterestHandle
Face::expressInterest(const Interest& interest,
const DataCallback& afterSatisfied,
@@ -326,9 +314,7 @@
m_ioService.run();
}
catch (...) {
- m_impl->m_ioServiceWork.reset();
- m_impl->m_pendingInterestTable.clear();
- m_impl->m_registeredPrefixTable.clear();
+ m_impl->shutdown();
throw;
}
}
@@ -337,13 +323,9 @@
Face::shutdown()
{
IO_CAPTURE_WEAK_IMPL(post) {
- impl->m_pendingInterestTable.clear();
- impl->m_registeredPrefixTable.clear();
-
+ impl->shutdown();
if (m_transport->isConnected())
m_transport->close();
-
- impl->m_ioServiceWork.reset();
} IO_CAPTURE_WEAK_IMPL_END
}
diff --git a/ndn-cxx/face.hpp b/ndn-cxx/face.hpp
index 6ee8949..f1b32a9 100644
--- a/ndn-cxx/face.hpp
+++ b/ndn-cxx/face.hpp
@@ -43,10 +43,6 @@
class InterestFilterId;
class InterestFilterHandle;
-namespace nfd {
-class Controller;
-} // namespace nfd
-
/**
* @brief Callback invoked when expressed Interest gets satisfied with a Data packet
*/
@@ -461,7 +457,7 @@
shutdown();
/**
- * @return reference to io_service object
+ * @brief Returns a reference to the io_service used by this face.
*/
boost::asio::io_service&
getIoService()
@@ -471,10 +467,13 @@
NDN_CXX_PUBLIC_WITH_TESTS_ELSE_PROTECTED:
/**
- * @return underlying transport
+ * @brief Returns the underlying transport.
*/
shared_ptr<Transport>
- getTransport();
+ getTransport() const
+ {
+ return m_transport;
+ }
protected:
virtual void
@@ -509,7 +508,7 @@
const UnregisterPrefixFailureCallback& onFailure);
private:
- /// the io_service owned by this Face, could be null
+ /// the io_service owned by this Face, may be null
unique_ptr<boost::asio::io_service> m_internalIoService;
/// the io_service used by this Face
boost::asio::io_service& m_ioService;
@@ -517,16 +516,14 @@
shared_ptr<Transport> m_transport;
/**
- * @brief if not null, a pointer to an internal KeyChain owned by Face
- * @note if a KeyChain is supplied to constructor, this pointer will be null,
- * and the passed KeyChain is given to nfdController;
- * currently Face does not keep the KeyChain passed in constructor
- * because it's not needed, but this may change in the future
+ * @brief If not null, a pointer to an internal KeyChain owned by this Face.
+ * @note If a KeyChain is supplied to constructor, this pointer will be null,
+ * and the supplied KeyChain is passed to Face::Impl constructor;
+ * currently Face does not keep a ref to the provided KeyChain
+ * because it's not needed, but this may change in the future.
*/
unique_ptr<KeyChain> m_internalKeyChain;
- unique_ptr<nfd::Controller> m_nfdController;
-
class Impl;
shared_ptr<Impl> m_impl;
diff --git a/ndn-cxx/impl/face-impl.hpp b/ndn-cxx/impl/face-impl.hpp
index 4a9c56a..2e98467 100644
--- a/ndn-cxx/impl/face-impl.hpp
+++ b/ndn-cxx/impl/face-impl.hpp
@@ -43,8 +43,8 @@
// DEBUG level: packet logging.
// Each log entry starts with a direction symbol ('<' denotes an outgoing packet, '>' denotes an
// incoming packet) and a packet type symbol ('I' denotes an Interest, 'D' denotes a Data, 'N'
-// denotes a Nack). Interest is printed as its string representation, Data is printed as name only,
-// Nack is printed as the Interest followed by the Nack reason and delimited by a '~' symbol. A
+// denotes a Nack). Interest is printed in its URI string representation, Data is printed as name
+// only, Nack is printed as the Interest followed by the Nack reason separated by a '~' symbol. A
// log line about an incoming packet may be followed by zero or more lines about Interest matching
// InterestFilter, Data satisfying Interest, or Nack rejecting Interest, which are also written at
// DEBUG level.
@@ -62,10 +62,10 @@
using InterestFilterTable = RecordContainer<InterestFilterRecord>;
using RegisteredPrefixTable = RecordContainer<RegisteredPrefix>;
- explicit
- Impl(Face& face)
+ Impl(Face& face, KeyChain& keyChain)
: m_face(face)
, m_scheduler(m_face.getIoService())
+ , m_nfdController(m_face, keyChain)
{
auto postOnEmptyPitOrNoRegisteredPrefixes = [this] {
this->m_face.getIoService().post([this] { this->onEmptyPitOrNoRegisteredPrefixes(); });
@@ -137,6 +137,7 @@
return true;
});
+
// if Data matches no pending Interest record, it is sent to the forwarder as unsolicited Data
return hasForwarderMatch || !hasAppMatch;
}
@@ -166,6 +167,7 @@
}
return true;
});
+
// send "least severe" Nack from any PendingInterest record originated from forwarder, because
// it is unimportant to consider Nack reason for the unlikely case when forwarder sends multiple
// Interests to an app in a short while
@@ -262,7 +264,7 @@
NDN_LOG_INFO("registering prefix: " << prefix);
auto id = m_registeredPrefixTable.allocateId();
- m_face.m_nfdController->start<nfd::RibRegisterCommand>(
+ m_nfdController.start<nfd::RibRegisterCommand>(
nfd::ControlParameters().setName(prefix).setFlags(flags),
[=] (const nfd::ControlParameters&) {
NDN_LOG_INFO("registered prefix: " << prefix);
@@ -307,7 +309,8 @@
}
NDN_LOG_INFO("unregistering prefix: " << record->getPrefix());
- m_face.m_nfdController->start<nfd::RibUnregisterCommand>(
+
+ m_nfdController.start<nfd::RibUnregisterCommand>(
nfd::ControlParameters().setName(record->getPrefix()),
[=] (const nfd::ControlParameters&) {
NDN_LOG_INFO("unregistered prefix: " << record->getPrefix());
@@ -328,9 +331,10 @@
void
ensureConnected(bool wantResume)
{
- if (!m_face.m_transport->isConnected())
- m_face.m_transport->connect(m_face.m_ioService,
- [=] (const Block& wire) { m_face.onReceiveElement(wire); });
+ if (!m_face.m_transport->isConnected()) {
+ m_face.m_transport->connect(m_face.getIoService(),
+ [this] (const Block& wire) { m_face.onReceiveElement(wire); });
+ }
if (wantResume && !m_face.m_transport->isReceiving()) {
m_face.m_transport->resume();
@@ -348,6 +352,14 @@
}
}
+ void
+ shutdown()
+ {
+ m_ioServiceWork.reset();
+ m_pendingInterestTable.clear();
+ m_registeredPrefixTable.clear();
+ }
+
private:
/** @brief Finish packet encoding
* @param lpPacket NDNLP packet without FragmentField
@@ -376,6 +388,7 @@
Face& m_face;
Scheduler m_scheduler;
scheduler::ScopedEventId m_processEventsTimeoutEvent;
+ nfd::Controller m_nfdController;
PendingInterestTable m_pendingInterestTable;
InterestFilterTable m_interestFilterTable;
diff --git a/ndn-cxx/transport/detail/stream-transport-impl.hpp b/ndn-cxx/transport/detail/stream-transport-impl.hpp
index a90143a..97b796e 100644
--- a/ndn-cxx/transport/detail/stream-transport-impl.hpp
+++ b/ndn-cxx/transport/detail/stream-transport-impl.hpp
@@ -41,15 +41,13 @@
class StreamTransportImpl : public std::enable_shared_from_this<StreamTransportImpl<BaseTransport, Protocol>>
{
public:
- typedef StreamTransportImpl<BaseTransport, Protocol> Impl;
- typedef std::list<Block> BlockSequence;
- typedef std::list<BlockSequence> TransmissionQueue;
+ using Impl = StreamTransportImpl<BaseTransport, Protocol>;
+ using BlockSequence = std::list<Block>;
+ using TransmissionQueue = std::list<BlockSequence>;
StreamTransportImpl(BaseTransport& transport, boost::asio::io_service& ioService)
: m_transport(transport)
, m_socket(ioService)
- , m_inputBufferSize(0)
- , m_isConnecting(false)
, m_connectTimer(ioService)
{
}
@@ -57,21 +55,22 @@
void
connect(const typename Protocol::endpoint& endpoint)
{
- if (!m_isConnecting) {
- m_isConnecting = true;
-
- // Wait at most 4 seconds to connect
- /// @todo Decide whether this number should be configurable
- m_connectTimer.expires_from_now(std::chrono::seconds(4));
- m_connectTimer.async_wait([self = this->shared_from_this()] (const auto& error) {
- self->connectTimeoutHandler(error);
- });
-
- m_socket.open();
- m_socket.async_connect(endpoint, [self = this->shared_from_this()] (const auto& error) {
- self->connectHandler(error);
- });
+ if (m_isConnecting) {
+ return;
}
+ m_isConnecting = true;
+
+ // Wait at most 4 seconds to connect
+ /// @todo Decide whether this number should be configurable
+ m_connectTimer.expires_from_now(std::chrono::seconds(4));
+ m_connectTimer.async_wait([self = this->shared_from_this()] (const auto& error) {
+ self->connectTimeoutHandler(error);
+ });
+
+ m_socket.open();
+ m_socket.async_connect(endpoint, [self = this->shared_from_this()] (const auto& error) {
+ self->connectHandler(error);
+ });
}
void
@@ -138,19 +137,18 @@
m_isConnecting = false;
m_connectTimer.cancel();
- if (!error) {
- m_transport.m_isConnected = true;
-
- if (!m_transmissionQueue.empty()) {
- resume();
- asyncWrite();
- }
- }
- else {
+ if (error) {
m_transport.m_isConnected = false;
m_transport.close();
NDN_THROW(Transport::Error(error, "error while connecting to the forwarder"));
}
+
+ m_transport.m_isConnected = true;
+
+ if (!m_transmissionQueue.empty()) {
+ resume();
+ asyncWrite();
+ }
}
void
@@ -181,7 +179,8 @@
{
BOOST_ASSERT(!m_transmissionQueue.empty());
boost::asio::async_write(m_socket, m_transmissionQueue.front(),
- bind(&Impl::handleAsyncWrite, this->shared_from_this(), _1, m_transmissionQueue.begin()));
+ bind(&Impl::handleAsyncWrite, this->shared_from_this(), _1,
+ m_transmissionQueue.begin()));
}
void
@@ -192,9 +191,8 @@
// async receive has been explicitly cancelled (e.g., socket close)
return;
}
-
m_transport.close();
- NDN_THROW(Transport::Error(error, "error while sending data to socket"));
+ NDN_THROW(Transport::Error(error, "error while writing data to socket"));
}
if (!m_transport.m_isConnected) {
@@ -224,7 +222,6 @@
// async receive has been explicitly cancelled (e.g., socket close)
return;
}
-
m_transport.close();
NDN_THROW(Transport::Error(error, "error while receiving data from socket"));
}
@@ -236,8 +233,7 @@
bool hasProcessedSome = processAllReceived(m_inputBuffer, offset, m_inputBufferSize);
if (!hasProcessedSome && m_inputBufferSize == MAX_NDN_PACKET_SIZE && offset == 0) {
m_transport.close();
- NDN_THROW(Transport::Error(boost::system::error_code(),
- "input buffer full, but a valid TLV cannot be decoded"));
+ NDN_THROW(Transport::Error("input buffer full, but a valid TLV cannot be decoded"));
}
if (offset > 0) {
@@ -263,7 +259,7 @@
if (!isOk)
return false;
- m_transport.receive(element);
+ m_transport.m_receiveCallback(element);
offset += element.size();
}
return true;
@@ -274,12 +270,11 @@
typename Protocol::socket m_socket;
uint8_t m_inputBuffer[MAX_NDN_PACKET_SIZE];
- size_t m_inputBufferSize;
+ size_t m_inputBufferSize = 0;
TransmissionQueue m_transmissionQueue;
- bool m_isConnecting;
-
boost::asio::steady_timer m_connectTimer;
+ bool m_isConnecting = false;
};
} // namespace detail
diff --git a/ndn-cxx/transport/tcp-transport.cpp b/ndn-cxx/transport/tcp-transport.cpp
index 0b78b19..512f5e1 100644
--- a/ndn-cxx/transport/tcp-transport.cpp
+++ b/ndn-cxx/transport/tcp-transport.cpp
@@ -78,15 +78,13 @@
}
void
-TcpTransport::connect(boost::asio::io_service& ioService,
- const ReceiveCallback& receiveCallback)
+TcpTransport::connect(boost::asio::io_service& ioService, ReceiveCallback receiveCallback)
{
NDN_LOG_DEBUG("connect host=" << m_host << " port=" << m_port);
if (m_impl == nullptr) {
- Transport::connect(ioService, receiveCallback);
-
- m_impl = make_shared<Impl>(ref(*this), ref(ioService));
+ Transport::connect(ioService, std::move(receiveCallback));
+ m_impl = make_shared<Impl>(*this, ioService);
}
boost::asio::ip::tcp::resolver::query query(m_host, m_port);
diff --git a/ndn-cxx/transport/tcp-transport.hpp b/ndn-cxx/transport/tcp-transport.hpp
index e3bb5a3..a727f27 100644
--- a/ndn-cxx/transport/tcp-transport.hpp
+++ b/ndn-cxx/transport/tcp-transport.hpp
@@ -1,6 +1,6 @@
/* -*- Mode:C++; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */
/*
- * Copyright (c) 2013-2018 Regents of the University of California.
+ * Copyright (c) 2013-2019 Regents of the University of California.
*
* This file is part of ndn-cxx library (NDN C++ library with eXperimental eXtensions).
*
@@ -50,7 +50,7 @@
~TcpTransport() override;
void
- connect(boost::asio::io_service& ioService, const ReceiveCallback& receiveCallback) override;
+ connect(boost::asio::io_service& ioService, ReceiveCallback receiveCallback) override;
void
close() override;
diff --git a/ndn-cxx/transport/transport.cpp b/ndn-cxx/transport/transport.cpp
index 23895b0..a78e36f 100644
--- a/ndn-cxx/transport/transport.cpp
+++ b/ndn-cxx/transport/transport.cpp
@@ -24,25 +24,17 @@
namespace ndn {
Transport::Error::Error(const boost::system::error_code& code, const std::string& msg)
- : std::runtime_error(msg + (code.value() ? " (" + code.category().message(code.value()) + ")" : ""))
-{
-}
-
-Transport::Transport()
- : m_ioService(nullptr)
- , m_isConnected(false)
- , m_isReceiving(false)
+ : std::runtime_error(msg + (code.value() ? " (" + code.message() + ")" : ""))
{
}
void
-Transport::connect(boost::asio::io_service& ioService,
- const ReceiveCallback& receiveCallback)
+Transport::connect(boost::asio::io_service& ioService, ReceiveCallback receiveCallback)
{
BOOST_ASSERT(receiveCallback != nullptr);
m_ioService = &ioService;
- m_receiveCallback = receiveCallback;
+ m_receiveCallback = std::move(receiveCallback);
}
} // namespace ndn
diff --git a/ndn-cxx/transport/transport.hpp b/ndn-cxx/transport/transport.hpp
index 6954686..4f4d72b 100644
--- a/ndn-cxx/transport/transport.hpp
+++ b/ndn-cxx/transport/transport.hpp
@@ -30,7 +30,7 @@
namespace ndn {
-/** \brief provides TLV-block delivery service
+/** \brief Provides TLV-block delivery service.
*/
class Transport : noncopyable
{
@@ -43,21 +43,19 @@
Error(const boost::system::error_code& code, const std::string& msg);
};
- typedef function<void(const Block& wire)> ReceiveCallback;
- typedef function<void()> ErrorCallback;
-
- Transport();
+ using ReceiveCallback = std::function<void(const Block& wire)>;
+ using ErrorCallback = std::function<void()>;
virtual
~Transport() = default;
- /** \brief asynchronously open the connection
+ /** \brief Asynchronously open the connection.
* \param ioService io_service to create socket on
* \param receiveCallback callback function when a TLV block is received; must not be empty
* \throw boost::system::system_error connection cannot be established
*/
virtual void
- connect(boost::asio::io_service& ioService, const ReceiveCallback& receiveCallback);
+ connect(boost::asio::io_service& ioService, ReceiveCallback receiveCallback);
/** \brief Close the connection.
*/
@@ -78,7 +76,7 @@
send(const Block& header, const Block& payload) = 0;
/** \brief pause the transport
- * \post receiveCallback will not be invoked
+ * \post the receive callback will not be invoked
* \note This operation has no effect if transport has been paused,
* or when connection is being established.
*/
@@ -86,7 +84,7 @@
pause() = 0;
/** \brief resume the transport
- * \post receiveCallback will be invoked
+ * \post the receive callback will be invoked
* \note This operation has no effect if transport is not paused,
* or when connection is being established.
*/
@@ -97,45 +95,27 @@
* \retval false connection is not yet established or has been closed
*/
bool
- isConnected() const;
+ isConnected() const noexcept
+ {
+ return m_isConnected;
+ }
- /** \retval true incoming packets are expected, receiveCallback will be invoked
- * \retval false incoming packets are not expected, receiveCallback will not be invoked
+ /** \retval true incoming packets are expected, the receive callback will be invoked
+ * \retval false incoming packets are not expected, the receive callback will not be invoked
*/
bool
- isReceiving() const;
+ isReceiving() const noexcept
+ {
+ return m_isReceiving;
+ }
protected:
- /** \brief invoke the receive callback
- */
- void
- receive(const Block& wire);
-
-protected:
- boost::asio::io_service* m_ioService;
- bool m_isConnected;
- bool m_isReceiving;
+ boost::asio::io_service* m_ioService = nullptr;
ReceiveCallback m_receiveCallback;
+ bool m_isConnected = false;
+ bool m_isReceiving = false;
};
-inline bool
-Transport::isConnected() const
-{
- return m_isConnected;
-}
-
-inline bool
-Transport::isReceiving() const
-{
- return m_isReceiving;
-}
-
-inline void
-Transport::receive(const Block& wire)
-{
- m_receiveCallback(wire);
-}
-
} // namespace ndn
#endif // NDN_TRANSPORT_TRANSPORT_HPP
diff --git a/ndn-cxx/transport/unix-transport.cpp b/ndn-cxx/transport/unix-transport.cpp
index b33704c..6e909a2 100644
--- a/ndn-cxx/transport/unix-transport.cpp
+++ b/ndn-cxx/transport/unix-transport.cpp
@@ -73,15 +73,13 @@
}
void
-UnixTransport::connect(boost::asio::io_service& ioService,
- const ReceiveCallback& receiveCallback)
+UnixTransport::connect(boost::asio::io_service& ioService, ReceiveCallback receiveCallback)
{
NDN_LOG_DEBUG("connect path=" << m_unixSocket);
if (m_impl == nullptr) {
- Transport::connect(ioService, receiveCallback);
-
- m_impl = make_shared<Impl>(ref(*this), ref(ioService));
+ Transport::connect(ioService, std::move(receiveCallback));
+ m_impl = make_shared<Impl>(*this, ioService);
}
m_impl->connect(boost::asio::local::stream_protocol::endpoint(m_unixSocket));
diff --git a/ndn-cxx/transport/unix-transport.hpp b/ndn-cxx/transport/unix-transport.hpp
index 424ed53..d18dbe4 100644
--- a/ndn-cxx/transport/unix-transport.hpp
+++ b/ndn-cxx/transport/unix-transport.hpp
@@ -1,6 +1,6 @@
/* -*- Mode:C++; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */
/*
- * Copyright (c) 2013-2018 Regents of the University of California.
+ * Copyright (c) 2013-2019 Regents of the University of California.
*
* This file is part of ndn-cxx library (NDN C++ library with eXperimental eXtensions).
*
@@ -47,8 +47,7 @@
~UnixTransport() override;
void
- connect(boost::asio::io_service& ioService,
- const ReceiveCallback& receiveCallback) override;
+ connect(boost::asio::io_service& ioService, ReceiveCallback receiveCallback) override;
void
close() override;