face: revert unintended behavior change in Transport::setPersistency
Prior to commit 32dab97321d954800f70e4a695e326c998d37a93, persistency
transitions from NONE were not logged and before/afterChangePersistency
was not called. Restore that behavior.
This commit also improves test coverage of the persistency-related
functions in the various Transport subclasses.
Change-Id: Ide77c67ea277fca3d1cad5ea131ae0fa259db75c
Refs: #3232
diff --git a/AUTHORS.md b/AUTHORS.md
index 0c36dc7..8356374 100644
--- a/AUTHORS.md
+++ b/AUTHORS.md
@@ -43,7 +43,7 @@
* University Pierre & Marie Curie, Sorbonne University
- * Davide Pesavento <http://www.lip6.fr/actualite/personnes-fiche.php?ident=D1469>
+ * Davide Pesavento <https://www.linkedin.com/in/davidepesavento>
* Giulio Grassi <http://www.lip6.fr/actualite/personnes-fiche.php?ident=D1461>
* Giovanni Pau <http://www.cs.ucla.edu/~gpau/Giovanni_Paus_Home_Page/Home.html>
diff --git a/daemon/face/transport.cpp b/daemon/face/transport.cpp
index 81f6edf..9c054fa 100644
--- a/daemon/face/transport.cpp
+++ b/daemon/face/transport.cpp
@@ -68,9 +68,7 @@
{
}
-Transport::~Transport()
-{
-}
+Transport::~Transport() = default;
void
Transport::setFaceAndLinkService(Face& face, LinkService& service)
@@ -91,7 +89,7 @@
this->setState(TransportState::CLOSING);
this->doClose();
- // warning: don't access any fields after this:
+ // warning: don't access any members after this:
// the Transport may be deallocated if doClose changes state to CLOSED
}
@@ -158,11 +156,13 @@
return;
}
- NFD_LOG_FACE_INFO("setPersistency " << m_persistency << " -> " << newPersistency);
-
auto oldPersistency = m_persistency;
m_persistency = newPersistency;
- this->afterChangePersistency(oldPersistency);
+
+ if (oldPersistency != ndn::nfd::FACE_PERSISTENCY_NONE) {
+ NFD_LOG_FACE_INFO("setPersistency " << oldPersistency << " -> " << newPersistency);
+ this->afterChangePersistency(oldPersistency);
+ }
}
void
@@ -198,7 +198,7 @@
}
if (!isValid) {
- throw std::runtime_error("invalid state transition");
+ BOOST_THROW_EXCEPTION(std::runtime_error("invalid state transition"));
}
NFD_LOG_FACE_INFO("setState " << m_state << " -> " << newState);
@@ -206,7 +206,7 @@
TransportState oldState = m_state;
m_state = newState;
afterStateChange(oldState, newState);
- // warning: don't access any fields after this:
+ // warning: don't access any members after this:
// the Transport may be deallocated in the signal handler if newState is CLOSED
}
diff --git a/daemon/face/transport.hpp b/daemon/face/transport.hpp
index 3db4714..ff5363f 100644
--- a/daemon/face/transport.hpp
+++ b/daemon/face/transport.hpp
@@ -221,16 +221,13 @@
ndn::nfd::FacePersistency
getPersistency() const;
- /** \brief check whether the intended change from the current persistency to \p newPersistency
- * can be performed
+ /** \brief check whether the face persistency can be changed to \p newPersistency
*
- * This function serves as an external API, and invokes the internal function
- * canChangePersistencyToImpl to perform further checks if \p newPersistency differs from
- * the current persistency.
+ * This function serves as the external API, and invokes the protected function
+ * canChangePersistencyToImpl to perform further checks if \p newPersistency differs
+ * from the current persistency.
*
- * \pre getPersistency() != NONE
- *
- * \return true if the intended change can be performed, otherwise false
+ * \return true if the change can be performed, false otherwise
*/
bool
canChangePersistencyTo(ndn::nfd::FacePersistency newPersistency) const;
@@ -238,7 +235,7 @@
/** \brief changes face persistency setting
*/
void
- setPersistency(ndn::nfd::FacePersistency persistency);
+ setPersistency(ndn::nfd::FacePersistency newPersistency);
/** \return whether face is point-to-point or multi-access
*/
diff --git a/tests/daemon/face/ethernet-transport.t.cpp b/tests/daemon/face/ethernet-transport.t.cpp
index 4cc9ceb..cd46a04 100644
--- a/tests/daemon/face/ethernet-transport.t.cpp
+++ b/tests/daemon/face/ethernet-transport.t.cpp
@@ -52,6 +52,16 @@
BOOST_CHECK_EQUAL(transport.getLinkType(), ndn::nfd::LINK_TYPE_MULTI_ACCESS);
}
+BOOST_AUTO_TEST_CASE(PersistencyChange)
+{
+ SKIP_IF_ETHERNET_NETIF_COUNT_LT(1);
+ EthernetTransport transport(netifs.front(), ethernet::getDefaultMulticastAddress());
+
+ BOOST_CHECK_EQUAL(transport.canChangePersistencyTo(ndn::nfd::FACE_PERSISTENCY_ON_DEMAND), false);
+ BOOST_CHECK_EQUAL(transport.canChangePersistencyTo(ndn::nfd::FACE_PERSISTENCY_PERSISTENT), false);
+ BOOST_CHECK_EQUAL(transport.canChangePersistencyTo(ndn::nfd::FACE_PERSISTENCY_PERMANENT), true);
+}
+
///\todo #3369 add the equivalent of these test cases from ethernet.t.cpp
/// as of commit:65caf200924b28748037750449e28bcb548dbc9c
/// SendPacket, ProcessIncomingPacket
diff --git a/tests/daemon/face/multicast-udp-transport.t.cpp b/tests/daemon/face/multicast-udp-transport.t.cpp
index ac1e33c..2b9407d 100644
--- a/tests/daemon/face/multicast-udp-transport.t.cpp
+++ b/tests/daemon/face/multicast-udp-transport.t.cpp
@@ -1,6 +1,6 @@
/* -*- Mode:C++; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */
/**
- * Copyright (c) 2014-2015, Regents of the University of California,
+ * Copyright (c) 2014-2017, Regents of the University of California,
* Arizona Board of Regents,
* Colorado State University,
* University Pierre & Marie Curie, Sorbonne University,
@@ -51,6 +51,16 @@
BOOST_CHECK_EQUAL(transport->getMtu(), 65535 - 60 - 8);
}
+BOOST_AUTO_TEST_CASE(PersistencyChange)
+{
+ SKIP_IF_IP_UNAVAILABLE(defaultAddr);
+ initialize(defaultAddr);
+
+ BOOST_CHECK_EQUAL(transport->canChangePersistencyTo(ndn::nfd::FACE_PERSISTENCY_ON_DEMAND), false);
+ BOOST_CHECK_EQUAL(transport->canChangePersistencyTo(ndn::nfd::FACE_PERSISTENCY_PERSISTENT), false);
+ BOOST_CHECK_EQUAL(transport->canChangePersistencyTo(ndn::nfd::FACE_PERSISTENCY_PERMANENT), true);
+}
+
BOOST_AUTO_TEST_CASE(ReceiveMultipleRemoteEndpoints)
{
SKIP_IF_IP_UNAVAILABLE(defaultAddr);
diff --git a/tests/daemon/face/tcp-transport.t.cpp b/tests/daemon/face/tcp-transport.t.cpp
index a881f83..0d5578f 100644
--- a/tests/daemon/face/tcp-transport.t.cpp
+++ b/tests/daemon/face/tcp-transport.t.cpp
@@ -1,6 +1,6 @@
/* -*- Mode:C++; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */
/**
- * Copyright (c) 2014-2016, Regents of the University of California,
+ * Copyright (c) 2014-2017, Regents of the University of California,
* Arizona Board of Regents,
* Colorado State University,
* University Pierre & Marie Curie, Sorbonne University,
@@ -102,6 +102,17 @@
BOOST_CHECK_EQUAL(transport->getMtu(), MTU_UNLIMITED);
}
+BOOST_AUTO_TEST_CASE(PersistencyChange)
+{
+ auto address = getTestIp<ip::address_v4>();
+ SKIP_IF_IP_UNAVAILABLE(address);
+ initialize(address);
+
+ BOOST_CHECK_EQUAL(transport->canChangePersistencyTo(ndn::nfd::FACE_PERSISTENCY_ON_DEMAND), true);
+ BOOST_CHECK_EQUAL(transport->canChangePersistencyTo(ndn::nfd::FACE_PERSISTENCY_PERSISTENT), true);
+ BOOST_CHECK_EQUAL(transport->canChangePersistencyTo(ndn::nfd::FACE_PERSISTENCY_PERMANENT), true);
+}
+
BOOST_AUTO_TEST_CASE(PermanentReconnect)
{
auto address = getTestIp<ip::address_v4>();
diff --git a/tests/daemon/face/transport.t.cpp b/tests/daemon/face/transport.t.cpp
index f1171e5..c9a4690 100644
--- a/tests/daemon/face/transport.t.cpp
+++ b/tests/daemon/face/transport.t.cpp
@@ -1,6 +1,6 @@
/* -*- Mode:C++; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */
/**
- * Copyright (c) 2014-2016, Regents of the University of California,
+ * Copyright (c) 2014-2017, Regents of the University of California,
* Arizona Board of Regents,
* Colorado State University,
* University Pierre & Marie Curie, Sorbonne University,
@@ -40,46 +40,58 @@
namespace face {
namespace tests {
-using namespace nfd::tests;
namespace mpl = boost::mpl;
-class DummyTransportFixture : public BaseFixture
-{
-protected:
- DummyTransportFixture()
- : transport(nullptr)
- , sentPackets(nullptr)
- , receivedPackets(nullptr)
- {
- // Constructor does not initialize the fixture,
- // so that test case may specify different parameters to DummyTransport constructor.
- }
-
- void
- initialize(unique_ptr<DummyTransport> transport = make_unique<DummyTransport>())
- {
- this->face = make_unique<Face>(make_unique<DummyReceiveLinkService>(), std::move(transport));
- this->transport = static_cast<DummyTransport*>(face->getTransport());
- this->sentPackets = &this->transport->sentPackets;
- this->receivedPackets = &static_cast<DummyReceiveLinkService*>(face->getLinkService())->receivedPackets;
- }
-
-protected:
- unique_ptr<Face> face;
- DummyTransport* transport;
- std::vector<Transport::Packet>* sentPackets;
- std::vector<Transport::Packet>* receivedPackets;
-};
-
BOOST_AUTO_TEST_SUITE(Face)
-BOOST_FIXTURE_TEST_SUITE(TestTransport, DummyTransportFixture)
+BOOST_AUTO_TEST_SUITE(TestTransport)
BOOST_AUTO_TEST_CASE(DummyTransportStaticProperties)
{
- this->initialize();
+ auto transport = make_unique<DummyTransport>();
checkStaticPropertiesInitialized(*transport);
}
+class PersistencyTestTransport : public DummyTransport
+{
+public:
+ PersistencyTestTransport()
+ : DummyTransport("dummy://", "dummy://",
+ ndn::nfd::FACE_SCOPE_NON_LOCAL,
+ ndn::nfd::FACE_PERSISTENCY_ON_DEMAND)
+ {
+ }
+
+protected:
+ void
+ afterChangePersistency(ndn::nfd::FacePersistency oldPersistency) final
+ {
+ persistencyHistory.push_back(oldPersistency);
+ }
+
+public:
+ std::vector<ndn::nfd::FacePersistency> persistencyHistory;
+};
+
+BOOST_AUTO_TEST_CASE(PersistencyChange)
+{
+ auto transport = make_unique<PersistencyTestTransport>();
+ BOOST_CHECK_EQUAL(transport->getPersistency(), ndn::nfd::FACE_PERSISTENCY_ON_DEMAND);
+ BOOST_CHECK_EQUAL(transport->persistencyHistory.size(), 0);
+
+ BOOST_CHECK_EQUAL(transport->canChangePersistencyTo(ndn::nfd::FACE_PERSISTENCY_NONE), false);
+ BOOST_REQUIRE_EQUAL(transport->canChangePersistencyTo(transport->getPersistency()), true);
+ BOOST_REQUIRE_EQUAL(transport->canChangePersistencyTo(ndn::nfd::FACE_PERSISTENCY_PERMANENT), true);
+
+ transport->setPersistency(transport->getPersistency());
+ BOOST_CHECK_EQUAL(transport->getPersistency(), ndn::nfd::FACE_PERSISTENCY_ON_DEMAND);
+ BOOST_CHECK_EQUAL(transport->persistencyHistory.size(), 0);
+
+ transport->setPersistency(ndn::nfd::FACE_PERSISTENCY_PERMANENT);
+ BOOST_CHECK_EQUAL(transport->getPersistency(), ndn::nfd::FACE_PERSISTENCY_PERMANENT);
+ BOOST_REQUIRE_EQUAL(transport->persistencyHistory.size(), 1);
+ BOOST_CHECK_EQUAL(transport->persistencyHistory.back(), ndn::nfd::FACE_PERSISTENCY_ON_DEMAND);
+}
+
/** \brief a macro to declare a TransportState as a integral constant
*/
#define TRANSPORT_STATE_C(X) mpl::int_<static_cast<int>(TransportState::X)>
@@ -157,20 +169,21 @@
BOOST_AUTO_TEST_CASE_TEMPLATE(SetState, T, AllStateTransitions)
{
- this->initialize();
+ auto transport = make_unique<DummyTransport>();
TransportState from = static_cast<TransportState>(T::first::value);
TransportState to = static_cast<TransportState>(T::second::value);
BOOST_TEST_MESSAGE("SetState " << from << " -> " << to);
// enter from state
- typedef typename mpl::at<StateEntering, mpl::int_<T::first::value>>::type Steps;
- mpl::for_each<Steps>(
- [this] (int state) { transport->setState(static_cast<TransportState>(state)); });
+ using Steps = typename mpl::at<StateEntering, mpl::int_<T::first::value>>::type;
+ mpl::for_each<Steps>([&transport] (int state) {
+ transport->setState(static_cast<TransportState>(state));
+ });
BOOST_REQUIRE_EQUAL(transport->getState(), from);
bool hasSignal = false;
- this->transport->afterStateChange.connect(
+ transport->afterStateChange.connect(
[from, to, &hasSignal] (TransportState oldState, TransportState newState) {
hasSignal = true;
BOOST_CHECK_EQUAL(oldState, from);
@@ -187,18 +200,46 @@
BOOST_CHECK_EQUAL(hasSignal, from != to);
}
else {
- BOOST_CHECK_THROW(this->transport->setState(to), std::runtime_error);
+ BOOST_CHECK_THROW(transport->setState(to), std::runtime_error);
}
}
BOOST_AUTO_TEST_CASE(NoExpirationTime)
{
- initialize();
+ auto transport = make_unique<DummyTransport>();
BOOST_CHECK_EQUAL(transport->getExpirationTime(), time::steady_clock::TimePoint::max());
}
-BOOST_AUTO_TEST_CASE(Send)
+class DummyTransportFixture : public nfd::tests::BaseFixture
+{
+protected:
+ DummyTransportFixture()
+ : transport(nullptr)
+ , sentPackets(nullptr)
+ , receivedPackets(nullptr)
+ {
+ // Constructor does not initialize the fixture,
+ // so that test case may specify different parameters to DummyTransport constructor.
+ }
+
+ void
+ initialize(unique_ptr<DummyTransport> transport = make_unique<DummyTransport>())
+ {
+ this->face = make_unique<nfd::Face>(make_unique<DummyReceiveLinkService>(), std::move(transport));
+ this->transport = static_cast<DummyTransport*>(face->getTransport());
+ this->sentPackets = &this->transport->sentPackets;
+ this->receivedPackets = &static_cast<DummyReceiveLinkService*>(face->getLinkService())->receivedPackets;
+ }
+
+protected:
+ unique_ptr<nfd::Face> face;
+ DummyTransport* transport;
+ std::vector<Transport::Packet>* sentPackets;
+ std::vector<Transport::Packet>* receivedPackets;
+};
+
+BOOST_FIXTURE_TEST_CASE(Send, DummyTransportFixture)
{
this->initialize();
@@ -224,7 +265,7 @@
BOOST_CHECK(sentPackets->at(2).packet == pkt3);
}
-BOOST_AUTO_TEST_CASE(Receive)
+BOOST_FIXTURE_TEST_CASE(Receive, DummyTransportFixture)
{
this->initialize();
diff --git a/tests/daemon/face/unicast-udp-transport.t.cpp b/tests/daemon/face/unicast-udp-transport.t.cpp
index d45b116..98d99ac 100644
--- a/tests/daemon/face/unicast-udp-transport.t.cpp
+++ b/tests/daemon/face/unicast-udp-transport.t.cpp
@@ -1,6 +1,6 @@
/* -*- Mode:C++; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */
/**
- * Copyright (c) 2014-2016, Regents of the University of California,
+ * Copyright (c) 2014-2017, Regents of the University of California,
* Arizona Board of Regents,
* Colorado State University,
* University Pierre & Marie Curie, Sorbonne University,
@@ -102,6 +102,17 @@
BOOST_CHECK_EQUAL(transport->getMtu(), 65535 - 8);
}
+BOOST_AUTO_TEST_CASE(PersistencyChange)
+{
+ auto address = getTestIp<ip::address_v4>();
+ SKIP_IF_IP_UNAVAILABLE(address);
+ initialize(address);
+
+ BOOST_CHECK_EQUAL(transport->canChangePersistencyTo(ndn::nfd::FACE_PERSISTENCY_ON_DEMAND), true);
+ BOOST_CHECK_EQUAL(transport->canChangePersistencyTo(ndn::nfd::FACE_PERSISTENCY_PERSISTENT), true);
+ BOOST_CHECK_EQUAL(transport->canChangePersistencyTo(ndn::nfd::FACE_PERSISTENCY_PERMANENT), true);
+}
+
BOOST_AUTO_TEST_CASE(IdleClose)
{
auto address = getTestIp<ip::address_v4>();
diff --git a/tests/daemon/face/unix-stream-transport.t.cpp b/tests/daemon/face/unix-stream-transport.t.cpp
index 8042042..de2e03a 100644
--- a/tests/daemon/face/unix-stream-transport.t.cpp
+++ b/tests/daemon/face/unix-stream-transport.t.cpp
@@ -1,6 +1,6 @@
/* -*- Mode:C++; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */
/**
- * Copyright (c) 2014-2015, Regents of the University of California,
+ * Copyright (c) 2014-2017, Regents of the University of California,
* Arizona Board of Regents,
* Colorado State University,
* University Pierre & Marie Curie, Sorbonne University,
@@ -50,6 +50,15 @@
BOOST_CHECK_EQUAL(transport->getMtu(), MTU_UNLIMITED);
}
+BOOST_AUTO_TEST_CASE(PersistencyChange)
+{
+ initialize();
+
+ BOOST_CHECK_EQUAL(transport->canChangePersistencyTo(ndn::nfd::FACE_PERSISTENCY_ON_DEMAND), true);
+ BOOST_CHECK_EQUAL(transport->canChangePersistencyTo(ndn::nfd::FACE_PERSISTENCY_PERSISTENT), false);
+ BOOST_CHECK_EQUAL(transport->canChangePersistencyTo(ndn::nfd::FACE_PERSISTENCY_PERMANENT), false);
+}
+
BOOST_AUTO_TEST_SUITE_END() // TestUnixStreamTransport
BOOST_AUTO_TEST_SUITE_END() // Face
diff --git a/tests/daemon/face/websocket-transport.t.cpp b/tests/daemon/face/websocket-transport.t.cpp
index 8442480..5d537ab 100644
--- a/tests/daemon/face/websocket-transport.t.cpp
+++ b/tests/daemon/face/websocket-transport.t.cpp
@@ -1,6 +1,6 @@
/* -*- Mode:C++; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */
/**
- * Copyright (c) 2014-2016, Regents of the University of California,
+ * Copyright (c) 2014-2017, Regents of the University of California,
* Arizona Board of Regents,
* Colorado State University,
* University Pierre & Marie Curie, Sorbonne University,
@@ -278,6 +278,17 @@
BOOST_CHECK_EQUAL(transport->getMtu(), MTU_UNLIMITED);
}
+BOOST_AUTO_TEST_CASE(PersistencyChange)
+{
+ auto address = getTestIp<ip::address_v4>();
+ SKIP_IF_IP_UNAVAILABLE(address);
+ this->endToEndInitialize(ip::tcp::endpoint(address, 20070));
+
+ BOOST_CHECK_EQUAL(transport->canChangePersistencyTo(ndn::nfd::FACE_PERSISTENCY_ON_DEMAND), true);
+ BOOST_CHECK_EQUAL(transport->canChangePersistencyTo(ndn::nfd::FACE_PERSISTENCY_PERSISTENT), false);
+ BOOST_CHECK_EQUAL(transport->canChangePersistencyTo(ndn::nfd::FACE_PERSISTENCY_PERMANENT), false);
+}
+
BOOST_AUTO_TEST_CASE(PingPong)
{
auto address = getTestIp<ip::address_v4>();