Skip to content

traits: PayloadWireFormat's visitor defaults silently disable SD registry population for downstream impls #145

Description

@JustinKovacich

Summary

An implementor of PayloadWireFormat that overrides offered_endpoints() / service_instances() — the two methods that look like the extension points — compiles clean, passes tests that call those methods directly, and silently populates nothing at runtime.

We hit exactly this, and it cost a scoped investigation to find. It is not detectable by the compiler, by clippy, or by any test that exercises the accessors.

Mechanism

In src/traits.rs:

  • for_each_offered_endpoint (:128) and for_each_service_instance (:143) are trait methods with empty default bodies — they visit nothing.
  • offered_endpoints() (:154) and service_instances() (:165) are provided wrappers that delegate to those visitors.
  • The client run loop consults only the visitors (src/client/inner.rs).

So the call graph the runtime uses and the call graph an implementor naturally reaches for are disjoint. Override the accessors and the run loop never sees your data; SD registry auto-population and reboot detection go quiet, with no error, no log line, and no failing test.

The trap is sharpened by the fact that the accessors are the more discoverable API — they have the obvious names and the obvious return types, and a test asserting payload.service_instances() == [...] passes while the run loop gets nothing.

Proposal

Make the wrong path stop existing, rather than documenting it:

  1. Make the two visitors required methods — drop the empty default bodies. A missing implementation becomes a compile error instead of silent runtime death. Empty defaults on a method the runtime depends on are the root cause here; there is no sensible "visit nothing" default for a type whose job is to yield SD entries.

  2. Move offered_endpoints() / service_instances() to a blanket-implemented extension trait (PayloadWireFormatExt), the Iterator / StreamExt pattern. They are pure derivations of the visitors, so no implementor has a legitimate reason to override them — and making them non-overridable removes the failure mode entirely rather than relying on everyone reading the doc comment.

Breaking?

Yes — every implementor must supply the visitors. That is the point of the change: the break is loud and immediate, whereas the current shape fails quietly and at runtime.

If a staged path is preferred, #[deprecated] on the accessors as overridable methods would at least surface a warning before the required-method change lands.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workingsimple_someipIssue related to the simple_someip crate

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions