face: don't crash if Face is destroyed with outstanding io.post
refs #3248
Change-Id: Ifd46e46fb1df849cbb3651204cd5a72f476cb81e
diff --git a/src/face.cpp b/src/face.cpp
index b52a54d..537f1b2 100644
--- a/src/face.cpp
+++ b/src/face.cpp
@@ -29,13 +29,27 @@
#include "util/random.hpp"
#include "util/face-uri.hpp"
+// A callback scheduled through io.post and io.dispatch may be invoked after the face
+// is destructed. To prevent this situation, these macros captures Face::m_impl as weak_ptr,
+// and skips callback execution if the face has been destructed.
+#define IO_CAPTURE_WEAK_IMPL(OP) \
+ { \
+ weak_ptr<Impl> implWeak(m_impl); \
+ m_ioService.OP([=] { \
+ auto impl = implWeak.lock(); \
+ if (impl != nullptr) {
+#define IO_CAPTURE_WEAK_IMPL_END \
+ } \
+ }); \
+ }
+
namespace ndn {
Face::Face(shared_ptr<Transport> transport)
: m_internalIoService(new boost::asio::io_service())
, m_ioService(*m_internalIoService)
, m_internalKeyChain(new KeyChain())
- , m_impl(new Impl(*this))
+ , m_impl(make_shared<Impl>(*this))
{
construct(transport, *m_internalKeyChain);
}
@@ -43,7 +57,7 @@
Face::Face(boost::asio::io_service& ioService)
: m_ioService(ioService)
, m_internalKeyChain(new KeyChain())
- , m_impl(new Impl(*this))
+ , m_impl(make_shared<Impl>(*this))
{
construct(nullptr, *m_internalKeyChain);
}
@@ -52,7 +66,7 @@
: m_internalIoService(new boost::asio::io_service())
, m_ioService(*m_internalIoService)
, m_internalKeyChain(new KeyChain())
- , m_impl(new Impl(*this))
+ , m_impl(make_shared<Impl>(*this))
{
construct(make_shared<TcpTransport>(host, port), *m_internalKeyChain);
}
@@ -60,7 +74,7 @@
Face::Face(shared_ptr<Transport> transport, KeyChain& keyChain)
: m_internalIoService(new boost::asio::io_service())
, m_ioService(*m_internalIoService)
- , m_impl(new Impl(*this))
+ , m_impl(make_shared<Impl>(*this))
{
construct(transport, keyChain);
}
@@ -68,14 +82,14 @@
Face::Face(shared_ptr<Transport> transport, boost::asio::io_service& ioService)
: m_ioService(ioService)
, m_internalKeyChain(new KeyChain())
- , m_impl(new Impl(*this))
+ , m_impl(make_shared<Impl>(*this))
{
construct(transport, *m_internalKeyChain);
}
Face::Face(shared_ptr<Transport> transport, boost::asio::io_service& ioService, KeyChain& keyChain)
: m_ioService(ioService)
- , m_impl(new Impl(*this))
+ , m_impl(make_shared<Impl>(*this))
{
construct(transport, keyChain);
}
@@ -136,7 +150,9 @@
m_nfdController.reset(new nfd::Controller(*this, keyChain));
- m_ioService.post([=] { m_impl->ensureConnected(false); });
+ IO_CAPTURE_WEAK_IMPL(post) {
+ impl->ensureConnected(false);
+ } IO_CAPTURE_WEAK_IMPL_END
}
Face::~Face() = default;
@@ -161,9 +177,9 @@
}
// If the same ioService thread, dispatch directly calls the method
- m_ioService.dispatch([=] {
- m_impl->asyncExpressInterest(interestToExpress, afterSatisfied, afterNacked, afterTimeout);
- });
+ IO_CAPTURE_WEAK_IMPL(dispatch) {
+ impl->asyncExpressInterest(interestToExpress, afterSatisfied, afterNacked, afterTimeout);
+ } IO_CAPTURE_WEAK_IMPL_END
return reinterpret_cast<const PendingInterestId*>(interestToExpress.get());
}
@@ -198,6 +214,28 @@
}
void
+Face::removePendingInterest(const PendingInterestId* pendingInterestId)
+{
+ IO_CAPTURE_WEAK_IMPL(post) {
+ impl->asyncRemovePendingInterest(pendingInterestId);
+ } IO_CAPTURE_WEAK_IMPL_END
+}
+
+void
+Face::removeAllPendingInterests()
+{
+ IO_CAPTURE_WEAK_IMPL(post) {
+ impl->asyncRemoveAllPendingInterests();
+ } IO_CAPTURE_WEAK_IMPL_END
+}
+
+size_t
+Face::getNPendingInterests() const
+{
+ return m_impl->m_pendingInterestTable.size();
+}
+
+void
Face::put(const Data& data)
{
// Use original `data`, since wire format should already exist for the original Data
@@ -215,31 +253,19 @@
}
// If the same ioService thread, dispatch directly calls the method
- m_ioService.dispatch([=] { m_impl->asyncPutData(dataPtr); });
+ IO_CAPTURE_WEAK_IMPL(dispatch) {
+ impl->asyncPutData(dataPtr);
+ } IO_CAPTURE_WEAK_IMPL_END
}
void
Face::put(const lp::Nack& nack)
{
- m_ioService.dispatch([=] { m_impl->asyncPutNack(make_shared<lp::Nack>(nack)); });
-}
+ auto nackPtr = make_shared<lp::Nack>(nack);
-void
-Face::removePendingInterest(const PendingInterestId* pendingInterestId)
-{
- m_ioService.post([=] { m_impl->asyncRemovePendingInterest(pendingInterestId); });
-}
-
-void
-Face::removeAllPendingInterests()
-{
- m_ioService.post([=] { m_impl->asyncRemoveAllPendingInterests(); });
-}
-
-size_t
-Face::getNPendingInterests() const
-{
- return m_impl->m_pendingInterestTable.size();
+ IO_CAPTURE_WEAK_IMPL(dispatch) {
+ impl->asyncPutNack(nackPtr);
+ } IO_CAPTURE_WEAK_IMPL_END
}
const RegisteredPrefixId*
@@ -275,7 +301,9 @@
{
auto filter = make_shared<InterestFilterRecord>(interestFilter, onInterest);
- m_ioService.post([=] { m_impl->asyncSetInterestFilter(filter); });
+ IO_CAPTURE_WEAK_IMPL(post) {
+ impl->asyncSetInterestFilter(filter);
+ } IO_CAPTURE_WEAK_IMPL_END
return reinterpret_cast<const InterestFilterId*>(filter.get());
}
@@ -381,13 +409,17 @@
void
Face::unsetInterestFilter(const RegisteredPrefixId* registeredPrefixId)
{
- m_ioService.post([=] { m_impl->asyncUnregisterPrefix(registeredPrefixId, nullptr, nullptr); });
+ IO_CAPTURE_WEAK_IMPL(post) {
+ impl->asyncUnregisterPrefix(registeredPrefixId, nullptr, nullptr);
+ } IO_CAPTURE_WEAK_IMPL_END
}
void
Face::unsetInterestFilter(const InterestFilterId* interestFilterId)
{
- m_ioService.post([=] { m_impl->asyncUnsetInterestFilter(interestFilterId); });
+ IO_CAPTURE_WEAK_IMPL(post) {
+ impl->asyncUnsetInterestFilter(interestFilterId);
+ } IO_CAPTURE_WEAK_IMPL_END
}
void
@@ -395,7 +427,9 @@
const UnregisterPrefixSuccessCallback& onSuccess,
const UnregisterPrefixFailureCallback& onFailure)
{
- m_ioService.post([=] { m_impl->asyncUnregisterPrefix(registeredPrefixId, onSuccess, onFailure); });
+ IO_CAPTURE_WEAK_IMPL(post) {
+ impl->asyncUnregisterPrefix(registeredPrefixId, onSuccess, onFailure);
+ } IO_CAPTURE_WEAK_IMPL_END
}
void
@@ -440,7 +474,9 @@
void
Face::shutdown()
{
- m_ioService.post([this] { this->asyncShutdown(); });
+ IO_CAPTURE_WEAK_IMPL(post) {
+ this->asyncShutdown();
+ } IO_CAPTURE_WEAK_IMPL_END
}
void
diff --git a/src/face.hpp b/src/face.hpp
index c83a9ae..51a5270 100644
--- a/src/face.hpp
+++ b/src/face.hpp
@@ -187,6 +187,7 @@
* @param host IP address or hostname of the NDN forwarder
* @param port port number or service name of the NDN forwarder (**default**: "6363")
*/
+ explicit
Face(const std::string& host, const std::string& port = "6363");
/**
@@ -744,7 +745,7 @@
unique_ptr<nfd::Controller> m_nfdController;
class Impl;
- unique_ptr<Impl> m_impl;
+ shared_ptr<Impl> m_impl;
};
} // namespace ndn
diff --git a/tests/unit-tests/face.t.cpp b/tests/unit-tests/face.t.cpp
index bf87de6..42def58 100644
--- a/tests/unit-tests/face.t.cpp
+++ b/tests/unit-tests/face.t.cpp
@@ -577,6 +577,8 @@
BOOST_AUTO_TEST_SUITE_END() // Producer
+BOOST_AUTO_TEST_SUITE(IoRoutines)
+
BOOST_AUTO_TEST_CASE(ProcessEvents)
{
face.processEvents(time::milliseconds(-1)); // io_service::reset()/poll() inside
@@ -594,6 +596,20 @@
BOOST_CHECK_EQUAL(nRegSuccesses, 1);
}
+BOOST_AUTO_TEST_CASE(DestroyWithoutProcessEvents) // Bug 3248
+{
+ auto face2 = make_unique<Face>(io);
+ face2.reset();
+
+ io.poll(); // should not crash
+}
+
+BOOST_AUTO_TEST_SUITE_END() // IoRoutines
+
+BOOST_AUTO_TEST_SUITE(Transport)
+
+using ndn::Transport;
+
struct PibDirWithDefaultTpm
{
const std::string PATH = "build/keys-with-default-tpm";
@@ -616,8 +632,6 @@
BOOST_CHECK(Face(transport, io, keyChain).getTransport() == transport);
}
-BOOST_AUTO_TEST_SUITE(CustomizeTransport)
-
class WithEnv : private IdentityManagementTimeFixture
{
public:
@@ -725,7 +739,8 @@
BOOST_CHECK(dynamic_pointer_cast<UnixTransport>(face->getTransport()) != nullptr);
}
-BOOST_AUTO_TEST_SUITE_END() // CustomizeTransport
+BOOST_AUTO_TEST_SUITE_END() // Transport
+
BOOST_AUTO_TEST_SUITE_END() // TestFace
} // namespace tests