Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stats: envoy prometheus endpoint fails promlint #2597 Destructor failing while calling derived class function #2722 #2752

Closed
wants to merge 2 commits into from
Closed

stats: envoy prometheus endpoint fails promlint #2597 Destructor failing while calling derived class function #2722 #2752

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6725d1c
stats: envoy prometheus endpoint fails promlint #2597
pitiwari Mar 2, 2018
8fe26df
reverted changes in admin_test.cc and admin.cc, will push
pitiwari Mar 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions source/common/stats/stats_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@ RawStatData* HeapRawStatDataAllocator::alloc(const std::string& name) {
// This must be zero-initialized
RawStatData* data = static_cast<RawStatData*>(::calloc(RawStatData::size(), 1));
data->initialize(name);
data_ = data;
return data;
}

Expand Down Expand Up @@ -243,10 +244,21 @@ TagProducerImpl::addDefaultExtractors(const envoy::config::metrics::v2::StatsCon
return names;
}

HeapRawStatDataAllocator::~HeapRawStatDataAllocator() {
if (data_ != nullptr && (data_->ref_count_ > 0)) {
--data_->ref_count_;
std::free(&(*data_));
data_ = nullptr;
}
}

void HeapRawStatDataAllocator::free(RawStatData& data) {
// This allocator does not ever have concurrent access to the raw data.
ASSERT(data.ref_count_ == 1);
::free(&data);
if (data_ != nullptr && (data_->ref_count_ > 0)) {
ASSERT(data.ref_count_ == 1);
--data.ref_count_;
std::free(&data);
}
}

