Only check GZ_TRANSPORT_TOPIC_STATISTICS once #731
Conversation
Signed-off-by: Carlos Agüero <[email protected]>
azeey
left a comment
azeey
left a comment
There was a problem hiding this comment.
I would agree that environment variables should not be used to change behavior after the program has started. If we want to do that, we should be providing a C++ API for it.
src/Discovery.hh
Outdated
| /// \return The discovery version. | ||
| private: uint8_t Version() const | ||
| { | ||
| static std::string gzStats; |
There was a problem hiding this comment.
We can make gzStats a non-static std::string then. It's always good to remove static variables with non-trivial destructors https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables
src/Discovery.hh
Outdated
| static int topicStats; | ||
| static bool versionInitialized = false; |
There was a problem hiding this comment.
Should these be std::atomic in case they are accessed from mulitple threads?
src/Discovery.hh
Outdated
| env("GZ_TRANSPORT_TOPIC_STATISTICS", gzStats) && !gzStats.empty()) | ||
| { | ||
| topicStats = (gzStats == "1"); | ||
| versionInitialized = true; |
There was a problem hiding this comment.
If GZ_TRANSPORT_TOPIC_STATISTICS is not set, this if block will not get executed, so we'll end up checking the environment variable everytime still. How about putting this versionInitialized = true after the if block?
Signed-off-by: Carlos Agüero <[email protected]>
src/Discovery.hh
Outdated
|
|
||
| if (env("GZ_TRANSPORT_TOPIC_STATISTICS", gzStats) && !gzStats.empty()) | ||
| if (!versionInitialized && | ||
| env("GZ_TRANSPORT_TOPIC_STATISTICS", gzStats) && !gzStats.empty()) | ||
| { | ||
| topicStats = (gzStats == "1"); | ||
| } | ||
|
|
||
| versionInitialized = true; | ||
|
|
There was a problem hiding this comment.
hmm, thinking about it some more. It's probably still not safe from a data race. versionInitialized, even though atomic, the whole processes of checking the environment variable and setting topicStats is not atomic.
My suggestion would be to use std::call_once with a local lambda
There was a problem hiding this comment.
Since this is the version targeting main and we can break ABI, I simplified the logic in 28aadf9.
There was a problem hiding this comment.
Okay, but note that we'll need to backport this because #701 occurs on Jetty.
There was a problem hiding this comment.
Absolutely, the backport will be different, we need to preserve ABI.
Signed-off-by: Carlos Agüero <[email protected]>
|
@Mergifyio backport gz-transport15 |
✅ Backports have been created
|
* Only check GZ_TRANSPORT_TOPIC_STATISTICS once Signed-off-by: Carlos Agüero <[email protected]> (cherry picked from commit 815b417)
🦟 Bug fix
Fixes #701
Summary
According to #701, checking an environment variable every time that we're inside the
Version()function might cause a crash. This patch only checks the environment variable once.There's some behavior change here because you cannot change the value of the environment variable in the middle of a gz-transport session anymore. I think it's probably a good idea not allowing that but I can be convinced otherwise.
Checklist
codecheckpassed (See contributing)Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining
Signed-off-byandGenerated-bymessages.