[
https://issues.apache.org/jira/browse/PROTON-2396?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17445320#comment-17445320
]
ASF GitHub Bot commented on PROTON-2396:
----------------------------------------
astitcher commented on a change in pull request #340:
URL: https://github.com/apache/qpid-proton/pull/340#discussion_r751379226
##########
File path: cpp/include/proton/uuid.hpp
##########
@@ -35,13 +38,20 @@ namespace proton {
/// A 16-byte universally unique identifier.
class uuid : public byte_array<16> {
+
+ private:
+ thread_local static std::independent_bits_engine<std::mt19937, CHAR_BIT,
+ unsigned int>
+ engine;
+ thread_local static std::seed_seq seed;
+
Review comment:
Please don't add these private members to the class. It changes the ABI
(also the API but in a more subtle way).
Since these are thread_locals anyway they don't need to be per instance
variables so can just be static to the implementation file itself and not be in
the class at all.
##########
File path: cpp/src/uuid.cpp
##########
@@ -38,20 +43,17 @@
namespace proton {
namespace {
-
-
-// Seed the random number generated once at startup.
-struct seed {
- seed() {
- // A hash of time and PID, time alone is a bad seed as programs started
- // within the same second will get the same seed.
- unsigned long secs = time(0);
- unsigned long pid = GETPID();
- std::srand(((secs*181)*((pid-83)*359))%104729);
- }
-} seed_;
-
-}
+// A hash of time, PID and random_device, time alone is a bad seed as programs
+// started within the same second will get the same seed.
+thread_local unsigned long ticks =
+ std::chrono::system_clock::now().time_since_epoch().count();
+unsigned long pid = GETPID();
+unsigned int rd = std::random_device{}();
+} // namespace
+
+thread_local std::seed_seq uuid::seed{ticks, pid, (unsigned long)rd};
+thread_local std::independent_bits_engine<std::mt19937, CHAR_BIT, unsigned int>
+ uuid::engine(seed);
Review comment:
I like the struct seed construct to initialise the seed once. You can
shift your code into there and make it thread_local I think which will be a
neater implementation and a smaller change.
##########
File path: cpp/include/proton/uuid.hpp
##########
@@ -35,13 +38,20 @@ namespace proton {
/// A 16-byte universally unique identifier.
class uuid : public byte_array<16> {
+
+ private:
+ thread_local static std::independent_bits_engine<std::mt19937, CHAR_BIT,
+ unsigned int>
+ engine;
+ thread_local static std::seed_seq seed;
+
public:
/// Make a copy.
PN_CPP_EXTERN static uuid copy();
/// Return a uuid copied from bytes. Bytes must point to at least
/// 16 bytes. If `bytes == 0` the UUID is zero-initialized.
- PN_CPP_EXTERN static uuid copy(const char* bytes);
+ PN_CPP_EXTERN static uuid copy(const char *bytes);
Review comment:
FWIW the convention used in our C++ is in line with usual C++
conventions and has the '*' in the type side not the name side. Our C code is
in line with more usual C conventions and is the other way round. This is
probably not written down anywhere - sorry!
##########
File path: cpp/src/uuid.cpp
##########
@@ -70,12 +72,7 @@ uuid uuid::copy(const char* bytes) {
uuid uuid::random() {
uuid bytes;
- int r = std::rand();
- for (size_t i = 0; i < bytes.size(); ++i ) {
- bytes[i] = r & 0xFF;
- r >>= 8;
- if (!r) r = std::rand();
- }
+ std::generate(bytes.begin(), bytes.end(), std::ref(engine));
Review comment:
in my way of doing this engine would be seed_::engine - maybe seed_
should have a better name?
##########
File path: cpp/src/uuid.cpp
##########
@@ -38,20 +43,17 @@
namespace proton {
namespace {
-
-
-// Seed the random number generated once at startup.
-struct seed {
- seed() {
- // A hash of time and PID, time alone is a bad seed as programs started
- // within the same second will get the same seed.
- unsigned long secs = time(0);
- unsigned long pid = GETPID();
- std::srand(((secs*181)*((pid-83)*359))%104729);
- }
-} seed_;
-
-}
+// A hash of time, PID and random_device, time alone is a bad seed as programs
+// started within the same second will get the same seed.
Review comment:
I think this comment maybe now incorrect as
std::chrono::system_clock::now() isn't in seconds any more (is it?)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
> [cpp] Seed in uuid.cpp can lead to duplicates
> ---------------------------------------------
>
> Key: PROTON-2396
> URL: https://issues.apache.org/jira/browse/PROTON-2396
> Project: Qpid Proton
> Issue Type: Bug
> Components: cpp-binding
> Environment: RHEL7 running in OpenStack
> docker-ce 19.03.5
> qpid-proton 0.28.0
> qpid-cpp 1.37.0
> Reporter: Ryan Herbert
> Assignee: Rakhi Kumari
> Priority: Major
>
> The random number seed used in qpid-proton/cpp/src/uuid.cpp is based on the
> current time and the PID of the running process. When starting multiple
> proton instances simultaneously in Docker containers via automated
> deployment, there is a high probability that multiple instances will get the
> same seed since the PID within the Docker container is consistent and the
> same across multiple copies of the same Docker container.
> This results in duplicate link names when binding to exchanges. When this
> happens, the queue gets bound to two different exchanges, and requests sent
> to one exchange will get responses from both services.
> To work around this error, we are specifying the link name via
> sender_options/receiver_options every time we open a new sender/receiver, and
> we also specify the container_id in connection_options. We are using
> std::mt19937_64 seeded with
> std::chrono::system_clock::now().time_since_epoch().count() to generate the
> random part of our link names, which seems to have enough randomness that it
> has eliminated the problem for us.
> As pointed out in the Proton user forum, std::random_device is probably a
> better choice for initializing the seed.
>
--
This message was sent by Atlassian Jira
(v8.20.1#820001)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org
For additional commands, e-mail: dev-h...@qpid.apache.org
