diff --git a/dependencies/opendht b/dependencies/opendht
index 88d15be4ea8d8b827b8d0712a138ef6b0ce1d0ef..f2cee8e9ce24746caa7dee1847829c526d340284 160000
--- a/dependencies/opendht
+++ b/dependencies/opendht
@@ -1 +1 @@
-Subproject commit 88d15be4ea8d8b827b8d0712a138ef6b0ce1d0ef
+Subproject commit f2cee8e9ce24746caa7dee1847829c526d340284
diff --git a/include/upnp/upnp_context.h b/include/upnp/upnp_context.h
index ead135f2179b85e7807ab35fdf0988ed2dda7792..9941e3b24f4feedded35036d2210849f19c20de5 100644
--- a/include/upnp/upnp_context.h
+++ b/include/upnp/upnp_context.h
@@ -318,6 +318,7 @@ private:
// Shutdown synchronization
bool shutdownComplete_ {false};
+ bool shutdownTimedOut_ {false};
};
} // namespace upnp
diff --git a/src/upnp/protocol/pupnp/pupnp.cpp b/src/upnp/protocol/pupnp/pupnp.cpp
index 8aa462dbaa4061722d58b519b034a8a02420c2a5..2c81e6740fbcc8874fa06e0798e925eb4849abb2 100644
--- a/src/upnp/protocol/pupnp/pupnp.cpp
+++ b/src/upnp/protocol/pupnp/pupnp.cpp
@@ -17,7 +17,6 @@
#include "pupnp.h"
#include "string_utils.h"
-#include <opendht/thread_pool.h>
#include <opendht/http.h>
namespace dhtnet {
@@ -98,6 +97,7 @@ errorOnResponse(IXML_Document* doc, const std::shared_ptr<dht::log::Logger>& log
PUPnP::PUPnP(const std::shared_ptr<asio::io_context>& ctx, const std::shared_ptr<dht::log::Logger>& logger)
: UPnPProtocol(logger), ioContext(ctx), searchForIgdTimer_(*ctx)
+ , ongoingOpsThreadPool_(1, 64)
{
if (logger_) logger_->debug("PUPnP: Creating instance [{}] ...", fmt::ptr(this));
}
@@ -188,15 +188,20 @@ PUPnP::getHostAddress() const
}
void
-PUPnP::terminate(std::condition_variable& cv)
+PUPnP::terminate()
{
if (logger_) logger_->debug("PUPnP: Terminate instance {}", fmt::ptr(this));
clientRegistered_ = false;
observer_ = nullptr;
- std::unique_lock lk(ongoingOpsMtx_);
- destroying_ = true;
- cvOngoing_.wait(lk, [&]() { return ongoingOps_ == 0; });
+ {
+ std::lock_guard lk(ongoingOpsMtx_);
+ destroying_ = true;
+ if (ongoingOps_ > 0) {
+ if (logger_) logger_->debug("PUPnP: {} ongoing operations, detaching corresponding threads", ongoingOps_);
+ ongoingOpsThreadPool_.detach();
+ }
+ }
UpnpUnRegisterClient(ctrlptHandle_);
@@ -214,25 +219,7 @@ PUPnP::terminate(std::condition_variable& cv)
std::lock_guard lock(pupnpMutex_);
validIgdList_.clear();
shutdownComplete_ = true;
- cv.notify_one();
-}
-
-void
-PUPnP::terminate()
-{
- std::condition_variable cv {};
- ioContext->dispatch([&] {
- terminate(cv);
- });
-
- std::unique_lock lk(pupnpMutex_);
- if (cv.wait_for(lk, std::chrono::seconds(10), [this] { return shutdownComplete_; })) {
- if (logger_) logger_->debug("PUPnP: Shutdown completed");
- } else {
- if (logger_) logger_->error("PUPnP: Shutdown timed-out");
- // Force stop if the shutdown take too much time.
- shutdownComplete_ = true;
- }
+ if (logger_) logger_->debug("PUPnP: Instance {} terminated", fmt::ptr(this));
}
void
@@ -771,7 +758,7 @@ PUPnP::processDiscoverySearchResult(const std::string& cpDeviceId,
// Run a separate thread to prevent blocking this thread
// if the IGD HTTP server is not responsive.
- dht::ThreadPool::io().run([w = weak(), url=igdLocationUrl] {
+ ongoingOpsThreadPool_.run([w = weak(), url=igdLocationUrl] {
if (auto upnpThis = w.lock()) {
upnpThis->downLoadIgdDescription(url);
}
@@ -791,6 +778,13 @@ PUPnP::downLoadIgdDescription(const std::string& locationUrl)
IXML_Document* doc_container_ptr = nullptr;
int upnp_err = UpnpDownloadXmlDoc(locationUrl.c_str(), &doc_container_ptr);
+ std::lock_guard lk(ongoingOpsMtx_);
+ // Trying to use libupnp functions after UpnpFinish has been called (which may
+ // be the case if destroying_ is true) can cause errors. It's probably not a
+ // problem here, but return early just in case.
+ if (destroying_)
+ return;
+
if (upnp_err != UPNP_E_SUCCESS or not doc_container_ptr) {
if(logger_) logger_->warn("PUPnP: Error downloading device XML document from {} -> {}",
locationUrl,
@@ -803,9 +797,7 @@ PUPnP::downLoadIgdDescription(const std::string& locationUrl)
}
});
}
- std::lock_guard lk(ongoingOpsMtx_);
ongoingOps_--;
- cvOngoing_.notify_one();
}
void
diff --git a/src/upnp/protocol/pupnp/pupnp.h b/src/upnp/protocol/pupnp/pupnp.h
index 7deb4fba43788627cc474271f96944ef3229eea7..03086ddc1f5ec09cd8106bebc1f60439104b96e3 100644
--- a/src/upnp/protocol/pupnp/pupnp.h
+++ b/src/upnp/protocol/pupnp/pupnp.h
@@ -26,6 +26,7 @@
#include "upnp_igd.h"
#include "ip_utils.h"
+#include <opendht/thread_pool.h>
#include <upnp/upnp.h>
#include <upnp/upnptools.h>
@@ -246,9 +247,9 @@ private:
// Count ongoing operations
std::mutex ongoingOpsMtx_;
- std::condition_variable cvOngoing_;
int ongoingOps_ {0};
bool destroying_ {false};
+ dht::ThreadPool ongoingOpsThreadPool_;
};
} // namespace upnp
diff --git a/src/upnp/upnp_context.cpp b/src/upnp/upnp_context.cpp
index cc54cb166db219a310a1cb5912da6f252a9e77eb..78488dce959d44cfd7a09f09e33034233e05d3c4 100644
--- a/src/upnp/upnp_context.cpp
+++ b/src/upnp/upnp_context.cpp
@@ -94,6 +94,12 @@ UPnPContext::shutdown(std::condition_variable& cv)
controllerList_.clear();
protocolList_.clear();
shutdownComplete_ = true;
+ if (shutdownTimedOut_) {
+ // If we timed out in shutdown(), then calling notify_one is not necessary,
+ // and doing so anyway can cause bugs, see:
+ // https://git.jami.net/savoirfairelinux/dhtnet/-/issues/28
+ return;
+ }
cv.notify_one();
}
@@ -110,7 +116,8 @@ UPnPContext::shutdown()
if (cv.wait_for(lk, std::chrono::seconds(30), [this] { return shutdownComplete_; })) {
if (logger_) logger_->debug("Shutdown completed");
} else {
- if (logger_) logger_->error("Shutdown timed-out");
+ if (logger_) logger_->error("Shutdown timed out");
+ shutdownTimedOut_ = true;
}
// NOTE: It's important to unlock mappingMutex_ here, otherwise we get a
// deadlock when the call to cv.wait_for() above times out before we return
@@ -165,7 +172,7 @@ UPnPContext::startUpnp()
void
UPnPContext::stopUpnp(bool forceRelease)
{
- if (logger_) logger_->debug("Stopping UPNP context");
+ if (logger_) logger_->debug("Stopping UPnP context");
// Clear all current mappings if any.