From a8281ff9f94274cba9bbcdcd90ce5093db411511 Mon Sep 17 00:00:00 2001 From: Jon Siwek Date: Wed, 1 May 2019 22:42:10 -0700 Subject: [PATCH 1/6] Fix a ref counnting bug in DNS_Mgr --- src/DNS_Mgr.cc | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/DNS_Mgr.cc b/src/DNS_Mgr.cc index b92c057eba..3be59981a7 100644 --- a/src/DNS_Mgr.cc +++ b/src/DNS_Mgr.cc @@ -289,10 +289,13 @@ ListVal* DNS_Mapping::Addrs() TableVal* DNS_Mapping::AddrsSet() { ListVal* l = Addrs(); - if ( l ) - return l->ConvertToSet(); - else + + if ( ! l ) return empty_addr_set(); + + auto rval = l->ConvertToSet(); + Unref(l); + return rval; } StringVal* DNS_Mapping::Host() From 6db576195c4417bac663a05a12bd4b712c47ff2a Mon Sep 17 00:00:00 2001 From: Jon Siwek Date: Wed, 1 May 2019 22:46:10 -0700 Subject: [PATCH 2/6] Improve DNS_Mgr I/O loop: prevent starvation due to busy Broker --- src/DNS_Mgr.cc | 15 +++++++++++++-- src/DNS_Mgr.h | 1 + 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/src/DNS_Mgr.cc b/src/DNS_Mgr.cc index 3be59981a7..11f1e30037 100644 --- a/src/DNS_Mgr.cc +++ b/src/DNS_Mgr.cc @@ -392,6 +392,7 @@ DNS_Mgr::DNS_Mgr(DNS_MgrMode arg_mode) successful = 0; failed = 0; nb_dns = nullptr; + next_timestamp = -1.0; } DNS_Mgr::~DNS_Mgr() @@ -1252,8 +1253,17 @@ void DNS_Mgr::GetFds(iosource::FD_Set* read, iosource::FD_Set* write, double DNS_Mgr::NextTimestamp(double* network_time) { - // This is kind of cheating ... - return asyncs_timeouts.size() ? timer_mgr->Time() : -1.0; + if ( asyncs_timeouts.empty() ) + // No pending requests. + return -1.0; + + if ( next_timestamp < 0 ) + // Store the timestamp to help prevent starvation by some other + // IOSource always trying to use the same timestamp + // (assuming network_time does actually increase). + next_timestamp = timer_mgr->Time(); + + return next_timestamp; } void DNS_Mgr::CheckAsyncAddrRequest(const IPAddr& addr, bool timeout) @@ -1382,6 +1392,7 @@ void DNS_Mgr::Flush() void DNS_Mgr::Process() { DoProcess(false); + next_timestamp = -1.0; } void DNS_Mgr::DoProcess(bool flush) diff --git a/src/DNS_Mgr.h b/src/DNS_Mgr.h index f6f62bd1ec..7c7ddc8738 100644 --- a/src/DNS_Mgr.h +++ b/src/DNS_Mgr.h @@ -236,6 +236,7 @@ protected: unsigned long num_requests; unsigned long successful; unsigned long failed; + double next_timestamp; }; extern DNS_Mgr* dns_mgr; From 5bccb44ad4b11d6f141e440e7a2c2cd6d1c711ba Mon Sep 17 00:00:00 2001 From: Jon Siwek Date: Wed, 1 May 2019 22:50:47 -0700 Subject: [PATCH 3/6] Remove dead code from DNS_Mgr --- src/DNS_Mgr.cc | 8 ++++---- src/DNS_Mgr.h | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/DNS_Mgr.cc b/src/DNS_Mgr.cc index 11f1e30037..db8100ca2b 100644 --- a/src/DNS_Mgr.cc +++ b/src/DNS_Mgr.cc @@ -1369,7 +1369,7 @@ void DNS_Mgr::CheckAsyncHostRequest(const char* host, bool timeout) void DNS_Mgr::Flush() { - DoProcess(false); + DoProcess(); HostMap::iterator it; for ( it = host_mappings.begin(); it != host_mappings.end(); ++it ) @@ -1391,11 +1391,11 @@ void DNS_Mgr::Flush() void DNS_Mgr::Process() { - DoProcess(false); + DoProcess(); next_timestamp = -1.0; } -void DNS_Mgr::DoProcess(bool flush) +void DNS_Mgr::DoProcess() { if ( ! nb_dns ) return; @@ -1404,7 +1404,7 @@ void DNS_Mgr::DoProcess(bool flush) { AsyncRequest* req = asyncs_timeouts.top(); - if ( req->time + DNS_TIMEOUT > current_time() || flush ) + if ( req->time + DNS_TIMEOUT > current_time() ) break; if ( req->IsAddrReq() ) diff --git a/src/DNS_Mgr.h b/src/DNS_Mgr.h index 7c7ddc8738..7fa805461c 100644 --- a/src/DNS_Mgr.h +++ b/src/DNS_Mgr.h @@ -132,7 +132,7 @@ protected: void CheckAsyncTextRequest(const char* host, bool timeout); // Process outstanding requests. - void DoProcess(bool flush); + void DoProcess(); // IOSource interface. void GetFds(iosource::FD_Set* read, iosource::FD_Set* write, From 5bb2a6b1c0d12a4000b55938a26e4c1e51f86d97 Mon Sep 17 00:00:00 2001 From: Jon Siwek Date: Wed, 1 May 2019 22:51:54 -0700 Subject: [PATCH 4/6] Fix DNS_Mgr priority_queue usage It was sorting by memory address stored in AsyncRequest pointers rather than their actual timestamp. --- src/DNS_Mgr.h | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/DNS_Mgr.h b/src/DNS_Mgr.h index 7fa805461c..5aac420303 100644 --- a/src/DNS_Mgr.h +++ b/src/DNS_Mgr.h @@ -228,7 +228,14 @@ protected: typedef list QueuedList; QueuedList asyncs_queued; - typedef priority_queue TimeoutQueue; + struct AsyncRequestCompare { + bool operator()(const AsyncRequest* a, const AsyncRequest* b) + { + return a->time > b->time; + } + }; + + typedef priority_queue, AsyncRequestCompare> TimeoutQueue; TimeoutQueue asyncs_timeouts; int asyncs_pending; From fd11c63efe94ae6967bb7a31e03a7aea556d9686 Mon Sep 17 00:00:00 2001 From: Jon Siwek Date: Wed, 1 May 2019 22:55:43 -0700 Subject: [PATCH 5/6] Remove an unhelpful/optimistic DNS_Mgr optimization DNS_Mgr is always "idle", so Process() is always called when the fd signals there's really something ready (except when flushing at termination-time), so checking whether all pending request maps are empty within Process() doesn't help much. If they are empty, but there's somehow something to pull off the socket, the main loop is just going to keep trying to call Process() until it gets read (which would be bad if it's preventing another IOSource from getting real work done). --- src/DNS_Mgr.cc | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/DNS_Mgr.cc b/src/DNS_Mgr.cc index db8100ca2b..4edff2088c 100644 --- a/src/DNS_Mgr.cc +++ b/src/DNS_Mgr.cc @@ -1418,9 +1418,6 @@ void DNS_Mgr::DoProcess() delete req; } - if ( asyncs_addrs.size() == 0 && asyncs_names.size() == 0 && asyncs_texts.size() == 0 ) - return; - if ( AnswerAvailable(0) <= 0 ) return; From 46799f75407391a1caa65c99f6a4b87afa3ba56a Mon Sep 17 00:00:00 2001 From: Jon Siwek Date: Wed, 1 May 2019 23:08:52 -0700 Subject: [PATCH 6/6] Fix timing out DNS lookups that were already resolved This could happen in the case of making repeated lookup requests for the same thing within a short period of time: cleaning up an old request that already got resolved would mistakenly see a new, yet-to-be-resolved request with identical host/addr and mistakenly assume it's in need of being timed out. --- src/DNS_Mgr.cc | 15 +++++++++------ src/DNS_Mgr.h | 8 ++++++-- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/src/DNS_Mgr.cc b/src/DNS_Mgr.cc index 4edff2088c..c52e6086f4 100644 --- a/src/DNS_Mgr.cc +++ b/src/DNS_Mgr.cc @@ -1407,12 +1407,15 @@ void DNS_Mgr::DoProcess() if ( req->time + DNS_TIMEOUT > current_time() ) break; - if ( req->IsAddrReq() ) - CheckAsyncAddrRequest(req->host, true); - else if ( req->is_txt ) - CheckAsyncTextRequest(req->name.c_str(), true); - else - CheckAsyncHostRequest(req->name.c_str(), true); + if ( ! req->processed ) + { + if ( req->IsAddrReq() ) + CheckAsyncAddrRequest(req->host, true); + else if ( req->is_txt ) + CheckAsyncTextRequest(req->name.c_str(), true); + else + CheckAsyncHostRequest(req->name.c_str(), true); + } asyncs_timeouts.pop(); delete req; diff --git a/src/DNS_Mgr.h b/src/DNS_Mgr.h index 5aac420303..9f9fe4ccc3 100644 --- a/src/DNS_Mgr.h +++ b/src/DNS_Mgr.h @@ -172,12 +172,13 @@ protected: struct AsyncRequest { double time; + bool is_txt; + bool processed; IPAddr host; string name; - bool is_txt; CallbackList callbacks; - AsyncRequest() : time(0.0), is_txt(false) { } + AsyncRequest() : time(0.0), is_txt(false), processed(false) { } bool IsAddrReq() const { return name.length() == 0; } @@ -190,6 +191,7 @@ protected: delete *i; } callbacks.clear(); + processed = true; } void Resolved(TableVal* addrs) @@ -201,6 +203,7 @@ protected: delete *i; } callbacks.clear(); + processed = true; } void Timeout() @@ -212,6 +215,7 @@ protected: delete *i; } callbacks.clear(); + processed = true; } };