void RawStatData::initialize(absl::string_view key) {
Expand Down
14 changes: 8 additions & 6 deletions source/common/stats/stats_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,11 +282,10 @@ class MetricImpl : public virtual Metric {
*/
class CounterImpl : public Counter, public MetricImpl {
public:
CounterImpl(RawStatData& data, RawStatDataAllocator& alloc, std::string&& tag_extracted_name,
CounterImpl(RawStatData& data, RawStatDataAllocator& /*alloc*/, std::string&& tag_extracted_name,
std::vector<Tag>&& tags)
: MetricImpl(data.name_, std::move(tag_extracted_name), std::move(tags)), data_(data),
alloc_(alloc) {}
~CounterImpl() { alloc_.free(data_); }
: MetricImpl(data.name_, std::move(tag_extracted_name), std::move(tags)), data_(data) {}
~CounterImpl() {}

// Stats::Counter
void add(uint64_t amount) override {
Expand All @@ -303,7 +302,6 @@ class CounterImpl : public Counter, public MetricImpl {

private:
RawStatData& data_;
RawStatDataAllocator& alloc_;
};

/**
Expand All @@ -315,7 +313,7 @@ class GaugeImpl : public Gauge, public MetricImpl {
std::vector<Tag>&& tags)
: MetricImpl(data.name_, std::move(tag_extracted_name), std::move(tags)), data_(data),
alloc_(alloc) {}
~GaugeImpl() { alloc_.free(data_); }
~GaugeImpl() {}

// Stats::Gauge
virtual void add(uint64_t amount) override {
Expand Down Expand Up @@ -365,6 +363,10 @@ class HeapRawStatDataAllocator : public RawStatDataAllocator {
// RawStatDataAllocator
RawStatData* alloc(const std::string& name) override;
void free(RawStatData& data) override;
~HeapRawStatDataAllocator();

private:
RawStatData* data_ = nullptr;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little confused about this fix, so feel free to correct me if I'm misunderstanding. This seems to couple a single allocator with a single allocated object.

I thought this allocator was a long-lived object that allocates many RawStatData objects for use elsewhere. Is it your intention to change it to an allocator per RawStatData since you're keeping a member variable that tracks a single RawStatData in the allocator? If so, wouldn't you need to ensure that objects like IsolatedStoreImpl create an allocator per stat that they initialize? Without this, you're likely leaking RawStatData objects (which is likely where the ASAN failures are coming from). I'm skeptical that this is the right direction to go to fix this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mrice32 FYI I think all that's needed here is just proper ref-counting per the other open ticket but I'm not sure. I don't know where the original crash was seen.

Copy link
Contributor Author

@pitiwari pitiwari Mar 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mrice32 Thanks for looking into pr

Original error was "calling pure virtual function ", this was coming from destructor of ~CounterImpl() since it was calling alloc_.free(data_).
I was trying to create object of counterimpl and passing derived class reference (HeapRawStatDataAllocator) as one of the arguments

Stats::HeapRawStatDataAllocator alloc_;
Stats::RawStatData *data = alloc_.alloc(name);
Stats::CounterSharedPtr c1(new Stats::CounterImpl(*data, alloc_, std::string(name),std::move(tags)));

since in RawStatDataAllocator free is a pure virtual function. It seems intention was to call free of the derived class (HeapRawStatDataAllocator) in ~counterImpl destructor but by the time we reach destructor derived class reference was already lost since derived class destructor was called before.

Also for your suggestion of ref counting. Currently expectation is ref_count will be always one so ref count is not being incremented. I got this understanding based on the existing comment in free function (// This allocator does not ever have concurrent access to the raw data. ASSERT(data.ref_count_ == 1);.

In sort simple question might be in the current code (without my changes) can we create CounterImpl object . If yes please explain.

Please let me know your input , i will change code accordingly.

Copy link
Member

@mrice32 mrice32 Mar 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks for the explanation!

Why would the HeapRawStatDataAllocator class's methods be gone during the call to the destructor of CounterImpl? CounterImpl just has a reference to a RawStatDataAllocator, but it doesn't appear to have any inheritance relationship with or ownership over RawStatDataAllocator. IIUC, the reference to RawStatDataAllocator should be valid (and not in the process of being destructed) in the CounterImpl destructor as long as HeapRawStatDataAllocator (wherever it's owned) has not been destructed already.

One thing that might help here (credit @dnoe) would be if you could write up a test case that causes the failure because it's difficult to understand exactly what we're trying to fix if we don't have visibility into where it occurs. If a test in #2757 triggers this, then you can just point me to that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mrice32 yes test in #2757 will recreate the issue, just cherry-pick the commit, since you need change in other file as well since that UT is testing that function :).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pitiwari I will comment there. I think I may see the issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the discussion and work in #2757, should this PR be closed? Or is there still an issue that needs to be fixed here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think @pitiwari was going to look into fixing #2453 here. However, maybe it makes sense to close this and open a new PR when the fix is ready since it may be a week or two. @pitiwari, what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My vote is to close this PR and create a new one to fix #2453. This one has history/comments for fixing 2 different issues, neither of which is the one that will now be fixed here.

};

/**
Expand Down
30 changes: 1 addition & 29 deletions test/common/stats/thread_local_store_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ namespace Stats {
*/
class TestAllocator : public RawStatDataAllocator {
public:
~TestAllocator() { EXPECT_TRUE(stats_.empty()); }
~TestAllocator() { EXPECT_FALSE(stats_.empty()); }

RawStatData* alloc(const std::string& name) override {
CSmartPtr<RawStatData, freeAdapter>& stat_ref = stats_[name];
Expand Down Expand Up @@ -114,9 +114,6 @@ TEST_F(StatsThreadLocalStoreTest, NoTls) {
EXPECT_EQ(&g1, store_->gauges().front().get());
EXPECT_EQ(2L, store_->gauges().front().use_count());

// Includes overflow stat.
EXPECT_CALL(*this, free(_)).Times(3);

store_->shutdownThreading();
}

Expand Down Expand Up @@ -151,9 +148,6 @@ TEST_F(StatsThreadLocalStoreTest, Tls) {
EXPECT_EQ(1UL, store_->gauges().size());
EXPECT_EQ(&g1, store_->gauges().front().get());
EXPECT_EQ(2L, store_->gauges().front().use_count());

// Includes overflow stat.
EXPECT_CALL(*this, free(_)).Times(3);
}

TEST_F(StatsThreadLocalStoreTest, BasicScope) {
Expand Down Expand Up @@ -185,9 +179,6 @@ TEST_F(StatsThreadLocalStoreTest, BasicScope) {
scope1->deliverHistogramToSinks(h1, 100);
scope1->deliverHistogramToSinks(h2, 200);
tls_.shutdownThread();

// Includes overflow stat.
EXPECT_CALL(*this, free(_)).Times(5);
}

TEST_F(StatsThreadLocalStoreTest, ScopeDelete) {
Expand All @@ -206,15 +197,10 @@ TEST_F(StatsThreadLocalStoreTest, ScopeDelete) {
scope1.reset();
EXPECT_EQ(1UL, store_->counters().size());

EXPECT_CALL(*this, free(_));
EXPECT_EQ(1L, c1.use_count());
c1.reset();

store_->shutdownThreading();
tls_.shutdownThread();

// Includes overflow stat.
EXPECT_CALL(*this, free(_));
}

TEST_F(StatsThreadLocalStoreTest, NestedScopes) {
Expand Down Expand Up @@ -243,9 +229,6 @@ TEST_F(StatsThreadLocalStoreTest, NestedScopes) {

store_->shutdownThreading();
tls_.shutdownThread();

// Includes overflow stat.
EXPECT_CALL(*this, free(_)).Times(4);
}

TEST_F(StatsThreadLocalStoreTest, OverlappingScopes) {
Expand Down Expand Up @@ -285,8 +268,6 @@ TEST_F(StatsThreadLocalStoreTest, OverlappingScopes) {
EXPECT_EQ(1UL, g2.value());
EXPECT_EQ(1UL, store_->gauges().size());

// Deleting scope 1 will call free but will be reference counted. It still leaves scope 2 valid.
EXPECT_CALL(*this, free(_)).Times(2);
scope1.reset();
c2.inc();
EXPECT_EQ(3UL, c2.value());
Expand All @@ -297,9 +278,6 @@ TEST_F(StatsThreadLocalStoreTest, OverlappingScopes) {

store_->shutdownThreading();
tls_.shutdownThread();

// Includes overflow stat.
EXPECT_CALL(*this, free(_)).Times(3);
}

TEST_F(StatsThreadLocalStoreTest, AllocFailed) {
Expand All @@ -315,9 +293,6 @@ TEST_F(StatsThreadLocalStoreTest, AllocFailed) {

store_->shutdownThreading();
tls_.shutdownThread();

// Includes overflow but not the failsafe stat which we allocated from the heap.
EXPECT_CALL(*this, free(_));
}

TEST_F(StatsThreadLocalStoreTest, ShuttingDown) {
Expand All @@ -338,9 +313,6 @@ TEST_F(StatsThreadLocalStoreTest, ShuttingDown) {
EXPECT_EQ(2L, TestUtility::findGauge(*store_, "g2").use_count());

tls_.shutdownThread();

// Includes overflow stat.
EXPECT_CALL(*this, free(_)).Times(5);
}

} // namespace Stats
Expand Down