feat(rtps): endpoint priority — banded channels, dedicated SEDP-advertised ports, deferred dispatch (phase 2) - #737
Conversation
…t ports in the engine
- EsppTransport: every receive channel takes ChannelOptions{band, dscp} -
the reactor dispatches the socket at the band (espp::QosBand) and marks
the socket's outgoing traffic with the optional DSCP; submit() takes a band.
- Domain: DomainConfig{metatraffic_band=High, user_traffic_band=Normal,
enable_dedicated_endpoint_ports, max_prioritized_endpoint_ports=4}.
SPDP/SEDP channels register at the metatraffic band (High by default) so
discovery dispatch overtakes queued user traffic; user channels at Normal.
- Per-endpoint priority: createWriter/createReader take EndpointOptions
{band, dscp}. A non-Normal band (or a dscp) requests a dedicated unicast
port, allocated deterministically at offset 100+ of the domain's RTPS port
block (7400+250*domain+100+n, linear probe with reuse-disabled bind) and
rationed by max_prioritized_endpoint_ports (each port is one fd; lwIP has
~10). The endpoint's SEDP announcement then carries the dedicated port in
its standard PID_UNICAST_LOCATOR (wire-format unchanged - only the port
value differs), so FastDDS/ROS 2 peers send that endpoint's traffic there,
and the endpoint sends FROM the dedicated (DSCP-marked) socket since
m_srcPort follows the unicast locator. Received datagrams on dedicated
ports route by a port->participant registry; entity demux is unchanged.
Ports are released on endpoint deletion and on creation failure.
- TopicData: local-only band/dscp/hasDedicatedPort attributes (never
serialized; SEDP encoding is byte-identical - golden tests unchanged).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ispatch fallback - RtpsParticipant::Config: metatraffic_band (High default), user_traffic_band, enable_dedicated_endpoint_ports, max_prioritized_endpoint_ports (4) - passed through to the engine DomainConfig. - WriterConfig/ReaderConfig: band (espp::QosBand) + optional dscp (espp::Dscp). Banded endpoints request a dedicated port; when none is granted (ration exhausted or disabled), banded READERS fall back to deferred banded dispatch: each sample is queued (bounded, 32/reader, newest dropped with a warning) and delivered by a single in-flight job re-submitted to the transport pool at the reader's band - one delivery per job, mirroring the reactor's one-shot arming, so per-reader ordering is preserved. Default-path readers keep the exact inline delivery. - ServiceConfig (band/dscp on both request+reply endpoints) and ActionConfig (inherited by all underlying service/topic endpoints) likewise; banded shared-port service servers run their handler deferred at the band, banded shared-port service clients defer the user-facing reply delivery. Native services/actions inherit via their pub/sub readers. - Typed facades (Publisher/Subscriber, ServiceServer/Client, ActionServer/ Client) expose the same band/dscp config fields. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Config gains metatraffic_band / user_traffic_band / enable_dedicated_endpoint_ports / max_prioritized_endpoint_ports; add_writer / add_reader / service + action (ROS and native) creation methods gain band (espp.QosBand, default Normal) and dscp (espp.Dscp | None) keyword arguments. The committed .pyi stub has no RtpsParticipant surface, so no stub update is needed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ispatch, ration exhaustion - rtps_sedp_dedicated_locator: engine-level - banded reader/writer get a dedicated port from the documented range; the SEDP announcement carries it in a byte-exact PID_UNICAST_LOCATOR parameter (plus round-trip parse); default endpoints keep the shared port unchanged; ration cap enforced; deleted endpoints return their port; disabled dedicated ports honored. - rtps_banded_pubsub: facade publisher -> engine-level banded subscriber on a dedicated port; end-to-end delivery proves the traffic flows through the dedicated socket (the announcement carries only that locator). - rtps_banded_deferred: banded reader with dedicated ports disabled receives all 30 sequence-numbered samples strictly in order via deferred dispatch. - rtps_banded_ration: cap=1 with two banded readers - the over-cap reader logs, falls back, and both still receive everything. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Deferred-dispatch closures capture shared_ptr payloads instead of moved vectors: keeps them cheaply copyable inside std::function and sidesteps a GCC 15 (xtensa, -O2 -Werror) -Wfree-nonheap-object false positive that broke the ESP32 build. - Domain: drop the always-true seed in initializeTransport's success chain, use std::find_if for the dedicated-port lookup (cppcheck). - Tests: pointer-to-const where cppcheck asked. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… fd ration - README: new 'Priority scheduling (bands, dedicated ports, DSCP)' section; architecture diagram notes the QosBand-priority reactor/pool. - doc/en/protocols/rtps.rst: 'Ports and Channels' gains the dedicated-port row (7400 + 250*domain + 100 + n) and a 'Per-endpoint priority (dedicated ports)' subsection covering the deterministic allocation, SEDP PID_UNICAST_LOCATOR announcement (wire-format unchanged), DSCP marking, the fd-budget rationing, and the deferred banded dispatch fallback. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
rtps_interop_sub gains a band argument (0=Critical..3=Low, default Normal); the matrix adds ros2_pub->espp_banded_sub: a QosBand::High espp reader on a dedicated unicast port receiving from a ROS 2 publisher, proving FastDDS honors the announced per-endpoint unicast locator. The banded loopback tests also run in the container. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… stale phase-2 port-collision note Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
✅Static analysis result - no issues found! ✅ |
There was a problem hiding this comment.
Pull request overview
This PR implements Phase 2 of priority-aware RTPS scheduling by threading espp::QosBand (and optional DSCP) through the RTPS transport, adding per-endpoint dedicated unicast ports (announced via standard SEDP PID_UNICAST_LOCATOR), and providing a shared-port fallback via deferred banded dispatch for user callbacks.
Changes:
- Add per-channel
QosBandscheduling to RTPS transport receive ports, with metatraffic defaulting toHigh. - Implement per-endpoint dedicated unicast ports (rationed) and route inbound dedicated-port traffic to the owning participant.
- Add deferred banded dispatch for banded shared-port readers / service handlers / client replies, plus Python/docs/tests/interop coverage.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pc/tests/rtps_sedp_dedicated_locator.cpp | New engine-level test validating dedicated-port allocation + SEDP locator encoding + ration + release. |
| pc/tests/rtps_interop_sub.cpp | Extend interop subscriber tool to accept a band argument and pass it into the reader config. |
| pc/tests/rtps_banded_ration.cpp | New end-to-end test proving ration exhaustion falls back while still delivering to both readers. |
| pc/tests/rtps_banded_pubsub.cpp | New loopback test proving traffic flows via the dedicated port announced in SEDP. |
| pc/tests/rtps_banded_deferred.cpp | New loopback test proving deferred shared-port dispatch preserves per-reader ordering. |
| lib/python_bindings/rtps_bindings.cpp | Expose participant config fields + band/dscp on endpoint creation APIs in Python. |
| doc/en/protocols/rtps.rst | Document banded channels, dedicated endpoint ports, and deferred dispatch behavior. |
| components/rtps/src/rtps_participant.cpp | Wire participant config into rtps::DomainConfig, pass endpoint options, and implement deferred dispatch helper + usage. |
| components/rtps/src/entities/Domain.cpp | Add DomainConfig, per-channel banding, dedicated-port allocation/release, and dedicated-port receive routing. |
| components/rtps/src/communication/EsppTransport.cpp | Add per-channel ChannelOptions (band/dscp) and band-aware submit() to the transport worker pool. |
| components/rtps/README.md | Add a “Priority scheduling” section describing bands, dedicated ports, DSCP, and deferred dispatch. |
| components/rtps/interop/run_interop.sh | Build and run new priority/dedicated-port tests; add ROS2→banded-espp interop scenario. |
| components/rtps/include/rtps/entities/Domain.hpp | Define DomainConfig + EndpointOptions, extend constructors/create APIs, expose getTransport(). |
| components/rtps/include/rtps/discovery/TopicData.hpp | Add local-only band/dscp/hasDedicatedPort attributes (not serialized). |
| components/rtps/include/rtps/communication/EsppTransport.hpp | Define ChannelOptions; extend transport APIs for band/DSCP-aware channels and submissions. |
| components/rtps/include/rtps_service.hpp | Extend typed service facade configs to carry band/dscp into underlying endpoints. |
| components/rtps/include/rtps_pubsub.hpp | Extend typed publisher/subscriber facade configs to carry band/dscp. |
| components/rtps/include/rtps_participant.hpp | Extend public configs (participant/writer/reader/service/action) and define deferred dispatch helper. |
| components/rtps/include/rtps_action.hpp | Extend typed action facade configs to carry band/dscp into underlying endpoints. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…iness can never wedge stop() select() readiness can be stale or spurious for UDP (Linux documents that a subsequent read may still block, e.g. a checksum-failed datagram discarded in between; a just-closed fd's readiness can also alias onto a reused fd number). add_udp_receiver()'s handler used an unbounded blocking recvfrom, so one such dispatch never finished and SocketReactor::stop()'s in-flight wait hung forever. Set a 1 s receive timeout on registration: invisible on the data path (reads only follow readiness) and guarantees every dispatch - and therefore stop() - makes progress. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…hang)
Runtime endpoint deletion (new in the per-endpoint-priority work: dedicated
ports are released by deleteReader/deleteWriter) exposed one regression and a
family of latent engine races, reproduced on Linux with a dedicated-port
churn-under-flood stress and diagnosed from gdb stack dumps of the hung/
crashed processes:
1. releaseReceivePort() destroyed the channel's UdpSocket immediately after a
NON-blocking SocketReactor::remove() - but remove() defers unregistration
while a dispatch is in flight, and that dispatch's handler references the
socket. The handler was observed blocked forever acquiring the FREED
object's internal logger mutex, which wedged SocketReactor::stop()'s
in-flight wait: the exact CI hang ("Still waiting for 1 in-flight
handler(s)" repeating for 24 min). Fix: RETIRE the socket (park it in
m_retiredSockets, fd stays open so stale readiness can't alias onto a
reused fd) and close it in stop() after the reactor and pool have
quiesced. Shutdown never waits on the released socket.
2. Lock-order inversion, Participant vs SEDPAgent: add/deleteReader/Writer
held Participant::m_mutex while calling into the SEDP agent (which locks
SEDPAgent::m_mutex), while the agent's receive handlers lock
SEDPAgent::m_mutex and then call back into the participant - a classic
ABBA deadlock under concurrent discovery traffic (second reproduced hang:
deleteReader vs handlePublisherReaderMessage). Fix: the slot bookkeeping
stays under m_mutex, the SEDP call moves OUTSIDE it; the global order is
SEDPAgent::m_mutex -> Participant::m_mutex, never the reverse. The
deletion loops now also match by pointer identity, fixing a null-slot
dereference (the old sequence-number comparison dereferenced empty
slots).
3. Domain's dedicated-port registry gets its own small mutex: the port ->
participant lookup runs on the receive workers, and routing it through
Domain::m_mutex let an API caller stall every receive worker (observed as
part of the deadlocked state).
4. Unlocked proxy-pool accesses that race SEDP (un)matching under endpoint
churn - StatefulWriter::sendHeartBeat (reproduced SIGSEGV iterating
m_proxies from the protocol task while a receive worker mutated them),
StatelessWriter::progress, StatefulReader/StatelessReader::
addNewMatchedWriter, Reader::isProxy/getProxy - now take the designated
mutex (Writer::m_mutex / Reader::m_proxies_mutex) that every other
accessor already used.
Verified: the churn reproducer (rtps_banded_churn, next commit) hung at
iter 5 and crashed at iter 6/26 before these fixes; afterwards 100/100
runs pass on Linux (docker, DDS noise) plus 20x with the final binaries.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…entry The reproducer for the CI shutdown hang, kept as a regression test. Phase 1: while a publisher floods a reliable topic, the subscriber domain repeatedly (25x) creates a banded dedicated-port reader, receives live traffic, and deletes it - exercising releaseReceivePort() with dispatches in flight, SEDP (un)announcements racing the API, and immediate port/slot reuse. Phase 2: a banded SHARED-port reader with deferred banded dispatch and a slow callback is stopped WHILE deliveries are in flight and queued. Any shutdown hang trips the harness timeout. Before the teardown fixes this hung at iteration 5 and segfaulted at iteration 6; it now passes 100/100 on Linux (docker) and 3/3 on macOS. Also run (with a 120 s timeout) in the docker interop matrix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…erop band arg - python add_reader(): the docstring claimed only a non-Normal band requests a dedicated receive port; a set dscp does too (matching add_writer's wording and the C++ ReaderConfig docs). - rtps_interop_sub: validate the band argument (0..3) before casting to espp::QosBand instead of propagating an unchecked value into band-indexed code; fail fast with a clear message. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The CI interop hang is fixed in c573c39..ef323f4 — the 30-minute timeout kill turned out to be a real shutdown deadlock, and reproducing it on Linux (gdb watchdog inside the interop image) surfaced three distinct concurrency bugs, two of them pre-existing in the engine and merely exposed by runtime endpoint deletion:
Defense-in-depth: the reactor's UDP receive is bounded with a 1 s Evidence: new |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
pc/tests/rtps_banded_deferred.cpp:76
- The comment says each sample is sent until it is "acknowledged by observation" via paced resends of the head, but the loop actually sends each sequence number at most once (next_to_send is always incremented on a successful publish and never retried). Either adjust the comment to match the current send strategy, or implement the described resend behavior.
// Wait for discovery/matching, then send the numbered sequence. Reliable
// writers retransmit on the wire, but a sample must reach the reader at least
// once for the deferred queue to see it - send each one until acknowledged by
// observation (simple paced resend of the not-yet-seen head).
New overload remove(id, on_removed): the callback fires EXACTLY ONCE when the registration is fully gone AND any handler that was running or pending for it has finished - i.e. when no reactor code can reference the socket/fd anymore - so callers finally have a non-blocking way to know when destroying the socket is safe (remove() itself never blocks and defers erasure while a dispatch is in flight). Covers all three completion paths: immediate erase (idle at remove() time; callback runs synchronously on the caller), deferred erase (fires on the pool worker right after the in-flight handler returns), and the pool-saturated dispatch revert (fires on the reactor loop). Invoked without reactor locks (may re-enter the reactor; must return promptly; must not call stop() from worker/loop context); repeated remove() for a pending id chains the callbacks; the erase paths wake the loop so the fd leaves the select interest set promptly. Doxygen documents the guarantees, threading, and the residual one-iteration stale-select caveat (bounded by the add_udp_receiver receive timeout). Python remove(id) binding unchanged (cast disambiguates the overload). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… completion Addresses the PR review on the retire strategy: parking released sockets until stop() kept each fd open (and the port bound) for the transport lifetime, so endpoint churn could accumulate fds and defeat the dedicated-port ration on small-socket platforms (lwIP ~10). releaseReceivePort() still parks the socket in m_retiredSockets (the object must outlive any in-flight handler), but now passes an on_removed completion to SocketReactor::remove() that destroys the retired socket - closing the fd and unbinding the port - the moment the reactor confirms the registration is fully gone and no handler can reference it. For an idle registration (the common case) that is synchronous within releaseReceivePort(); with a handler in flight it fires right after that handler finishes. destroyRetiredSocket() erases by pointer under m_mutex, so racing stop()'s clearing of the retired list is a benign no-op (the pointer is only used to find the entry, never dereferenced), and the recursive mutex makes the synchronous-callback path (caller already holds m_mutex) safe. stop() remains the backstop for any socket whose completion never fired (e.g. the pool died first). retiredSocketCount() is exposed for tests/diagnostics. Tests now prove the prompt release: rtps_sedp_dedicated_locator binds a fresh reuse-disabled socket to the released dedicated port (and sees zero retired sockets) well before the domain stops; rtps_banded_churn asserts that after 25 delete/create cycles under flood the retired list drains to zero and the FIRST iteration's port is bindable again before stop. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 29 changed files in this pull request and generated 4 comments.
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
components/rtps/src/rtps_participant.cpp:673
- These two endpoint creations are not transactional. If the writer succeeds (and consumes a dedicated-port slot) but the reader pool is exhausted, the method returns false while leaving the writer announced and its dedicated socket allocated; the inverse case similarly leaves a reader behind. Roll back whichever endpoint was created before returning failure so failed service registration cannot permanently consume endpoint and fd budgets.
This issue also appears on line 1172 of the same file.
domain_->createWriter(*participant_, rep_topic.c_str(), rep_type.c_str(), /*reliable=*/true,
/*enforceUnicast=*/false, endpoint_options);
rtps::Reader *request_reader =
domain_->createReader(*participant_, req_topic.c_str(), req_type.c_str(), /*reliable=*/true,
/*mcastaddress=*/{0, 0, 0, 0}, endpoint_options);
components/rtps/src/entities/Domain.cpp:413
- A failed probe window never advances
m_nextDedicatedPortOffset; it is updated only on a successful bind. If offsets 0–15 are occupied while offset 16 is free, every later endpoint retries the same 16 ports and dedicated allocation remains permanently disabled despite the documented 100–249 range. Advance past the failed window (bounded by the domain block) before returning fallback.
for (uint16_t probe = 0; probe < DEDICATED_PORT_PROBE_LIMIT; ++probe) {
const uint16_t offset = m_nextDedicatedPortOffset + probe;
if (DEDICATED_PORT_OFFSET + offset > 249) {
break; // stay inside this domain's 250-port block
components/rtps/src/rtps_participant.cpp:1176
- As in the server path, partial creation leaks the successful endpoint. For example, a successful dedicated reply reader followed by request-writer exhaustion makes
add_service_client()fail while retaining the reader/socket and ration slot. Delete whichever endpoint succeeded before returning failure.
domain_->createReader(*participant_, rep_topic.c_str(), rep_type.c_str(), /*reliable=*/true,
/*mcastaddress=*/{0, 0, 0, 0}, endpoint_options);
rtps::Writer *request_writer =
domain_->createWriter(*participant_, req_topic.c_str(), req_type.c_str(), /*reliable=*/true,
/*enforceUnicast=*/false, endpoint_options);
…stration If the SO_RCVTIMEO install fails, the unbounded-recvfrom hang guard is void, so registration now fails with a clear error instead of proceeding. Evaluated O_NONBLOCK as the alternative: it would also bound reads, but this fd is used for SENDS too (the owner and the echo path), and non-blocking mode changes send semantics under buffer pressure (EWOULDBLOCK instead of a brief block). SO_RCVTIMEO bounds only receives and is supported on POSIX, lwIP (LWIP_SO_RCVTIMEO), and Windows, so it remains the mechanism - now mandatory. (PR #737 review.) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 61 out of 62 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
Previously missed (2) — in code that hasn't changed since the last review.
components/rtps/src/rtps_participant.cpp:223
- The engine rejects names whose length is exactly the configured array size (
Domain::createWriter()uses>=), but this diagnostic uses>. A 64-byte topic/type therefore reports a writer-capacity failure instead of the actual name-length failure and points users at the wrong Kconfig knobs.
This issue also appears on line 280 of the same file.
components/rtps/src/communication/EsppTransport.cpp:268
- Keeping one owed run per rejected publish creates an unbounded work debt for static KEEP_LAST writers. Once their finite history overwrites older samples, most of these counts no longer correspond to sendable data; after a sustained overload the retry timer can spend minutes submitting millions of no-op
progress()jobs, continuing to occupy the bounded pool long after traffic stops. Coalesce/cap debt when history drops and have admitted work drain the currently retained history instead of preserving pokes for samples that no longer exist.
// Coalesce by producer: a repeated poke for the same writer just bumps its
// owed count (the job/band are identical), so the map is bounded by the
// producer count and nothing is ever dropped. Each owed run re-invokes
// progress() once, preserving one-poke/one-sample.
auto &entry = m_pendingByKey[key];
entry.job = std::move(job);
entry.band = band;
++entry.count;
components/rtps/src/rtps_participant.cpp:281
- The reader engine rejects names at exactly
MAX_*_LENGTH(Domain::createReader()uses>=), so this>check misclassifies that failure as pool exhaustion and emits an incorrect remediation.
components/rtps/src/rtps_participant.cpp:734 ServiceResponderis explicitly storable beyond the handler invocation, but this raw reply-writer lifetime is only guarded by the participant-wide token used duringstop(). During action rollback,remove_service_server()can delete/reset this writer afterclose()returns while a retained responder still sees the participant as live, so a later or racingreply()can target a reused writer slot. Add an endpoint-scoped liveness/generation guard shared with responders and invalidate it before deleting this writer.
…-scoped responder liveness; name-length bound Round-22 review fixes (1 inline + 4 previously-missed comments). 1) Writer progress pokes now use DRAIN semantics instead of per-poke owed counts: new EsppTransport::submitGuaranteedDrain() parks with a pending-count capped at ONE, and both writers' progress() re-arm themselves while unsent samples remain (StatelessWriter: cursor <= getSeqNumMax; StatefulWriter: cursor <= getCurrentSeqNumMax). Under a KEEP_LAST overflow storm the old per-sample counts accumulated unbounded debt for samples the ring had already overwritten - millions of no-op pokes ground through the retry timer after the storm. Now the parked debt per writer is bounded at one and each admitted run drains the RETAINED history (send one, re-arm) - still lossless. submitGuaranteed() keeps its counted lossless contract for other producers (and its tests). The drain chain sends a burst back-to-back at pool speed instead of the old accidental retry-timer metering, so the saturation test's dynamic-mode gate is retuned to the deterministic properties (publish accepted all + drops == 0, i.e. every sample reached the wire) with a collapse floor of 80%: a slower container receiver's kernel buffer can now shed a few percent as genuine best-effort wire loss (observed 95.2% in docker, 100% on host), while a chain/parking regression delivers at most the pool-queue prefix (~13%). 2) DSCP code points are validated at BOTH layers before any port is probed: Domain::createWriter/createReader reject > 63 with an explicit error (Socket::set_dscp can never apply it, so probing would burn dedicated-port offsets on binds whose marking fails and then silently fall back), and the facade's add_writer/add_reader reject early with the same guidance. 3) ServiceResponder gains an ENDPOINT-scoped liveness token alongside the participant-wide one: remove_service_server() flips it false (under its lock, waiting out an in-flight reply()) BEFORE deleting the reply writer, so a responder retained past an individual server removal (e.g. an action rollback) no-ops instead of writing through a reset/reused writer slot while the participant is still alive. Lock order participant -> endpoint; removal takes the endpoint lock alone, so no reverse nesting. 4) The facade's name-too-long diagnostics now use >= (matching the engine's fixed-array + terminating-NUL bound), so a name of exactly MAX_TOPICNAME/TYPENAME_LENGTH is attributed to its real cause instead of the pool-exhaustion hint. Verified: rtps_guaranteed_submit/fairness (counted contract intact, all owed runs execute), rtps_stateless_saturation 100% (drain chain lossless), docker interop matrix 43/43 PASS (incl. the static-storage saturation gate); standalone sweep 3x (only the known port-in-use flake once, passes solo); TSan spot-checks (guaranteed_submit/fairness/saturation/writer_churn) 0 findings; cppcheck clean (writer syntaxError is a pre-existing DEBUG_BUILD config false positive, present at HEAD); esp32 rtps example builds clean.
|
Addressed the 4 suppressed/previously-missed findings from the latest review round in 58a6934 (the inline DSCP finding is answered on its thread):
Verified: |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 61 out of 62 changed files in this pull request and generated 2 comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
pc/tests/rtps_remove_reader_deadlock.cpp:197
- This wait stops as soon as the callback sets
self_removed, before it stores theremove_reader()result intoself_removed_ok. The main thread can therefore reach the check at line 194 during that window and fail a successful run nondeterministically. Wait for completion/result (or add a separateself_remove_doneatomic) rather than the entry flag.
…elow dedicated port range; test completion race Round-23 review fixes (2 inline + 1 previously-missed comment). 1) Stale-participant removal now runs in the documented SEDP -> participant lock order. checkAndResetHeartbeats() previously held the SPDP-agent and participant mutexes while calling removeRemoteParticipant(), whose nested SEDP call (removeUnmatchedEntitiesOfParticipant) takes the SEDP-agent mutex - an ABBA inversion against the SEDP receive handlers, which hold that mutex and call findRemoteParticipant() (participant mutex). The scan is now phase 1 (SELECT the expired prefix under SPDP -> participant, read-only), the locks are released, and phase 2 removes through removeRemoteParticipant(), which itself now acquires the SEDP-agent mutex BEFORE the participant mutex so any caller gets the documented order. A liveness refresh racing the unlocked window loses by design: the lease already expired, and SPDP rediscovery re-adds the participant. 2) The dedicated endpoint-port range's non-collision with the standard RTPS unicast ports is now ENFORCED, not assumed: participant-id probing is capped (while dedicated ports are enabled) at the last id whose standard user-unicast offset (D3 + PG*id, the larger of the two) stays below DEDICATED_PORT_OFFSET - i.e. id 44. Past that, the shared user port lands inside the dedicated range, where a later dedicated-port probe would hit the already-bound port, ensureReceivePort() would report the existing shared channel as a successful "dedicated" allocation, and the registry would route the participant's user traffic to the wrong participant. Creation now fails with an explicit log instead. The Domain.hpp range comment now states the enforcement rather than claiming impossibility. 3) rtps_remove_reader_deadlock scenario 2 waits on a completion flag set AFTER the remove_reader() result is stored, instead of the entry flag set before the call - the old wait could judge self_removed_ok mid-removal and fail a good run nondeterministically. Verified: rtps_remove_reader_deadlock 3x + TSan; standalone sweep 3x 0 fail; TSan spot-checks (remove_reader_deadlock/writer_churn/guaranteed_fairness) 0 findings (the lock-order change is additionally validated by the CI linux TSan deadlock detector); docker interop matrix 43/43 PASS; cppcheck clean (only pre-existing style notes); esp32 rtps example builds clean.
|
Also addressed the 1 previously-missed comment from the latest review round in 673ae02: rtps_remove_reader_deadlock.cpp scenario 2 waited on the entry flag ( Verified: |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 61 out of 62 changed files in this pull request and generated 2 comments.
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
lib/espp.cmake:161
- This “participant budget” validation ignores the transport channel pool. A Domain permanently uses two multicast channels plus two unicast channels per participant, so the host defaults already need 18 channels for
MAX_NUM_PARTICIPANTS=8whileConfig::MAX_NUM_UDP_CONNECTIONSis 16 (host_large similarly needs 66 but has 32). Overrides can therefore pass this check yetcreateParticipant()still fails before reaching the advertised participant capacity. Include the channel constraint, and either raise the profile channel caps or lower the participant defaults.
components/rtps/CMakeLists.txt:120
- This cross-limit check still accepts an undeliverable participant budget because it omits
MAX_NUM_UDP_CONNECTIONS: the Domain consumes two multicast channels plus two unicast channels per participant. For example, the host profile advertises 8 participants but its 16 channel slots only fit 7. Mirror a channel-capacity check here and reconcile the profile defaults/caps.
math(EXPR _rtps_need_stateful "2 * ${RTPS_EFFECTIVE_MAX_NUM_PARTICIPANTS}")
if(RTPS_EFFECTIVE_NUM_STATELESS_WRITERS LESS RTPS_EFFECTIVE_MAX_NUM_PARTICIPANTS
OR RTPS_EFFECTIVE_NUM_STATELESS_READERS LESS RTPS_EFFECTIVE_MAX_NUM_PARTICIPANTS
OR RTPS_EFFECTIVE_NUM_STATEFUL_WRITERS LESS _rtps_need_stateful
OR RTPS_EFFECTIVE_NUM_STATEFUL_READERS LESS _rtps_need_stateful)
…s; document the dedicated-port probe window
Round-24 review fixes (2 inline + 2 previously-missed comments).
1) The participant-budget validation now includes the TRANSPORT CHANNEL pool:
a Domain permanently binds 2 shared multicast channels (SPDP metatraffic +
user multicast) plus 2 unicast channels per participant (builtin + user),
all from the same MAX_NUM_UDP_CONNECTIONS pool that runtime dedicated
endpoint ports draw from - so the old endpoint-pool math could accept a
combination whose createParticipant() still fails on channels before the
advertised participant capacity (the host profile advertised 8
participants but its 16 channels only fit 7).
- MAX_NUM_UDP_CONNECTIONS is now an overridable knob (RTPS_CFG_ wrap in
all three profile headers, espp.cmake allowlist, Kconfig int with the
0 sentinel), minimum 4 (2 multicast + 2 unicast for one participant).
- Both build systems cross-check the effective combination:
MAX_NUM_UDP_CONNECTIONS >= 2 + 2*MAX_NUM_PARTICIPANTS, with a message
spelling out the budget math and the dedicated-port headroom.
- Profile defaults reconciled so the advertised participant budget
actually fits with dedicated-port headroom: host 16 -> 24 (needs 18),
host_large 32 -> 72 (needs 66); embedded stays 10 (needs 4; lwIP fd
budget). Channels are ~32-byte array slots, so the host increases are
negligible.
2) The dedicated-port allocation docs (doc/en/protocols/rtps.rst and
components/rtps/README.md) now describe the WINDOWED probe: each
allocation probes at most 16 consecutive candidates from an advancing
cursor, falls back to the shared user port when the whole window is
occupied, and the next request resumes past the window - the old "linear
probe" wording read as though one request scans the full 100..249 range.
Verified: host cmake probes (17 rejected for host P=8/needs-18, 18 accepted,
3 rejected below minimum-4, channels=10 with participants=4 accepted);
ESP-IDF reconfigure probes (channels=4 with participants=2 rejected with the
budget math, a consistent lowered combo accepted); default builds clean on
all profiles; standalone sweep 3x 0 fail; docker interop matrix 43/43 PASS;
esp32 rtps example builds clean.
|
Also addressed the 2 previously-missed comments from the latest review round (the channel-pool validation gap in The participant-budget cross-check now includes the transport channel pool: a Domain permanently binds 2 shared multicast channels (SPDP metatraffic + user multicast) plus 2 unicast channels per participant (builtin + user), all from the same
Verified: host cmake probes (channels=17 rejected for host's 8 participants / needs 18, 18 accepted, 3 rejected below the minimum of 4, channels=10 with participants=4 accepted); ESP-IDF reconfigure probes (channels=4 with participants=2 rejected with the budget math, a consistent lowered combo accepted); default builds clean on all profiles; standalone sweep ×3 0 fail; docker interop 43/43 PASS; esp32 rtps example builds clean. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 61 out of 62 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
components/rtps/src/rtps_participant.cpp:2057
- Pin this composite add before reading
participant_or creating either endpoint.participant_is mutex-protected but is read here without that mutex whilestop()can reset it; stop can also clear both nested endpoints before this function later commits the client, leaving a stale handle in a stopped/restarted participant. An engine-operation guard makes the pointer read and the full transaction coexist safely with teardown.
…ations Round-25 review fixes (1 inline + 1 previously-missed comment). add_native_service_server_internal() and add_native_service_client() now pin their ENTIRE two-endpoint transaction with the engine-operation guard (same idiom as the composite action builders): the nested add_writer/add_reader release mutex_ between endpoints, so a concurrent stop() could previously tear the engine down after either nested add returned but before the context was committed - the registry push_back then appended a stale server/client context after teardown had cleared the containers. With the operation registered, stop() waits at its phase 1.5 until the builder returns; once stopping, the add is rejected up front. The pin also covers the client's participant_->m_guidPrefix read, which was previously done without the mutex while stop() could reset the pointer: begin_engine_op() validates participant_ under mutex_, and stop() cannot reset it while the operation is registered (the redundant unlocked null check is dropped). Nesting is safe throughout - the guard is counter-based, so the action builders' existing pins compose with these (and with the nested remove_* rollback paths). Verified: service/action loopback tests (native_service, service, typed_rpc, native_action) 3x each; TSan spot-checks on the service paths 0 findings; standalone sweep 3x 0 fail; docker interop matrix 43/43 PASS; esp32 rtps example builds clean.
|
Also addressed the 1 previously-missed comment from the latest review round ( Verified: service/action loopback tests (native_service, service, typed_rpc, native_action) ×3 each; TSan spot-checks on the service paths 0 findings; standalone sweep ×3 (one known port-in-use flake, passes solo ×3); docker interop 43/43 PASS; esp32 rtps example builds clean. |
…able interface discovery in tests Round-26 review fixes (6 inline comments; 2 distinct defects). 1) Domain::readerExists()/writerExists() compared m_attributes.topicName with strncmp bounded by Config::MAX_TYPENAME_LENGTH (4 sites). With the two limits now independently overridable, a type limit larger than the topic limit could read past the fixed topicName array, and a smaller one could alias distinct long topics sharing a prefix. All four comparisons now use MAX_TOPICNAME_LENGTH (the typeName comparisons alongside them were already correct; a tree-wide grep found no other topic/type bound mixups). 2) Four newer PC tests (rtps_writer_churn, rtps_stateless_saturation, rtps_banded_pubsub, rtps_banded_churn) carried their own POSIX-only getifaddrs interface detector (<ifaddrs.h>/<arpa/inet.h>), breaking the MSVC build of the globbed pc test suite. They now use the existing platform-guarded rtps_test::guess_local_ipv4() from rtps_common.hpp (POSIX + winsock), deriving Ip4AddressBytes from its result where needed and preserving the old skip-if-loopback-only semantics. Verified: the four converted tests + rtps_pubsub pass; standalone sweep 3x 0 fail; docker interop matrix 43/43 PASS; esp32 rtps example builds clean. (MSVC compilation is validated by CI - rtps_common.hpp is the suite's established Windows-guarded helper.)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 61 out of 62 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
lib/python_bindings/rtps_bindings.cpp:1
- The Python binding accepts
max_prioritized_endpoint_portsasuint8_t, which can silently wrap/truncate for Python ints outside 0..255 (and negative values). Consider taking this parameter as anintin the binding layer and explicitly range-checking (raising a PythonValueError) before assigning to the config’suint8_t, to avoid surprising runtime behavior (e.g., 256 becoming 0).
Declared extern in Diagnostics.hpp since the component consolidation (#712) but never defined in any TU - dormant today (nothing references it), but any future ODR-use would fail at link time. Defined alongside the other diagnostics counters. Verified: lib+pc rebuild, standalone sweep 0 fail, esp32 rtps example builds clean. The suppressed python-binding comment from the same round is answered on the PR with empirical evidence (pybind11 already range-checks; no code change).
|
Regarding the 1 suppressed comment from the latest round ( Both the constructor argument and the The inline Diagnostics comment is fixed in 6d59849 (missing definition added); verified with lib+pc rebuild, standalone sweep 0 fail, and a clean esp32 example build. |
Description
Phase 2 of priority-aware scheduling (#735 was phase 1): wires
espp::QosBandinto the RTPS stack so a high-priority subscriber/service actually preempts lower-priority traffic, end to end — from the socket to the user callback. Wire format is untouched (goldens byte-identical); the two intentional default-behavior changes are called out below.Per-channel transport bands
rtps::ChannelOptions{band, dscp}on the transport's receive ports;DomainConfig{metatraffic_band, user_traffic_band, enable_dedicated_endpoint_ports, max_prioritized_endpoint_ports}exposed onRtpsParticipant::Config. Metatraffic (SPDP/SEDP) now dispatches atQosBand::Highby default so discovery stays responsive under user-traffic floods. Two intentional default-behavior changes (everything else is opt-in): (1) this metatraffic elevation, and (2) the transport worker-pool queue is now bounded (64 jobs) instead of unbounded — under sustained overload a submission is rejected (real backpressure the reactor and the deferred/guaranteed retry paths recover from losslessly) rather than growing an unbounded heap backlog. This only changes behavior under extreme overload.Per-endpoint priority via dedicated ports (the real enabler)
All user traffic for a participant normally shares ONE unicast port (demux happens in-engine), so socket-level priority alone can't distinguish endpoints. An endpoint configured with a non-default
band(or adscp) now gets:7400 + 250·domain + 100 + n(linear-probed, reuse-disabled bind; standard ports stay below the +100 offset for participant ids 0–44, and the range stays inside the domain's port block), registered on the reactor at the endpoint's band and optionally DSCP-marked (Socket::set_dscp);PID_UNICAST_LOCATOR(already parsed/serialized by the engine — no new wire construct), which FastDDS/ROS 2 honor, so peers send that endpoint's traffic straight to the banded socket; outbound traffic also leaves from it (m_srcPortfollows the locator);max_prioritized_endpoint_ports(default 4, lwIP has ~10 sockets) — on exhaustion, warn + fall back to deferred dispatch below. Ports are released on endpoint deletion.Deferred banded dispatch (shared-port fallback)
Banded endpoints without a dedicated port get their user callbacks re-submitted to the pool at their band instead of running inline on the receive worker: bounded queue (32/endpoint, newest-dropped with warn), single in-flight drain job so per-endpoint ordering is preserved (mirrors the reactor's one-shot pattern). Applies to reader
on_sample, service-server handlers, and service-client reply delivery; default (Normal) endpoints keep today's inline path byte-for-byte.Facade + python
band+ optionaldscpappended to Writer/Reader/Service/Action configs and all typed facades (a service applies them to request+reply; an action inherits to all sub-endpoints — a ROS action is ~8 endpoints, more than the default ration, so most defer). Pythonrtps_bindings.cpp: participant config fields +band/dscpkwargs on all creation methods.Review-driven hardening (rounds 2–18)
The review rounds hardened the concurrency model well beyond the feature itself. Highlights, each with a regression test or sanitizer coverage:
submitGuaranteed): a pool-rejected writerprogress()poke is parked (coalesced per producer, lossless owed counts) and re-armed by a retry timer whose admission is fair round-robin across producers — band priority is applied by the pool after admission, so no band or producer can be starved (rtps_guaranteed_submit,rtps_guaranteed_fairness).stop()waits for; pooled endpoint slots carry generation counters checked by queued jobs and the inbound receive dispatch (DATA/DATA_FRAG/HEARTBEAT/ACKNACK/GAP), so deletion + slot reuse can never dispatch into the wrong endpoint; composite action builds are pinned end-to-end and commit under the lock; rollbacks join goal workers and retain failed engine deletions for retry atstop()(rtps_remove_reader_deadlock,rtps_writer_churn,rtps_service_rollback).checkAndResetHeartbeatsandaddBuiltInEndpointslock-order inversions;executeCallbackssnapshots registrations and invokes unlocked;StatefulReaderdelivers user callbacks under a leaf delivery mutex instead of the proxies mutex (kills the callback→facade→SEDP→proxies cycle); Diagnostics counters and reader/writer init flags are atomic. The suite is TSan-clean with the deadlock detector enabled and ASan-clean.progress()now skips history holes (found byrtps_writer_churn).Consumer-driven fixes (pace-racer-fw #14 / #15)
StatelessWriter::progress()clamps a cursor that fell behind the ring, so a saturated publisher degrades to true KEEP_LAST drop-oldest (measured: 9.9% → 58–71% delivered at 40k/s on a 2-slot static ring); every overwrite is surfaced via a rate-limitedpublish()warning +Diagnostics::Writer::history_overwrite_dropsand per-writerhistoryDrops().HISTORY_SIZE_STATELESSnow differs by profile (embedded 2 / host 8 / host_large 32). Newrtps_stateless_saturationregression.-DRTPS_LIMIT_OVERRIDES="NUM_STATELESS_WRITERS=16;…", validated names + uint8 ranges).SO_RCVTIMEO(DWORD ms, sub-ms clamp) so the reactor's mandatory bounded read works on Windows.Testing
rtps_goldenbyte-identical; wire format never changed across all rounds.rtps_sedp_dedicated_locator,rtps_banded_pubsub,rtps_banded_deferred,rtps_banded_ration,rtps_banded_churn,rtps_deferred_recovery,rtps_guaranteed_submit,rtps_guaranteed_fairness,rtps_remove_reader_deadlock,rtps_writer_churn,rtps_stateless_saturation— each validated to fail against the bug it guards where feasible.ros2_pub→espp_banded_subon a dedicated port and all the regression tests above..github/workflows/host_sanitizers.yml: TSan (races + lock-order/deadlock detection) and ASan over the standalone suite, path-filtered to RTPS/socket/thread_pool/task/timer changes — both green; the leg found and led to fixes for 11 real defects while being brought up.rtpsexample builds every round; python config surface smoke-tested; cppcheck (CI flags) clean.Reviewer notes
shared_ptr<vector>payloads to work around a GCC 15 xtensa-Werror=free-nonheap-objectfalse positive on moved-vector captures.publish()still returnstruewhen KEEP_LAST overflow drops an older unsent sample (the new sample was queued); the drop is surfaced via the rate-limited warning and Diagnostics counters.falsecontinues to mean "this sample was not accepted".HISTORY_SIZE_STATELESSchanged 2→8/32 (capacity-only, wire-neutral); embedded stays 2 and is per-limit overridable.Phase 3 (flood-latency p99 acceptance test) remains as follow-up.
🤖 Generated with Claude Code