kapitan/IXWebSocket
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

IXDNSLookup. Uses weak pointer + smart_ptr + shared_from_this instead of static sets + mutex to handle object going away before dns lookup has resolved

Browse Source
This commit is contained in:
Benjamin Sergeant 2019-06-19 00:43:59 -07:00
parent 2d02ae0f0c
commit 43deaba547
6 changed files with 28 additions and 52 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
CHANGELOG.md
View File

@ -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

47
ixwebsocket/IXDNSLookup.cpp
View File

@ -14,27 +14,15 @@ namespace ix
{
const int64_t DNSLookup::kDefaultWait = 10; // ms
std::atomic<uint64_t> DNSLookup::_nextId(0);
std::set<uint64_t> 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<std::mutex> 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<std::mutex> 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<DNSLookup> self(ptr);
_thread = std::thread(&DNSLookup::run, this, self, getHostname(), _port);
_thread.detach();
std::unique_lock<std::mutex> 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<DNSLookup> 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<std::mutex> 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)

12
ixwebsocket/IXDNSLookup.h
View File

@ -16,16 +16,17 @@
#include <set>
#include <string>
#include <thread>
#include <memory>
struct addrinfo;
namespace ix
{
class DNSLookup
class DNSLookup : public std::enable_shared_from_this<DNSLookup>
{
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<DNSLookup> 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<uint64_t> _nextId;
static std::set<uint64_t> _activeJobs;
static std::mutex _activeJobsMutex;
const static int64_t kDefaultWait;
};
} // namespace ix

4
ixwebsocket/IXSocketConnect.cpp
View File

@ -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<DNSLookup>(hostname, port);
struct addrinfo *res = dnsLookup->resolve(errMsg, isCancellationRequested);
if (res == nullptr)
{
return -1;

4
makefile
View File

@ -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

12
test/IXDNSLookupTest.cpp
View File

@ -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<DNSLookup>("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<DNSLookup>("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<DNSLookup>("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);
}