further threadsafety hardening #55
Conversation
| namespace detail { | ||
| inline std::string sanitizePrefix(std::string prefix) { | ||
| // For convenience we provide the dot when generating the stat message | ||
| if (!prefix.empty() && prefix.back() == '.') { | ||
| prefix.pop_back(); | ||
| } | ||
| return prefix; | ||
| } | ||
|
|
||
| inline float rand(unsigned int seed) { | ||
| static thread_local std::mt19937 twister(seed); | ||
| static thread_local std::uniform_real_distribution<float> dist(0.f, 1.f); | ||
| return dist(twister); | ||
| } | ||
|
|
||
| // All supported metric types | ||
| constexpr char METRIC_TYPE_COUNT[] = "c"; | ||
| constexpr char METRIC_TYPE_GAUGE[] = "g"; | ||
| constexpr char METRIC_TYPE_TIMING[] = "ms"; | ||
| constexpr char METRIC_TYPE_SET[] = "s"; | ||
| } // namespace detail |
There was a problem hiding this comment.
moved this up above the constructor for the class so i could make use of the random function as a default argument
| FrequencyFunc frequencyFunc = | ||
| std::bind(detail::rand, std::random_device()())) noexcept; |
There was a problem hiding this comment.
we no longer have a seed method on the class but rather you pass in a sampling function. i personally like the term sample better here but because it has interplay with the "frequency" (nomenclature from statsd proper) i chose to make them match
| //! Sets a configuration | ||
| void setConfig(const std::string& host, | ||
| const uint16_t port, | ||
| const std::string& prefix, | ||
| const uint64_t batchsize = 0, | ||
| const uint64_t sendInterval = 1000, | ||
| const int gaugePrecision = 4) noexcept; | ||
|
|
There was a problem hiding this comment.
allowing this makes it possible to break threadsafety gaurantees so its better to just remove it. you can simply construct a new class if you want to configure it differently (should be a rare thing anyway)
| //! Flush any queued stats to the daemon | ||
| void flush() noexcept; | ||
| void flush() const noexcept; |
There was a problem hiding this comment.
Can it be made const given that UDPSender::flush() is not const?
There was a problem hiding this comment.
its kind of a dirty trick in this case but because the udpsender is a unique_ptr and its -> dereference operator is const (since it doesnt modify the smart pointer itself) we can then freely call the non const member on the dereferenced instance of udpsender that the smart ptr returned us
| @@ -152,58 +172,29 @@ class StatsdClient { | |||
|
|
|||
| //!@} | |||
|
|
|||
| private: | |||
| //! The prefix to be used for metrics | ||
| std::string m_prefix; | ||
|
|
||
| //! The UDP sender to be used for actual sending | ||
| std::unique_ptr<UDPSender> m_sender; | ||
|
|
||
| //! The random number generator for handling sampling | ||
| mutable std::mt19937 m_randomEngine; |
There was a problem hiding this comment.
not needed as its now encapsulated inside of the random functor below
| //! Fixed floating point precision of gauges | ||
| int m_gaugePrecision; | ||
| }; | ||
|
|
||
| namespace detail { |
There was a problem hiding this comment.
moved up as previously state
| : m_prefix(detail::sanitizePrefix(prefix)), | ||
| m_sender(new UDPSender{host, port, batchsize, sendInterval}), | ||
| m_gaugePrecision(gaugePrecision) { | ||
| // Initialize the random generator to be used for sampling | ||
| seed(); |
There was a problem hiding this comment.
happens in the declaration as part of the default param
| @@ -53,43 +53,24 @@ void testErrorConditions() { | |||
| throwOnError(client, false, "Should not be able to resolve a ridiculous ip"); | |||
| } | |||
|
|
|||
| void testReconfigure() { | |||
There was a problem hiding this comment.
functionality is removed so test is removed
tests/testStatsdClient.cpp
Outdated
| std::mt19937 twister(std::random_device{}()); | ||
| std::uniform_real_distribution<float> dist(0.f, 1.f); | ||
| auto rand = [&]()->float{return dist(twister);}; |
There was a problem hiding this comment.
we pass our own custom sampling function so we can fiddle with it to get it to give the results we want
| std::cerr << "Unexpected stats received by server, got:" << std::endl; | ||
| for (const auto& message : messages) { | ||
| std::cerr << message << std::endl; | ||
| for (size_t i = 0; i < expected.size(); ++i) { |
There was a problem hiding this comment.
while i was debugging this trying to get a magical seed that would work for all examples i found changing the formatting of this output makes it easier to debug what is going on
|
Thank you for the work @kevinkreiser ! I have to admin that I've only been following this distantly, but I'll review asap. |
docs updates
|
well i saw your ship and so i went to edit the docs to say that the library is thread-safe but upon review i think there is still one thing to do which is to make the sender thread-safe even when the sending interval is see in most use-cases you would make use of the sending interval to spawn the background sending thread and the library is then thread-safe HOWEVER when the sending interval is therefore if you configure your client with send interval |
over in #12 we've clarified a few things to make the library threadsafe. there are a couple more little things to finish the job though largely the library is already threadsafe. this pr makes the use of the rng threadsafe by using thread local storage. additionally we remove the ability to change the internal state of the client via
setConfigandseed. you're only option for configuration is up front or to make a new client which is certainly a clearer contractthe only thing left to do is fix the tests since the api has changed some of the tests are hard to keep in their current form
fixes #12