Skip to content

Commit

Permalink
[SDK] Fix memory leak in TlsRandomNumberGenerator() constructor (#2661)
Browse files Browse the repository at this point in the history
  • Loading branch information
hongweipeng authored Jul 19, 2024
1 parent 4520aa5 commit deed1e3
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 1 deletion.
9 changes: 8 additions & 1 deletion sdk/src/common/random.cc
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include "src/common/random.h"
#include "src/common/platform/fork.h"

#include <atomic>
#include <cstring>
#include <random>

Expand All @@ -26,12 +27,17 @@ class TlsRandomNumberGenerator
TlsRandomNumberGenerator() noexcept
{
Seed();
platform::AtFork(nullptr, nullptr, OnFork);
if (!flag.test_and_set())
{
platform::AtFork(nullptr, nullptr, OnFork);
}
}

static FastRandomNumberGenerator &engine() noexcept { return engine_; }

private:
static std::atomic_flag flag;

static thread_local FastRandomNumberGenerator engine_;

static void OnFork() noexcept { Seed(); }
Expand All @@ -44,6 +50,7 @@ class TlsRandomNumberGenerator
}
};

std::atomic_flag TlsRandomNumberGenerator::flag;
thread_local FastRandomNumberGenerator TlsRandomNumberGenerator::engine_{};
} // namespace

Expand Down
27 changes: 27 additions & 0 deletions sdk/test/common/random_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,10 @@
#include "src/common/random.h"

#include <algorithm>
#include <atomic>
#include <iterator>
#include <thread>
#include <vector>

#include <gtest/gtest.h>
using opentelemetry::sdk::common::Random;
Expand Down Expand Up @@ -34,3 +37,27 @@ TEST(RandomTest, GenerateRandomBuffer)
std::equal(std::begin(buf1_vector), std::end(buf1_vector), std::begin(buf2_vector)));
}
}

void doSomethingOnce(std::atomic_uint *count)
{
static std::atomic_flag flag;
if (!flag.test_and_set())
{
(*count)++;
}
}

TEST(RandomTest, AtomicFlagMultiThreadTest)
{
std::vector<std::thread> threads;
std::atomic_uint count(0);
for (int i = 0; i < 10; ++i)
{
threads.push_back(std::thread(doSomethingOnce, &count));
}
for (auto &t : threads)
{
t.join();
}
EXPECT_EQ(1, count.load());
}

1 comment on commit deed1e3

@github-actions
Copy link

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'OpenTelemetry-cpp sdk Benchmark'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.

Benchmark suite Current: deed1e3 Previous: 4520aa5 Ratio
BM_BaselineBuffer/2 16647655.963897705 ns/iter 7384438.514709473 ns/iter 2.25
BM_LockFreeBuffer/1 6789910.793304443 ns/iter 3017295.1221466064 ns/iter 2.25

This comment was automatically generated by workflow using github-action-benchmark.

Please sign in to comment.