You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@trafficserver.apache.org by jp...@apache.org on 2016/11/29 04:31:15 UTC
[trafficserver] branch master updated (b8f0669 -> e5257fc)
This is an automated email from the ASF dual-hosted git repository.
jpeach pushed a change to branch master
in repository https://git-dual.apache.org/repos/asf/trafficserver.git.
from b8f0669 TS-5063: Fixes the strlcpy that I missed
new 23672db TS-5065: Fix RefCountCache iterator invalidation.
new 4a380d9 TS-5066: Fix HostDB memory leaks on serialization failure.
new e5257fc Improve HostDB serialization warnings.
The 3 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "adds" were already present in the repository and have only
been added to this reference.
Summary of changes:
iocore/hostdb/HostDB.cc | 4 +-
iocore/hostdb/P_RefCountCache.h | 82 +++++++++++++++++++++----------
iocore/hostdb/P_RefCountCacheSerializer.h | 40 ++++++++-------
iocore/hostdb/RefCountCache.cc | 14 +++++-
4 files changed, 94 insertions(+), 46 deletions(-)
--
To stop receiving notification emails like this one, please contact
['"commits@trafficserver.apache.org" <co...@trafficserver.apache.org>'].
[trafficserver] 03/03: Improve HostDB serialization warnings.
Posted by jp...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
jpeach pushed a commit to branch master
in repository https://git-dual.apache.org/repos/asf/trafficserver.git
commit e5257fc0bd72669bc90a88ec0391f8f6f6215d7a
Author: James Peach <jp...@apache.org>
AuthorDate: Sat Nov 26 12:41:37 2016 -0800
Improve HostDB serialization warnings.
---
iocore/hostdb/P_RefCountCacheSerializer.h | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/iocore/hostdb/P_RefCountCacheSerializer.h b/iocore/hostdb/P_RefCountCacheSerializer.h
index 4004f37..853fd4e 100644
--- a/iocore/hostdb/P_RefCountCacheSerializer.h
+++ b/iocore/hostdb/P_RefCountCacheSerializer.h
@@ -168,7 +168,7 @@ RefCountCacheSerializer<C>::write_partition(int /* event */, Event *e)
// Write the RefCountCacheItemMeta (as our header)
int ret = this->write_to_disk((char *)&entry->meta, sizeof(entry->meta));
if (ret < 0) {
- Warning("Error writing cache item header to %s: %d", this->tmp_filename.c_str(), ret);
+ Warning("Error writing cache item header to %s: %s", this->tmp_filename.c_str(), strerror(-ret));
delete this;
return EVENT_DONE;
}
@@ -176,7 +176,7 @@ RefCountCacheSerializer<C>::write_partition(int /* event */, Event *e)
// write the actual object now
ret = this->write_to_disk((char *)entry->item.get(), entry->meta.size);
if (ret < 0) {
- Warning("Error writing cache item to %s: %d", this->tmp_filename.c_str(), ret);
+ Warning("Error writing cache item to %s: %s", this->tmp_filename.c_str(), strerror(-ret));
delete this;
return EVENT_DONE;
}
@@ -229,8 +229,7 @@ RefCountCacheSerializer<C>::initialize_storage(int /* event */, Event *e)
{
this->fd = socketManager.open(this->tmp_filename.c_str(), O_TRUNC | O_RDWR | O_CREAT, 0644); // TODO: configurable perms
if (this->fd == -1) {
- Warning("Unable to create temporary file %s, unable to persist hostdb: %d :%s\n", this->tmp_filename.c_str(), this->fd,
- strerror(errno));
+ Warning("Unable to create temporary file %s, unable to persist hostdb: %s", this->tmp_filename.c_str(), strerror(errno));
delete this;
return EVENT_DONE;
}
@@ -238,7 +237,7 @@ RefCountCacheSerializer<C>::initialize_storage(int /* event */, Event *e)
// Write out the header
int ret = this->write_to_disk((char *)&this->cache->get_header(), sizeof(RefCountCacheHeader));
if (ret < 0) {
- Warning("Error writing cache header to %s: %d", this->tmp_filename.c_str(), ret);
+ Warning("Error writing cache header to %s: %s", this->tmp_filename.c_str(), strerror(-ret));
delete this;
return EVENT_DONE;
}
@@ -314,7 +313,7 @@ RefCountCacheSerializer<C>::write_to_disk(const void *ptr, size_t n_bytes)
while (written < n_bytes) {
int ret = socketManager.write(this->fd, (char *)ptr + written, n_bytes - written);
if (ret <= 0) {
- return -1;
+ return ret;
} else {
written += ret;
}
--
To stop receiving notification emails like this one, please contact
"commits@trafficserver.apache.org" <co...@trafficserver.apache.org>.
[trafficserver] 02/03: TS-5066: Fix HostDB memory leaks on
serialization failure.
Posted by jp...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
jpeach pushed a commit to branch master
in repository https://git-dual.apache.org/repos/asf/trafficserver.git
commit 4a380d9d683d425cb7b2af30e1d57f0260ab829a
Author: James Peach <jp...@apache.org>
AuthorDate: Sat Nov 26 11:47:04 2016 -0800
TS-5066: Fix HostDB memory leaks on serialization failure.
If the serializer fails to write the partition, the copied entries were
being leaked. Clean up the cache entry allocation to centralize the
pain of it and allow both the cache and the serializer to share the same
allocation and free helpers.
---
iocore/hostdb/HostDB.cc | 4 ++--
iocore/hostdb/P_RefCountCache.h | 28 +++++++++++++++++++---------
iocore/hostdb/P_RefCountCacheSerializer.h | 29 ++++++++++++++++++-----------
iocore/hostdb/RefCountCache.cc | 14 +++++++++++++-
4 files changed, 52 insertions(+), 23 deletions(-)
diff --git a/iocore/hostdb/HostDB.cc b/iocore/hostdb/HostDB.cc
index 54238e7..f31fa37 100644
--- a/iocore/hostdb/HostDB.cc
+++ b/iocore/hostdb/HostDB.cc
@@ -276,8 +276,8 @@ struct HostDBSync : public HostDBBackgroundTask {
SET_HANDLER(&HostDBSync::wait_event);
start_time = Thread::get_hrtime();
- new RefCountCacheSerializer<RefCountCache<HostDBInfo>>(this, hostDBProcessor.cache()->refcountcache, this->frequency,
- this->storage_path, this->full_path);
+ new RefCountCacheSerializer<HostDBInfo>(this, hostDBProcessor.cache()->refcountcache, this->frequency, this->storage_path,
+ this->full_path);
return EVENT_DONE;
}
};
diff --git a/iocore/hostdb/P_RefCountCache.h b/iocore/hostdb/P_RefCountCache.h
index 15ed326..4b7998e 100644
--- a/iocore/hostdb/P_RefCountCache.h
+++ b/iocore/hostdb/P_RefCountCache.h
@@ -94,10 +94,25 @@ public:
{
return this->meta.expiry_time < v2.meta.expiry_time;
}
+
+ static RefCountCacheHashEntry *alloc();
+ static void dealloc(RefCountCacheHashEntry *e);
+
+ template <typename C>
+ static void
+ free(RefCountCacheHashEntry *e)
+ {
+ // Since the Value is actually RefCountObj-- when this gets deleted normally it calls the wrong
+ // `free` method, this forces the delete/decr to happen with the right type
+ Ptr<C> *tmp = (Ptr<C> *)&e->item;
+ tmp->clear();
+
+ e->~RefCountCacheHashEntry();
+ dealloc(e);
+ }
};
// Since the hashing values are all fixed size, we can simply use a classAllocator to avoid mallocs
-extern ClassAllocator<RefCountCacheHashEntry> refCountCacheHashingValueAllocator;
extern ClassAllocator<PriorityQueueEntry<RefCountCacheHashEntry *>> expiryQueueEntry;
struct RefCountCacheHashing {
@@ -202,7 +217,7 @@ RefCountCachePartition<C>::put(uint64_t key, C *item, int size, int expire_time)
}
// Create our value-- which has a ref to the `item`
- RefCountCacheHashEntry *val = refCountCacheHashingValueAllocator.alloc();
+ RefCountCacheHashEntry *val = RefCountCacheHashEntry::alloc();
val->set(item, key, size, expire_time);
// add expiry_entry to expiry queue, if the expire time is positive (otherwise it means don't expire)
@@ -262,12 +277,7 @@ RefCountCachePartition<C>::dealloc_entry(Iterator ptr)
ptr->expiry_entry = nullptr; // To avoid the destruction of `l` calling the destructor again-- and causing issues
}
- // Since the Value is actually RefCountObj-- when this gets deleted normally it calls the wrong
- // `free` method, this forces the delete/decr to happen with the right type
- Ptr<C> *tmp = (Ptr<C> *)&ptr->item;
- tmp->clear();
- ptr->~RefCountCacheHashEntry();
- refCountCacheHashingValueAllocator.free(ptr.m_value);
+ RefCountCacheHashEntry::free<C>(ptr.m_value);
}
}
@@ -332,7 +342,7 @@ void
RefCountCachePartition<C>::copy(Vec<RefCountCacheHashEntry *> &items)
{
for (RefCountCachePartition<C>::iterator_type i = this->item_map.begin(); i != this->item_map.end(); ++i) {
- RefCountCacheHashEntry *val = refCountCacheHashingValueAllocator.alloc();
+ RefCountCacheHashEntry *val = RefCountCacheHashEntry::alloc();
val->set(i.m_value->item.get(), i.m_value->meta.key, i.m_value->meta.size, i.m_value->meta.expiry_time);
items.push_back(val);
}
diff --git a/iocore/hostdb/P_RefCountCacheSerializer.h b/iocore/hostdb/P_RefCountCacheSerializer.h
index 63bc8ca..4004f37 100644
--- a/iocore/hostdb/P_RefCountCacheSerializer.h
+++ b/iocore/hostdb/P_RefCountCacheSerializer.h
@@ -39,8 +39,8 @@
template <class C> class RefCountCacheSerializer : public Continuation
{
public:
- size_t partition; // Current partition
- C *cache; // Pointer to the entire cache
+ size_t partition; // Current partition
+ RefCountCache<C> *cache; // Pointer to the entire cache
Continuation *cont;
int copy_partition(int event, Event *e);
@@ -55,7 +55,7 @@ public:
// helper method to spin on writes to disk
int write_to_disk(const void *, size_t);
- RefCountCacheSerializer(Continuation *acont, C *cc, int frequency, std::string dirname, std::string filename);
+ RefCountCacheSerializer(Continuation *acont, RefCountCache<C> *cc, int frequency, std::string dirname, std::string filename);
~RefCountCacheSerializer();
private:
@@ -77,7 +77,7 @@ private:
};
template <class C>
-RefCountCacheSerializer<C>::RefCountCacheSerializer(Continuation *acont, C *cc, int frequency, std::string dirname,
+RefCountCacheSerializer<C>::RefCountCacheSerializer(Continuation *acont, RefCountCache<C> *cc, int frequency, std::string dirname,
std::string filename)
: Continuation(nullptr),
partition(0),
@@ -107,6 +107,11 @@ template <class C> RefCountCacheSerializer<C>::~RefCountCacheSerializer()
socketManager.close(fd);
}
+ forv_Vec (RefCountCacheHashEntry, entry, this->partition_items) {
+ RefCountCacheHashEntry::free<C>(entry);
+ }
+ this->partition_items.clear();
+
Debug("refcountcache", "finished serializer %p", this);
// Note that we have to do the unlink before we send the completion event, otherwise
@@ -153,15 +158,15 @@ RefCountCacheSerializer<C>::write_partition(int /* event */, Event *e)
// write to disk with headers per item
for (unsigned int i = 0; i < this->partition_items.length(); i++) {
- RefCountCacheHashEntry *it = this->partition_items[i];
+ RefCountCacheHashEntry *entry = this->partition_items[i];
// check if the item has expired, if so don't persist it to disk
- if (it->meta.expiry_time < curr_time) {
+ if (entry->meta.expiry_time < curr_time) {
continue;
}
// Write the RefCountCacheItemMeta (as our header)
- int ret = this->write_to_disk((char *)&it->meta, sizeof(it->meta));
+ int ret = this->write_to_disk((char *)&entry->meta, sizeof(entry->meta));
if (ret < 0) {
Warning("Error writing cache item header to %s: %d", this->tmp_filename.c_str(), ret);
delete this;
@@ -169,7 +174,7 @@ RefCountCacheSerializer<C>::write_partition(int /* event */, Event *e)
}
// write the actual object now
- ret = this->write_to_disk((char *)it->item.get(), it->meta.size);
+ ret = this->write_to_disk((char *)entry->item.get(), entry->meta.size);
if (ret < 0) {
Warning("Error writing cache item to %s: %d", this->tmp_filename.c_str(), ret);
delete this;
@@ -177,11 +182,13 @@ RefCountCacheSerializer<C>::write_partition(int /* event */, Event *e)
}
this->total_items++;
- this->total_size += it->meta.size;
- refCountCacheHashingValueAllocator.free(it);
+ this->total_size += entry->meta.size;
}
- // Clear partition-- for the next user
+ // Clear the copied partition for the next round.
+ forv_Vec (RefCountCacheHashEntry, entry, this->partition_items) {
+ RefCountCacheHashEntry::free<C>(entry);
+ }
this->partition_items.clear();
SET_HANDLER(&RefCountCacheSerializer::pause_event);
diff --git a/iocore/hostdb/RefCountCache.cc b/iocore/hostdb/RefCountCache.cc
index 1d35ecb..26abc75 100644
--- a/iocore/hostdb/RefCountCache.cc
+++ b/iocore/hostdb/RefCountCache.cc
@@ -22,10 +22,22 @@
#include <P_RefCountCache.h>
// Since the hashing values are all fixed size, we can simply use a classAllocator to avoid mallocs
-ClassAllocator<RefCountCacheHashEntry> refCountCacheHashingValueAllocator("refCountCacheHashingValueAllocator");
+static ClassAllocator<RefCountCacheHashEntry> refCountCacheHashingValueAllocator("refCountCacheHashingValueAllocator");
ClassAllocator<PriorityQueueEntry<RefCountCacheHashEntry *>> expiryQueueEntry("expiryQueueEntry");
+RefCountCacheHashEntry *
+RefCountCacheHashEntry::alloc()
+{
+ return refCountCacheHashingValueAllocator.alloc();
+}
+
+void
+RefCountCacheHashEntry::dealloc(RefCountCacheHashEntry *e)
+{
+ return refCountCacheHashingValueAllocator.free(e);
+}
+
RefCountCacheHeader::RefCountCacheHeader(VersionNumber object_version)
: magic(REFCOUNTCACHE_MAGIC_NUMBER), object_version(object_version)
{
--
To stop receiving notification emails like this one, please contact
"commits@trafficserver.apache.org" <co...@trafficserver.apache.org>.
[trafficserver] 01/03: TS-5065: Fix RefCountCache iterator
invalidation.
Posted by jp...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
jpeach pushed a commit to branch master
in repository https://git-dual.apache.org/repos/asf/trafficserver.git
commit 23672db4f1e6b3d7b633a82666955abe6ca86950
Author: James Peach <jp...@apache.org>
AuthorDate: Fri Nov 25 23:09:47 2016 -0800
TS-5065: Fix RefCountCache iterator invalidation.
Removing items from the TSHashMap invalidates the iterator because the
linked list pointers are embedded in the hash node, so we can't do that
while clearing. Instead, deallocate and remove each entry explicitly.
---
iocore/hostdb/P_RefCountCache.h | 60 +++++++++++++++++++++++++++--------------
1 file changed, 40 insertions(+), 20 deletions(-)
diff --git a/iocore/hostdb/P_RefCountCache.h b/iocore/hostdb/P_RefCountCache.h
index c417a5c..15ed326 100644
--- a/iocore/hostdb/P_RefCountCache.h
+++ b/iocore/hostdb/P_RefCountCache.h
@@ -79,21 +79,23 @@ public:
PriorityQueueEntry<RefCountCacheHashEntry *> *expiry_entry;
RefCountCacheItemMeta meta;
+ // Need a no-argument constructor to use the classAllocator
+ RefCountCacheHashEntry() : item(Ptr<RefCountObj>()), expiry_entry(nullptr), meta(0, 0) {}
void
set(RefCountObj *i, uint64_t key, unsigned int size, int expire_time)
{
this->item = make_ptr(i);
this->meta = RefCountCacheItemMeta(key, size, expire_time);
- };
- // Need a no-argument constructor to use the classAllocator
- RefCountCacheHashEntry() : item(Ptr<RefCountObj>()), expiry_entry(nullptr), meta(0, 0) {}
+ }
+
// make these values comparable -- so we can sort them
bool
operator<(const RefCountCacheHashEntry &v2) const
{
return this->meta.expiry_time < v2.meta.expiry_time;
- };
+ }
};
+
// Since the hashing values are all fixed size, we can simply use a classAllocator to avoid mallocs
extern ClassAllocator<RefCountCacheHashEntry> refCountCacheHashingValueAllocator;
extern ClassAllocator<PriorityQueueEntry<RefCountCacheHashEntry *>> expiryQueueEntry;
@@ -134,6 +136,7 @@ public:
void clear();
bool is_full() const;
bool make_space_for(unsigned int);
+ template <class Iterator> void dealloc_entry(Iterator ptr);
size_t count() const;
void copy(Vec<RefCountCacheHashEntry *> &items);
@@ -228,31 +231,43 @@ RefCountCachePartition<C>::erase(uint64_t key, ink_time_t expiry_time)
if (expiry_time >= 0 && l.m_value->meta.expiry_time != expiry_time) {
return;
}
+
// TSHashMap does NOT clean up the item-- this remove just removes it from the map
// we are responsible for cleaning it up here
this->item_map.remove(l);
+ this->dealloc_entry(l);
+ }
+}
- // decrement usage counters
- this->size -= l.m_value->meta.size;
+template <class C>
+template <class Iterator>
+void
+RefCountCachePartition<C>::dealloc_entry(Iterator ptr)
+{
+ if (ptr.m_value) {
+ // decrement usag are not cleaned up. The values are not touched in this method, therefore it is safe
+ // counters
+ this->size -= ptr->meta.size;
this->items--;
- this->metric_inc(refcountcache_current_size_stat, -((int64_t)l.m_value->meta.size));
+ this->metric_inc(refcountcache_current_size_stat, -((int64_t)ptr->meta.size));
this->metric_inc(refcountcache_current_items_stat, -1);
// remove from expiry queue
- if (l.m_value->expiry_entry != nullptr) {
- Debug("refcountcache", "partition %d deleting item from expiry_queue idx=%d\n", this->part_num,
- l.m_value->expiry_entry->index);
- this->expiry_queue.erase(l.m_value->expiry_entry);
- expiryQueueEntry.free(l.m_value->expiry_entry);
- l.m_value->expiry_entry = nullptr; // To avoid the destruction of `l` calling the destructor again-- and causing issues
+ if (ptr->expiry_entry != nullptr) {
+ Debug("refcountcache", "partition %d deleting item from expiry_queue idx=%d", this->part_num, ptr->expiry_entry->index);
+
+ this->expiry_queue.erase(ptr->expiry_entry);
+ expiryQueueEntry.free(ptr->expiry_entry);
+ ptr->expiry_entry = nullptr; // To avoid the destruction of `l` calling the destructor again-- and causing issues
}
+
// Since the Value is actually RefCountObj-- when this gets deleted normally it calls the wrong
// `free` method, this forces the delete/decr to happen with the right type
- Ptr<C> *tmp = (Ptr<C> *)&l.m_value->item;
+ Ptr<C> *tmp = (Ptr<C> *)&ptr->item;
tmp->clear();
- l.m_value->~RefCountCacheHashEntry();
- refCountCacheHashingValueAllocator.free(l.m_value);
+ ptr->~RefCountCacheHashEntry();
+ refCountCacheHashingValueAllocator.free(ptr.m_value);
}
}
@@ -260,10 +275,15 @@ template <class C>
void
RefCountCachePartition<C>::clear()
{
- // this->item_map.clear() doesn't clean up anything, so instead of using that we'll iterate
- // over every item and then call delete
- for (RefCountCachePartition<C>::iterator_type i = this->item_map.begin(); i != this->item_map.end(); ++i) {
- this->erase(i.m_value->meta.key, i.m_value->meta.expiry_time);
+ // Since the hash nodes embed the list pointers, you can't iterate over the
+ // hash elements and deallocate them, let alone remove them from the hash.
+ // Hence, this monstrosity.
+ while (this->item_map.count() > 0) {
+ location_type pos = this->item_map.find(this->item_map.begin().m_value);
+
+ ink_assert(pos.isValid());
+ this->item_map.remove(pos);
+ this->dealloc_entry(pos);
}
}
--
To stop receiving notification emails like this one, please contact
"commits@trafficserver.apache.org" <co...@trafficserver.apache.org>.