Skip to content

Include localhost in ros2 doctor hello connectivity checks #1269

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
sylvesterkaczmarek wants to merge 2 commits into
base: rolling
Choose a base branch
from
+51 −25
Open

Include localhost in ros2 doctor hello connectivity checks #1269

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
692d98e
Include localhost in ros2 doctor hello checks
sylvesterkaczmarek Aug 20, 2026
53a34f4
fix(doctor): make localhost hello synchronization robust
sylvesterkaczmarek Aug 20, 2026
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
12 changes: 6 additions & 6 deletions ros2doctor/ros2doctor/verb/hello.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,8 +160,7 @@ def destroy(self):
def _callback(self, msg):
msg_data = msg.data.split()
pub_hostname = msg_data[-1]
if pub_hostname != socket.gethostname():
self._summary_table.increment_sub(pub_hostname)
self._summary_table.increment_sub(pub_hostname)


class HelloMulticastUDPSender:
Expand Down Expand Up @@ -223,10 +222,11 @@ def recv(self):
try:
while not self._is_shutdown:
data, _ = self._socket.recvfrom(4096)
if self._is_shutdown:
break

@fujitatomoya fujitatomoya Aug 20, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

how does it guarantee that is come from the same instance shutdown though? guessing this is flaky and can leak into other instances on the same host?

@sylvesterkaczmarek sylvesterkaczmarek Aug 20, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Good point. The shared UDP wake-up could be delivered to another receiver bound to the same port. I replaced it with a per-instance socket.socketpair() used only to wake that receiver thread, so shutdown signaling can no longer leak into another ros2 doctor hello instance.

data = data.decode('utf-8')
sender_hostname = data.split()[-1]
if sender_hostname != socket.gethostname():
self._summary_table.increment_receive(sender_hostname)
self._summary_table.increment_receive(sender_hostname)
except socket.timeout:
pass

Expand Down Expand Up @@ -267,7 +267,7 @@ def increment_pub(self):
self._pub += 1

def increment_sub(self, hostname):
"""Increment subscribed msg count from different host(s)."""
"""Increment subscribed msg count from host(s)."""
with self.lock:
if hostname not in self._sub:
self._sub[hostname] = 1
Expand All @@ -280,7 +280,7 @@ def increment_send(self):
self._send += 1

def increment_receive(self, hostname):
"""Increment multicast-received msg count from different host(s)."""
"""Increment multicast-received msg count from host(s)."""
with self.lock:
if hostname not in self._receive:
self._receive[hostname] = 1
Expand Down
11 changes: 7 additions & 4 deletions ros2doctor/test/test_cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,12 +42,14 @@ def generate_test_description():
])


def _generate_expected_summary_table():
def _generate_expected_summary_table(hostname):
"""Generate expected summary table for one emit period on a single host."""
expected_summary = SummaryTable()
# 1 pub/send per default emit period
# 1 pub/send and matching local sub/receive per default emit period
expected_summary.increment_pub()
expected_summary.increment_sub(hostname)
expected_summary.increment_send()
expected_summary.increment_receive(hostname)
return expected_summary


Expand All @@ -61,11 +63,12 @@ def test_hello_single_host(self):
args.print_period = 1.0
args.ttl = None
args.once = True
with mock.patch('socket.gethostname', return_value='!nv@lid-n*de-n4me'):
hostname = '!nv@lid-n*de-n4me'
with mock.patch('socket.gethostname', return_value=hostname):
summary = SummaryTable()
hello_verb = HelloVerb()
hello_verb.main(args=args, summary_table=summary)
expected_summary = _generate_expected_summary_table()
expected_summary = _generate_expected_summary_table(hostname)

@fujitatomoya fujitatomoya Aug 20, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

with PR, the following assertion is only correct if two independent round-trips both complete within roughly that one 0.1-second window, i suggest that is tough and this is gonna be flaky test.

@sylvesterkaczmarek sylvesterkaczmarek Aug 20, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Agreed. I removed the 0.1-second timing assumption. In --once mode the command now waits, with a bounded 5-second timeout, for both the local topic subscription and multicast receive before producing the summary.

self.assertEqual(summary._pub, expected_summary._pub)
self.assertEqual(summary._sub, expected_summary._sub)
self.assertEqual(summary._send, expected_summary._send)
Expand Down