Repository: trafficserver Updated Branches: refs/heads/master 3838cf2c1 -> c36204d5a
TS-3774: Fix memory leak issue in host file parsing for HostDB. Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo Commit: http://git-wip-us.apache.org/repos/asf/trafficserver/commit/c36204d5 Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/c36204d5 Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/c36204d5 Branch: refs/heads/master Commit: c36204d5abaf3d4c335dd885ace1e5d58b57d451 Parents: 3838cf2 Author: Alan M. Carroll <[email protected]> Authored: Tue Jul 21 11:10:05 2015 -0500 Committer: Alan M. Carroll <[email protected]> Committed: Thu Jul 23 15:43:50 2015 -0500 ---------------------------------------------------------------------- iocore/hostdb/HostDB.cc | 39 +++++++++++++++++++--------------- iocore/hostdb/I_HostDBProcessor.h | 7 +++++- iocore/hostdb/P_HostDBProcessor.h | 4 +++- 3 files changed, 31 insertions(+), 19 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/trafficserver/blob/c36204d5/iocore/hostdb/HostDB.cc ---------------------------------------------------------------------- diff --git a/iocore/hostdb/HostDB.cc b/iocore/hostdb/HostDB.cc index bb438a1..0df0cd8 100644 --- a/iocore/hostdb/HostDB.cc +++ b/iocore/hostdb/HostDB.cc @@ -662,19 +662,13 @@ HostDBInfo * probe(ProxyMutex *mutex, HostDBMD5 const &md5, bool ignore_timeout) { // If we have an entry in our hosts file, we don't need to bother with DNS + // Make a local copy/reference so we don't have to worry about updates. Ptr<RefCountedHostsFileMap> current_host_file_map = hostDB.hosts_file_ptr; ts::ConstBuffer hname(md5.host_name, md5.host_len); HostsFileMap::iterator find_result = current_host_file_map->hosts_file_map.find(hname); if (find_result != current_host_file_map->hosts_file_map.end()) { - // TODO: Something to make this not local :/ - static HostDBInfo r; - r.round_robin = false; - r.round_robin_elt = false; - r.reverse_dns = false; - r.is_srv = false; - ats_ip_set(r.ip(), (*find_result).second); - return &r; + return &(find_result->second); } ink_assert(this_ethread() == hostDB.lock_for_bucket((int)(fold_md5(md5.hash) % hostDB.buckets))->thread_holding); @@ -2731,8 +2725,6 @@ HostDBFileContinuation::destroy() // proceeding at a time in any case so we might as well make these // globals. int HostDBFileUpdateActive = 0; -// map of hostname -> IpAddr -Ptr<RefCountedHostsFileMap> parsed_hosts_file_ptr(new RefCountedHostsFileMap()); // Actual ordering doesn't matter as long as it's consistent. bool @@ -2743,7 +2735,7 @@ CmpMD5(INK_MD5 const &lhs, INK_MD5 const &rhs) void -ParseHostLine(char *l) +ParseHostLine(RefCountedHostsFileMap *map, char *l) { Tokenizer elts(" \t"); int n_elts = elts.Initialize(l, SHARE_TOKS); @@ -2754,8 +2746,13 @@ ParseHostLine(char *l) for (int i = 1; i < n_elts; ++i) { ts::ConstBuffer name(elts[i], strlen(elts[i])); // If we don't have an entry already (host files only support single IPs for a given name) - if (parsed_hosts_file_ptr->hosts_file_map.find(name) == parsed_hosts_file_ptr->hosts_file_map.end()) { - parsed_hosts_file_ptr->hosts_file_map[name] = ip; + if (map->hosts_file_map.find(name) == map->hosts_file_map.end()) { + HostsFileMap::mapped_type& item = map->hosts_file_map[name]; + item.round_robin = false; + item.round_robin_elt = false; + item.reverse_dns = false; + item.is_srv = false; + ats_ip_set(item.ip(), ip); } } } @@ -2765,6 +2762,8 @@ ParseHostLine(char *l) void ParseHostFile(char const *path) { + Ptr<RefCountedHostsFileMap> parsed_hosts_file_ptr; + // Test and set for update in progress. if (0 != ink_atomic_swap(&HostDBFileUpdateActive, 1)) { Debug("hostdb", "Skipped load of host file because update already in progress"); @@ -2780,6 +2779,7 @@ ParseHostFile(char const *path) // +1 in case no terminating newline int64_t size = info.st_size + 1; + parsed_hosts_file_ptr = new RefCountedHostsFileMap; parsed_hosts_file_ptr->HostFileText = static_cast<char *>(ats_malloc(size)); if (parsed_hosts_file_ptr->HostFileText) { char *base = parsed_hosts_file_ptr->HostFileText; @@ -2805,7 +2805,7 @@ ParseHostFile(char const *path) while (base < spot && isspace(*base)) ++base; // skip leading ws if (*base != '#' && base < spot) // non-empty non-comment line - ParseHostLine(base); + ParseHostLine(parsed_hosts_file_ptr, base); base = spot + 1; } @@ -2815,10 +2815,15 @@ ParseHostFile(char const *path) } } - // Swap out hostDB's map for ours + // Rotate the host file maps down. We depend on two assumptions here - + // 1) Host files are not updated frequently. Even if updated via traffic_ctl that will be at least 5-10 seconds between reloads. + // 2) The HostDB clients (essentially the HttpSM) copies the HostDB record or data over to local storage during event processing, + // which means the data only has to be valid until the event chaining rolls back up to the event processor. This will certainly + // be less than 1s + // The combination of these means keeping one file back in the rotation is sufficient to keep any outstanding references valid until + // dropped. + hostDB.prev_hosts_file_ptr = hostDB.hosts_file_ptr; hostDB.hosts_file_ptr = parsed_hosts_file_ptr; - // Make a new map, so we can do it all again - parsed_hosts_file_ptr = new RefCountedHostsFileMap(); // Mark this one as completed, so we can allow another update to happen HostDBFileUpdateActive = 0; } http://git-wip-us.apache.org/repos/asf/trafficserver/blob/c36204d5/iocore/hostdb/I_HostDBProcessor.h ---------------------------------------------------------------------- diff --git a/iocore/hostdb/I_HostDBProcessor.h b/iocore/hostdb/I_HostDBProcessor.h index abb0997..abbbc55 100644 --- a/iocore/hostdb/I_HostDBProcessor.h +++ b/iocore/hostdb/I_HostDBProcessor.h @@ -157,12 +157,17 @@ struct HostDBInfo { char *hostname(); char *srvname(HostDBRoundRobin *rr); - /// Check if this entry is a round robin entry. + /// Check if this entry is the root of a round robin entry. + /// If @c true then this entry needs to be converted to a specific element of the round robin to be used. bool is_rr() const { return 0 != round_robin; } + /// Check if this entry is an element of a round robin entry. + /// If @c true then this entry is part of and was obtained from a round robin root. This is useful if the + /// address doesn't work - a retry can probably get a new address by doing another lookup and resolving to + /// a different element of the round robin. bool is_rr_elt() const { http://git-wip-us.apache.org/repos/asf/trafficserver/blob/c36204d5/iocore/hostdb/P_HostDBProcessor.h ---------------------------------------------------------------------- diff --git a/iocore/hostdb/P_HostDBProcessor.h b/iocore/hostdb/P_HostDBProcessor.h index 5cfa025..22b095d 100644 --- a/iocore/hostdb/P_HostDBProcessor.h +++ b/iocore/hostdb/P_HostDBProcessor.h @@ -196,7 +196,7 @@ struct CmpConstBuffferCaseInsensitive { }; // Our own typedef for the host file mapping -typedef std::map<ts::ConstBuffer, IpAddr, CmpConstBuffferCaseInsensitive> HostsFileMap; +typedef std::map<ts::ConstBuffer, HostDBInfo, CmpConstBuffferCaseInsensitive> HostsFileMap; // A to hold a ref-counted map struct RefCountedHostsFileMap : public RefCountObj { HostsFileMap hosts_file_map; @@ -225,6 +225,8 @@ struct HostDBCache : public MultiCache<HostDBInfo> { // Map to contain all of the host file overrides, initialize it to empty Ptr<RefCountedHostsFileMap> hosts_file_ptr; + // Double buffer the hosts file becase it's small and it solves dangling reference problems. + Ptr<RefCountedHostsFileMap> prev_hosts_file_ptr; Queue<HostDBContinuation, Continuation::Link_link> pending_dns[MULTI_CACHE_PARTITIONS]; Queue<HostDBContinuation, Continuation::Link_link> &pending_dns_for_hash(INK_MD5 &md5);
