Only check GZ_TRANSPORT_TOPIC_STATISTICS once (backport #731) #743
Conversation
* Only check GZ_TRANSPORT_TOPIC_STATISTICS once Signed-off-by: Carlos Agüero <[email protected]> (cherry picked from commit 815b417)
Signed-off-by: Carlos Agüero <[email protected]>
Signed-off-by: Carlos Agüero <[email protected]>
src/Discovery.hh
Outdated
| /// \brief Wire protocol version. Bump up the version number if you modify | ||
| /// the wire protocol (for discovery or message/service exchange). | ||
| /// \TODO(caguero): Remove static in Gazebo K. | ||
| private: static uint8_t wireVersion; |
There was a problem hiding this comment.
| private: static uint8_t wireVersion; | |
| private: static inline uint8_t wireVersion = 10; |
src/Discovery.hh
Outdated
| /// \brief wireVersion definition. | ||
| /// \TODO(caguero): Remove in Gazebo K. | ||
| template<typename Pub> | ||
| uint8_t Discovery<Pub>::wireVersion = 10u; | ||
|
|
There was a problem hiding this comment.
This can be removed if we use static inline as suggested.
| // Update the wire version if GZ_TRANSPORT_TOPIC_STATISTICS is set. | ||
| std::string gzStats; | ||
| if (env("GZ_TRANSPORT_TOPIC_STATISTICS", gzStats) && gzStats == "1") | ||
| this->wireVersion += 100; |
There was a problem hiding this comment.
Can we be sure that Discovery is constructed only once?
There was a problem hiding this comment.
It's constructed twice, one for msgs and one for services here.
Are you thinking on race conditions? If so, they're constructed in sequence, so I think we should be good.
There was a problem hiding this comment.
No, I'm concerned that the since wireVersion is a static variable, every construction of the same template type would increment the wire version. I guess I'm not sure why it needs to be static at all if each instance will end up computing the wireVersion anyway.
There was a problem hiding this comment.
The reason to make it static is to preserve ABI.
Signed-off-by: Carlos Agüero <[email protected]>
🦟 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.This is an automatic backport of pull request #731 done by Mergify.