From b26e9d033800094af3fa2a0b85ccd8d3faf51cf2 Mon Sep 17 00:00:00 2001 From: Benjamin Sergeant Date: Wed, 19 Jun 2019 00:43:59 -0700 Subject: [PATCH] IXDNSLookup. Uses weak pointer + smart_ptr + shared_from_this instead of static sets + mutex to handle object going away before dns lookup has resolved --- CHANGELOG.md | 1 + ixwebsocket/IXDNSLookup.cpp | 47 +++++++++------------------------ ixwebsocket/IXDNSLookup.h | 12 +++------ ixwebsocket/IXSocketConnect.cpp | 4 +-- makefile | 4 +-- test/IXDNSLookupTest.cpp | 12 ++++----- 6 files changed, 28 insertions(+), 52 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1d96271f..26fd9850 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ All notable changes to this project will be documented in this file. ## [unreleased] - 2019-06-09 ### Changed +- IXDNSLookup. Uses weak pointer + smart_ptr + shared_from_this instead of static sets + mutex to handle object going away before dns lookup has resolved - cobra_to_sentry / backtraces are reversed and line number is not extracted correctly - mbedtls and zlib are searched with find_package, and we use the vendored version if nothing is found - travis CI uses g++ on Linux diff --git a/ixwebsocket/IXDNSLookup.cpp b/ixwebsocket/IXDNSLookup.cpp index 139ab4be..f7e767c1 100644 --- a/ixwebsocket/IXDNSLookup.cpp +++ b/ixwebsocket/IXDNSLookup.cpp @@ -14,27 +14,15 @@ namespace ix { const int64_t DNSLookup::kDefaultWait = 10; // ms - std::atomic DNSLookup::_nextId(0); - std::set DNSLookup::_activeJobs; - std::mutex DNSLookup::_activeJobsMutex; - DNSLookup::DNSLookup(const std::string& hostname, int port, int64_t wait) : _port(port), _wait(wait), _res(nullptr), - _done(false), - _id(_nextId++) + _done(false) { setHostname(hostname); } - DNSLookup::~DNSLookup() - { - // Remove this job from the active jobs list - std::lock_guard lock(_activeJobsMutex); - _activeJobs.erase(_id); - } - struct addrinfo* DNSLookup::getAddrInfo(const std::string& hostname, int port, std::string& errMsg) @@ -94,17 +82,14 @@ namespace ix // if you need a second lookup. } - // Record job in the active Job set - { - std::lock_guard lock(_activeJobsMutex); - _activeJobs.insert(_id); - } - // // Good resource on thread forced termination // https://www.bo-yang.net/2017/11/19/cpp-kill-detached-thread // - _thread = std::thread(&DNSLookup::run, this, _id, getHostname(), _port); + auto ptr = shared_from_this(); + std::weak_ptr self(ptr); + + _thread = std::thread(&DNSLookup::run, this, self, getHostname(), _port); _thread.detach(); std::unique_lock lock(_conditionVariableMutex); @@ -138,7 +123,7 @@ namespace ix return getRes(); } - void DNSLookup::run(uint64_t id, const std::string& hostname, int port) // thread runner + void DNSLookup::run(std::weak_ptr self, const std::string& hostname, int port) // thread runner { // We don't want to read or write into members variables of an object that could be // gone, so we use temporary variables (res) or we pass in by copy everything that @@ -146,21 +131,15 @@ namespace ix std::string errMsg; struct addrinfo* res = getAddrInfo(hostname, port, errMsg); - // if this isn't an active job, and the control thread is gone - // there is nothing to do, and we don't want to touch the defunct - // object data structure such as _errMsg or _condition - std::lock_guard lock(_activeJobsMutex); - if (_activeJobs.count(id) == 0) + if (self.lock()) { - return; + // Copy result into the member variables + setRes(res); + setErrMsg(errMsg); + + _condition.notify_one(); + _done = true; } - - // Copy result into the member variables - setRes(res); - setErrMsg(errMsg); - - _condition.notify_one(); - _done = true; } void DNSLookup::setHostname(const std::string& hostname) diff --git a/ixwebsocket/IXDNSLookup.h b/ixwebsocket/IXDNSLookup.h index bc502309..4268a992 100644 --- a/ixwebsocket/IXDNSLookup.h +++ b/ixwebsocket/IXDNSLookup.h @@ -16,16 +16,17 @@ #include #include #include +#include struct addrinfo; namespace ix { - class DNSLookup + class DNSLookup : public std::enable_shared_from_this { public: DNSLookup(const std::string& hostname, int port, int64_t wait = DNSLookup::kDefaultWait); - ~DNSLookup(); + ~DNSLookup() = default; struct addrinfo* resolve(std::string& errMsg, const CancellationRequest& isCancellationRequested, @@ -41,7 +42,7 @@ namespace ix int port, std::string& errMsg); - void run(uint64_t id, const std::string& hostname, int port); // thread runner + void run(std::weak_ptr self, const std::string& hostname, int port); // thread runner void setHostname(const std::string& hostname); const std::string& getHostname(); @@ -69,11 +70,6 @@ namespace ix std::condition_variable _condition; std::mutex _conditionVariableMutex; - uint64_t _id; - static std::atomic _nextId; - static std::set _activeJobs; - static std::mutex _activeJobsMutex; - const static int64_t kDefaultWait; }; } // namespace ix diff --git a/ixwebsocket/IXSocketConnect.cpp b/ixwebsocket/IXSocketConnect.cpp index 2c19871b..3c71eef0 100644 --- a/ixwebsocket/IXSocketConnect.cpp +++ b/ixwebsocket/IXSocketConnect.cpp @@ -128,8 +128,8 @@ namespace ix // // First do DNS resolution // - DNSLookup dnsLookup(hostname, port); - struct addrinfo *res = dnsLookup.resolve(errMsg, isCancellationRequested); + auto dnsLookup = std::make_shared(hostname, port); + struct addrinfo *res = dnsLookup->resolve(errMsg, isCancellationRequested); if (res == nullptr) { return -1; diff --git a/makefile b/makefile index 30b3a299..ea55fe54 100644 --- a/makefile +++ b/makefile @@ -9,10 +9,10 @@ install: brew # on osx it is good practice to make /usr/local user writable # sudo chown -R `whoami`/staff /usr/local brew: - mkdir -p build && (cd build ; cmake -DUSE_TLS=1 -DUSE_WS=1 .. ; make -j install) + mkdir -p build && (cd build ; cmake -DCMAKE_BUILD_TYPE=Debug -DUSE_TLS=1 -DUSE_WS=1 .. ; make -j install) ws: - mkdir -p build && (cd build ; cmake -DUSE_TLS=1 -DUSE_WS=1 -DUSE_MBED_TLS=1 -DUSE_VENDORED_THIRD_PARTY=1 .. ; make -j) + mkdir -p build && (cd build ; cmake -DCMAKE_BUILD_TYPE=Debug -DUSE_TLS=1 -DUSE_WS=1 -DUSE_MBED_TLS=1 -DUSE_VENDORED_THIRD_PARTY=1 .. ; make -j) uninstall: xargs rm -fv < build/install_manifest.txt diff --git a/test/IXDNSLookupTest.cpp b/test/IXDNSLookupTest.cpp index 0c3617d0..4086633e 100644 --- a/test/IXDNSLookupTest.cpp +++ b/test/IXDNSLookupTest.cpp @@ -17,33 +17,33 @@ TEST_CASE("dns", "[net]") { SECTION("Test resolving a known hostname") { - DNSLookup dnsLookup("www.google.com", 80); + auto dnsLookup = std::make_shared("www.google.com", 80); std::string errMsg; struct addrinfo* res; - res = dnsLookup.resolve(errMsg, [] { return false; }); + res = dnsLookup->resolve(errMsg, [] { return false; }); std::cerr << "Error message: " << errMsg << std::endl; REQUIRE(res != nullptr); } SECTION("Test resolving a non-existing hostname") { - DNSLookup dnsLookup("wwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwww", 80); + auto dnsLookup = std::make_shared("wwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwww", 80); std::string errMsg; - struct addrinfo* res = dnsLookup.resolve(errMsg, [] { return false; }); + struct addrinfo* res = dnsLookup->resolve(errMsg, [] { return false; }); std::cerr << "Error message: " << errMsg << std::endl; REQUIRE(res == nullptr); } SECTION("Test resolving a good hostname, with cancellation") { - DNSLookup dnsLookup("www.google.com", 80, 1); + auto dnsLookup = std::make_shared("www.google.com", 80, 1); std::string errMsg; // The callback returning true means we are requesting cancellation - struct addrinfo* res = dnsLookup.resolve(errMsg, [] { return true; }); + struct addrinfo* res = dnsLookup->resolve(errMsg, [] { return true; }); std::cerr << "Error message: " << errMsg << std::endl; REQUIRE(res == nullptr); }