Include localhost in ros2 doctor hello connectivity checks - #1269
Include localhost in ros2 doctor hello connectivity checks#1269sylvesterkaczmarek wants to merge 2 commits into
Conversation
Count local topic and multicast communication so `ros2 doctor hello` can validate connectivity on a single host. Ignore the internal UDP wake-up packet used during receiver shutdown. Signed-off-by: Sylvester Kaczmarek <16242628+sylvesterkaczmarek@users.noreply.github.com>
|
Tick the box to add this pull request to the merge queue (same as
|
fujitatomoya
left a comment
fujitatomoya
left a comment
There was a problem hiding this comment.
@sylvesterkaczmarek thanks for creating PR.
i assume you are using AI agent to create this PR, that is fine. but we have AI guideline: https://discourse.openrobotics.org/t/guidelines-for-ai-assisted-contributions-in-the-ros-project/55903 and please use our PR template to disclose the AI tools.
| if self._is_shutdown: | ||
| break |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| hello_verb = HelloVerb() | ||
| hello_verb.main(args=args, summary_table=summary) | ||
| expected_summary = _generate_expected_summary_table() | ||
| expected_summary = _generate_expected_summary_table(hostname) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Use a per-instance socket pair to wake the multicast receiver without leaking shutdown traffic to other ros2 doctor instances, and wait deterministically for the local topic and multicast round-trip in --once mode. Signed-off-by: Sylvester Kaczmarek <16242628+sylvesterkaczmarek@users.noreply.github.com>
ef9512d to
53a34f4
Compare
|
@sylvesterkaczmarek thanks for fixing up, out of curiosity what kind of AI models and tools are used to generate this code? |
2 participants
Summary
Addresses the localhost-connectivity part of #1077.
ros2 doctor hellocurrently excludes messages originating from the same hostname, so it cannot validate local topic delivery or multicast loopback. Count those messages as well so the command reports single-host connectivity.The UDP receiver shutdown path now uses a per-instance socket pair instead of sending a wake-up datagram to the shared UDP port, so one
ros2 doctor helloinstance cannot wake or contaminate another instance on the same host. In--oncemode, the command waits with a bounded timeout for the local topic and multicast round-trip instead of assuming both complete within one 0.1-second emit period.The existing single-host CLI test now expects one local topic subscription and one local multicast receive.
Testing
Did you use Generative AI?
Yes. AI was used to assist with tests.