Commit eb7bde60 authored by Adrien Béraud's avatar Adrien Béraud

upnp: download xml doc using std::future

The UpnpDownloadXmlDoc is now called using shared_future. The api
function is a blocking call and there were instances where,
depending on the router and wifi speeds, the function might fail
or hang. By using std::shared_future, if such a thing arises, the
program won't block.

Change-Id: Ibca8d926e8980ebd4519f0e6bbd6ce2125396159
parent fc251a5d
...@@ -97,7 +97,7 @@ NatPmp::~NatPmp() ...@@ -97,7 +97,7 @@ NatPmp::~NatPmp()
} }
void void
NatPmp::clearIGDs() NatPmp::clearIgds()
{ {
// Lock valid IGD. // Lock valid IGD.
std::lock_guard<std::mutex> lock(validIgdMutex_); std::lock_guard<std::mutex> lock(validIgdMutex_);
...@@ -113,7 +113,7 @@ NatPmp::clearIGDs() ...@@ -113,7 +113,7 @@ NatPmp::clearIGDs()
} }
void void
NatPmp::searchForIGD() NatPmp::searchForIgd()
{ {
// Lock valid IGD. // Lock valid IGD.
std::lock_guard<std::mutex> lock(validIgdMutex_); std::lock_guard<std::mutex> lock(validIgdMutex_);
......
...@@ -55,10 +55,10 @@ public: ...@@ -55,10 +55,10 @@ public:
Type getType() const override { return Type::NAT_PMP; } Type getType() const override { return Type::NAT_PMP; }
// Notifies a change in network. // Notifies a change in network.
void clearIGDs() override; void clearIgds() override;
// Renew pmp_igd. // Renew pmp_igd.
void searchForIGD() override; void searchForIgd() override;
// Tries to add mapping. Assumes mutex is already locked. // Tries to add mapping. Assumes mutex is already locked.
Mapping addMapping(IGD* igd, uint16_t port_external, uint16_t port_internal, PortType type, UPnPProtocol::UpnpError& upnp_error) override; Mapping addMapping(IGD* igd, uint16_t port_external, uint16_t port_internal, PortType type, UPnPProtocol::UpnpError& upnp_error) override;
......
This diff is collapsed.
...@@ -2,7 +2,7 @@ ...@@ -2,7 +2,7 @@
* Copyright (C) 2004-2019 Savoir-faire Linux Inc. * Copyright (C) 2004-2019 Savoir-faire Linux Inc.
* *
* Author: Stepan Salenikovich <stepan.salenikovich@savoirfairelinux.com> * Author: Stepan Salenikovich <stepan.salenikovich@savoirfairelinux.com>
* Author: Eden Abitbol <eden.abitbol@savoirfairelinux.com> * Author: Eden Abitbol <eden.abitbol@savoirfairelinux.com>
* *
* This program is free software; you can redistribute it and/or modify * This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by * it under the terms of the GNU General Public License as published by
...@@ -50,6 +50,12 @@ ...@@ -50,6 +50,12 @@
#include <atomic> #include <atomic>
#include <thread> #include <thread>
#include <list>
#include <map>
#include <set>
#include <string>
#include <memory>
#include <future>
namespace jami { namespace jami {
class IpAddr; class IpAddr;
...@@ -68,6 +74,12 @@ constexpr static unsigned int SUBSCRIBE_TIMEOUT {300}; ...@@ -68,6 +74,12 @@ constexpr static unsigned int SUBSCRIBE_TIMEOUT {300};
class PUPnP : public UPnPProtocol class PUPnP : public UPnPProtocol
{ {
public: public:
using XMLDocument = std::unique_ptr<IXML_Document, decltype(ixmlDocument_free)&>;
struct IGDInfo {
std::string location;
XMLDocument document;
};
PUPnP(); PUPnP();
~PUPnP(); ~PUPnP();
...@@ -75,10 +87,10 @@ public: ...@@ -75,10 +87,10 @@ public:
Type getType() const override { return Type::PUPNP; } Type getType() const override { return Type::PUPNP; }
// Notifies a change in network. // Notifies a change in network.
void clearIGDs() override; void clearIgds() override;
// Sends out async search for IGD. // Sends out async search for IGD.
void searchForIGD() override; void searchForIgd() override;
// Tries to add mapping. Assumes mutex is already locked. // Tries to add mapping. Assumes mutex is already locked.
Mapping addMapping(IGD* igd, uint16_t port_external, uint16_t port_internal, PortType type, UPnPProtocol::UpnpError& upnp_error) override; Mapping addMapping(IGD* igd, uint16_t port_external, uint16_t port_internal, PortType type, UPnPProtocol::UpnpError& upnp_error) override;
...@@ -90,8 +102,8 @@ public: ...@@ -90,8 +102,8 @@ public:
void removeAllLocalMappings(IGD* igd) override; void removeAllLocalMappings(IGD* igd) override;
private: private:
// Register client in an async manner using a thread. // Validate IGD from the xml document received from the router.
void registerClientAsync(); bool validateIgd(const IGDInfo&);
// Control point callback. // Control point callback.
static int ctrlPtCallback(Upnp_EventType event_type, const void* event, void* user_data); static int ctrlPtCallback(Upnp_EventType event_type, const void* event, void* user_data);
...@@ -116,7 +128,7 @@ private: ...@@ -116,7 +128,7 @@ private:
int handleSubscriptionUPnPEvent(Upnp_EventType event_type, const void* event); int handleSubscriptionUPnPEvent(Upnp_EventType event_type, const void* event);
// Parses the IGD candidate. // Parses the IGD candidate.
std::unique_ptr<UPnPIGD> parseIGD(IXML_Document* doc, const UpnpDiscovery* d_event); std::unique_ptr<UPnPIGD> parseIgd(IXML_Document* doc, std::string locationUrl);
// These functions directly create UPnP actions and make synchronous UPnP control point calls. Assumes mutex is already locked. // These functions directly create UPnP actions and make synchronous UPnP control point calls. Assumes mutex is already locked.
bool actionIsIgdConnected(const UPnPIGD& igd); bool actionIsIgdConnected(const UPnPIGD& igd);
...@@ -133,11 +145,14 @@ private: ...@@ -133,11 +145,14 @@ private:
std::thread pupnpThread_ {}; // PUPnP thread for non-blocking client registration. std::thread pupnpThread_ {}; // PUPnP thread for non-blocking client registration.
std::map<std::string, std::shared_ptr<IGD>> validIgdList_; // Map of valid IGDs with their UDN (universal Id). std::map<std::string, std::shared_ptr<IGD>> validIgdList_; // Map of valid IGDs with their UDN (universal Id).
std::map<std::string, std::string> cpDeviceList_; // Control point device list containing the device ID and device subscription event url. std::set<std::string> cpDeviceList_; // Control point device list containing the device ID and device subscription event url.
std::list<std::future<IGDInfo>> dwnldlXmlList_; // List of shared_futures for blocking xml download function calls.
std::mutex ctrlptMutex_; // Mutex for client handle protection. std::mutex ctrlptMutex_; // Mutex for client handle protection.
UpnpClient_Handle ctrlptHandle_ {-1}; // Control point handle. UpnpClient_Handle ctrlptHandle_ {-1}; // Control point handle.
std::atomic_bool clientRegistered_ { false }; // Indicates of the client is registered. std::atomic_bool clientRegistered_ { false }; // Indicates of the client is registered.
std::atomic_bool searchForIgd_ { false }; // Variable to signal thread for a search.
}; };
}} // namespace jami::upnp }} // namespace jami::upnp
\ No newline at end of file
...@@ -23,15 +23,19 @@ ...@@ -23,15 +23,19 @@
namespace jami { namespace upnp { namespace jami { namespace upnp {
UPnPIGD::UPnPIGD(std::string&& UDN, std::string&& baseURL, std::string&& friendlyName, std::string&& serviceType, std::string&& serviceId, std::string&& controlURL, std::string&& eventSubURL, UPnPIGD::UPnPIGD(std::string&& UDN, std::string&& baseURL,
IpAddr&& localIp, IpAddr&& publicIp): std::string&& friendlyName, std::string&& serviceType,
IGD(std::move(localIp), std::move(publicIp)) std::string&& serviceId, std::string && locationURL,
std::string&& controlURL, std::string&& eventSubURL,
IpAddr&& localIp, IpAddr&& publicIp):
IGD(std::move(localIp), std::move(publicIp))
{ {
UDN_ = std::move(UDN); UDN_ = std::move(UDN);
baseURL_ = std::move(baseURL); baseURL_ = std::move(baseURL);
friendlyName_ = std::move(friendlyName); friendlyName_ = std::move(friendlyName);
serviceType_ = std::move(serviceType); serviceType_ = std::move(serviceType);
serviceId_ = std::move(serviceId); serviceId_ = std::move(serviceId);
locationURL_ = std::move(locationURL);
controlURL_ = std::move(controlURL); controlURL_ = std::move(controlURL);
eventSubURL_ = std::move(eventSubURL); eventSubURL_ = std::move(eventSubURL);
} }
...@@ -56,6 +60,7 @@ UPnPIGD::operator==(UPnPIGD& other) const ...@@ -56,6 +60,7 @@ UPnPIGD::operator==(UPnPIGD& other) const
friendlyName_ == other.friendlyName_ and friendlyName_ == other.friendlyName_ and
serviceType_ == other.serviceType_ and serviceType_ == other.serviceType_ and
serviceId_ == other.serviceId_ and serviceId_ == other.serviceId_ and
locationURL_ == other.locationURL_ and
controlURL_ == other.controlURL_ and controlURL_ == other.controlURL_ and
eventSubURL_ == other.eventSubURL_; eventSubURL_ == other.eventSubURL_;
} }
......
...@@ -47,18 +47,20 @@ public: ...@@ -47,18 +47,20 @@ public:
std::string&& friendlyName, std::string&& friendlyName,
std::string&& serviceType, std::string&& serviceType,
std::string&& serviceId, std::string&& serviceId,
std::string&& locationURL,
std::string&& controlURL, std::string&& controlURL,
std::string&& eventSubURL, std::string&& eventSubURL,
IpAddr&& localIp = {}, IpAddr&& localIp = {},
IpAddr&& publicIp = {}); IpAddr&& publicIp = {});
~UPnPIGD(){} ~UPnPIGD(){}
const std::string& getUDN() const { return UDN_; }; const std::string& getUDN() const { return UDN_; }
const std::string& getBaseURL() const { return baseURL_; }; const std::string& getBaseURL() const { return baseURL_; }
const std::string& getFriendlyName() const { return friendlyName_; }; const std::string& getFriendlyName() const { return friendlyName_; }
const std::string& getServiceType() const { return serviceType_; }; const std::string& getServiceType() const { return serviceType_; }
const std::string& getServiceId() const { return serviceId_; }; const std::string& getServiceId() const { return serviceId_; }
const std::string& getControlURL() const { return controlURL_; }; const std::string& getLocationURL() const { return locationURL_; }
const std::string& getEventSubURL() const { return eventSubURL_; }; const std::string& getControlURL() const { return controlURL_; }
const std::string& getEventSubURL() const { return eventSubURL_; }
bool operator==(IGD& other) const; bool operator==(IGD& other) const;
bool operator==(UPnPIGD& other) const; bool operator==(UPnPIGD& other) const;
...@@ -69,6 +71,7 @@ private: ...@@ -69,6 +71,7 @@ private:
std::string friendlyName_ {}; std::string friendlyName_ {};
std::string serviceType_ {}; std::string serviceType_ {};
std::string serviceId_ {}; std::string serviceId_ {};
std::string locationURL_ {};
std::string controlURL_ {}; std::string controlURL_ {};
std::string eventSubURL_ {}; std::string eventSubURL_ {};
}; };
......
...@@ -73,10 +73,10 @@ public: ...@@ -73,10 +73,10 @@ public:
virtual Type getType() const = 0; virtual Type getType() const = 0;
// Clear all known IGDs. // Clear all known IGDs.
virtual void clearIGDs() = 0; virtual void clearIgds() = 0;
// Search for IGD. // Search for IGD.
virtual void searchForIGD() = 0; virtual void searchForIgd() = 0;
// Tries to add mapping. Assumes mutex is already locked. // Tries to add mapping. Assumes mutex is already locked.
virtual Mapping addMapping(IGD* igd, uint16_t port_external, uint16_t port_internal, PortType type, UPnPProtocol::UpnpError& upnp_error) = 0; virtual Mapping addMapping(IGD* igd, uint16_t port_external, uint16_t port_internal, PortType type, UPnPProtocol::UpnpError& upnp_error) = 0;
......
...@@ -48,13 +48,13 @@ UPnPContext::UPnPContext() ...@@ -48,13 +48,13 @@ UPnPContext::UPnPContext()
#if HAVE_LIBNATPMP #if HAVE_LIBNATPMP
auto natPmp = std::make_unique<NatPmp>(); auto natPmp = std::make_unique<NatPmp>();
natPmp->setOnIgdChanged(std::bind(&UPnPContext::igdListChanged, this, _1, _2, _3, _4)); natPmp->setOnIgdChanged(std::bind(&UPnPContext::igdListChanged, this, _1, _2, _3, _4));
natPmp->searchForIGD(); natPmp->searchForIgd();
protocolList_.push_back(std::move(natPmp)); protocolList_.push_back(std::move(natPmp));
#endif #endif
#if HAVE_LIBUPNP #if HAVE_LIBUPNP
auto pupnp = std::make_unique<PUPnP>(); auto pupnp = std::make_unique<PUPnP>();
pupnp->setOnIgdChanged(std::bind(&UPnPContext::igdListChanged, this, _1, _2, _3, _4)); pupnp->setOnIgdChanged(std::bind(&UPnPContext::igdListChanged, this, _1, _2, _3, _4));
pupnp->searchForIGD(); pupnp->searchForIgd();
protocolList_.push_back(std::move(pupnp)); protocolList_.push_back(std::move(pupnp));
#endif #endif
} }
...@@ -70,7 +70,7 @@ UPnPContext::connectivityChanged() ...@@ -70,7 +70,7 @@ UPnPContext::connectivityChanged()
{ {
std::lock_guard<std::mutex> lock(igdListMutex_); std::lock_guard<std::mutex> lock(igdListMutex_);
for (auto const& protocol : protocolList_) for (auto const& protocol : protocolList_)
protocol->clearIGDs(); protocol->clearIgds();
if (not igdList_.empty()) { if (not igdList_.empty()) {
// Clear main IGD list. // Clear main IGD list.
igdList_.clear(); igdList_.clear();
...@@ -81,7 +81,7 @@ UPnPContext::connectivityChanged() ...@@ -81,7 +81,7 @@ UPnPContext::connectivityChanged()
} }
for (auto const& protocol : protocolList_) for (auto const& protocol : protocolList_)
protocol->searchForIGD(); protocol->searchForIgd();
} }
bool bool
...@@ -306,6 +306,7 @@ UPnPContext::addIgdToList(UPnPProtocol* protocol, IGD* igd) ...@@ -306,6 +306,7 @@ UPnPContext::addIgdToList(UPnPProtocol* protocol, IGD* igd)
} }
if (isIgdInList(igd->publicIp_)) { if (isIgdInList(igd->publicIp_)) {
JAMI_DBG("UPnPContext: IGD with public IP %s is already in the list", igd->publicIp_.toString().c_str());
return false; return false;
} }
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment