Skip to content

Only check GZ_TRANSPORT_TOPIC_STATISTICS once (backport #731) #743

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: gz-transport15
Choose a base branch
from
Open

Only check GZ_TRANSPORT_TOPIC_STATISTICS once (backport #731) #743

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2b56275
Only check GZ_TRANSPORT_TOPIC_STATISTICS once (#731)
caguero Oct 24, 2025
7e6209e
ABI
caguero Oct 31, 2025
e47c0a6
Tweak
caguero Oct 31, 2025
ac61c48
inline
caguero Oct 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 17 additions & 9 deletions src/Discovery.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,11 @@ namespace gz
this->hostInterfaces = determineInterfaces();
}

// 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;
Copy link
Contributor

@azeey azeey Oct 31, 2025

Choose a reason for hiding this comment

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

Can we be sure that Discovery is constructed only once?

Copy link
Collaborator

@caguero caguero Oct 31, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@azeey azeey Oct 31, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@caguero caguero Oct 31, 2025

Choose a reason for hiding this comment

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

The reason to make it static is to preserve ABI.


#ifdef _WIN32
WORD wVersionRequested;
WSADATA wsaData;
Expand Down Expand Up @@ -1568,15 +1573,7 @@ namespace gz
/// \return The discovery version.
private: uint8_t Version() const
{
static std::string gzStats;
static int topicStats;

if (env("GZ_TRANSPORT_TOPIC_STATISTICS", gzStats) && !gzStats.empty())
{
topicStats = (gzStats == "1");
}

return this->kWireVersion + (topicStats * 100);
return this->wireVersion;
}

/// \brief Register a new network interface in the discovery system.
Expand Down Expand Up @@ -1653,8 +1650,14 @@ namespace gz

/// \brief Wire protocol version. Bump up the version number if you modify
/// the wire protocol (for discovery or message/service exchange).
/// Note: Deprecated: Use wireVersion instead.
private: static const uint8_t kWireVersion = 10;

/// \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;
Copy link
Contributor

@azeey azeey Oct 31, 2025

Choose a reason for hiding this comment

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

Suggested change
private: static uint8_t wireVersion;
private: static inline uint8_t wireVersion = 10;

Copy link
Collaborator

@caguero caguero Oct 31, 2025

Choose a reason for hiding this comment

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

Done in ac61c48.


/// \brief Port used to broadcast the discovery messages.
private: int port;

Expand Down Expand Up @@ -1763,6 +1766,11 @@ namespace gz
private: bool useZenoh = false;
};

/// \brief wireVersion definition.
/// \TODO(caguero): Remove in Gazebo K.
template<typename Pub>
uint8_t Discovery<Pub>::wireVersion = 10u;

Copy link
Contributor

@azeey azeey Oct 31, 2025

Choose a reason for hiding this comment

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

This can be removed if we use static inline as suggested.

Copy link
Collaborator

@caguero caguero Oct 31, 2025

Choose a reason for hiding this comment

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

Thanks, removed in ac61c48.

/// \def MsgDiscovery
/// \brief A discovery object for topics.
using MsgDiscovery = Discovery<MessagePublisher>;
Expand Down
Loading