Skip to content

ISO-TP: DatagramReceived runs one Task.Run per PDU — no ordering, can fire after Dispose, and the README misdescribes it #206

Description

@dborgards

Severity

Low.

Location

  • src/CanKit.Pro.IsoTp/IsoTpChannel.cs:1483-1507: EmitPdu, with _ = Task.Run(...) at 1496.
  • IsoTpChannel.cs:1079 ff: HandleReceivedFrame has no _disposed check.
  • src/CanKit.Pro.IsoTp/IIsoTpChannel.cs:173-181: the event doc says only "Raised (on a thread-pool thread)".
  • src/CanKit.Pro.IsoTp/README.md:52-53: calls the three receive surfaces "three surfaces onto the same bounded, drop-oldest PDU inbox".
  • For comparison: src/CanKit.Pro.J1939Tp/J1939TpChannel.cs:1202-1224 uses the same Task.Run pattern, but IJ1939TpChannel.cs:84-89 documents it ("may therefore reach their handlers concurrently, and not necessarily in the order they completed; ReceiveAsync keeps that order"), and HandleIncoming drops frames after dispose (:475, "Codex on test(uds): drive functional response windows from an injected clock #183"). CANopen uses a serialized, bounded event pump (CanOpenNode.cs:60-74, 210, 661-665).

Problem

  • Each completed PDU gets its own thread-pool work item, so DatagramReceived handlers can run concurrently and out of order. The inbox is FIFO; the event is not.
  • Handlers can run after Dispose() returns. A Task.Run queued before dispose runs anyway. In ISO-TP the window is wider than in J1939-TP: frames still in the mailbox are drained by the owned actor's final drain during Dispose, reach EmitPdu, and the handler is still scheduled even though the inbox write fails.
  • The README is wrong about the event: it does not go through the inbox, so it is neither bounded nor drop-oldest.

Proposed fix

Minimum, to reach parity with J1939-TP:

  • Document in IIsoTpChannel.DatagramReceived that delivery is concurrent and unordered and that ReceiveAsync keeps order.
  • Stop emitting after dispose, with a _disposed check in HandleReceivedFrame/EmitPdu.
  • Fix the README sentence.

Larger option: adopt the CANopen event-pump pattern (serialized, bounded, completed on dispose) for both ISO-TP and J1939-TP.

Decision question

Document-and-guard only, or move both transport channels to a serialized event pump? The latter changes observable handler threading.

Review

From the CanKit.Pro deep review of main @ 2e7beb6 (2026-09-28), finding 8. Re-verified against main @ ff0cbe7; the J1939-TP half is already documented and guarded, so this issue targets ISO-TP.

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

    area: isotpCanKit.Pro.IsoTp — ISO 15765-2 codec and channelseverity/lowLow-severity finding from reviewtype: bugSomething behaves differently than documented

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions