Skip to content

question(registry): should a built-in engine, reassembly or traceflow overridden by name be restorable through the public registrar? #1363

Closed

Description

@JarryShaw

This is a question and needs the owner's ruling; it may be by design.

Describe the behaviour
The public registrars accept an override of a built-in name, but refuse the built-in class when it is offered back, because they check the public Engine / Reassembly / TraceFlow (pcapkit/foundation/extraction.py:469) while the built-ins derive from the *Base classes. #513 and #1016 recorded this on purpose (_register_internal_* is the door for built-ins), so a restore needs the private method or a saved copy of the registry.

To Reproduce
register_extractor_engine('dpkt', MyEngine) works (with a RegistryWarning). register_extractor_engine('dpkt', DPKT) then raises RegistryError: engine must be an Engine subclass, not <class 'pcapkit.foundation.engines.dpkt.DPKT'>. The same holds for reassembly ipv4 / ipv6 / tcp and traceflow tcp.

Question
Is "an override of a built-in is permanent for the process" the intended contract? If so, register_engine's docstring could say so; if not, the public registrars could accept a class already registered under another name by the library, or register_* could offer an unregister / restore path.

Found by the round-trip audit of pcapkit/foundation. Run /tmp/rtissues/foundation-repro.py from the repo root (it prints pcapkit.__file__ first).

Found by the foundation round-trip audit (#1202). The repro was re-checked by an independent verifier, and I re-ran it on main at c9676c563.

Activity

  1. added
    fixPull requests that fix a defect (fix: subject prefix)
    pendingAccepted and unstarted - nobody is on it and nothing blocks it
    on Oct 7, 2026
  2. JarryShaw commented on Oct 7, 2026

    @JarryShaw
    OwnerAuthor

    Decision needed. #513 and #1016 made the public registrars refuse built-ins on purpose: extraction.py:458-469 records it in a comment and the docstring. The consequence is that once register_extractor_engine('dpkt', MyEngine) overrides a built-in, putting DPKT back raises RegistryError: engine must be an Engine subclass. The same holds for reassembly ipv4/ipv6/tcp and traceflow tcp.

    Options:

    • (1) Let the registrar accept the original built-in for a name it shipped with, so an override can be undone. Everything else stays refused.
    • (2) Add an explicit unregister_*/reset_* that restores the shipped default.
    • (3) Leave it as designed and document that an override is permanent for the process.

    I recommend (1). It is the smallest change, keeps #1016's guarantee for every foreign class, and makes the round trip override→restore exact.

  3. added
    needs: decisionWaiting on the maintainer to decide — not blocked by other work
    questionIssues asking how something works rather than reporting a defect
    and removed
    fixPull requests that fix a defect (fix: subject prefix)
    pendingAccepted and unstarted - nobody is on it and nothing blocks it
    on Oct 7, 2026
  4. JarryShaw commented on Oct 7, 2026

    @JarryShaw
    OwnerAuthor

    okay take (1) then, and this will apply to all registrars.

  5. JarryShaw commented on Oct 7, 2026

    @JarryShaw
    OwnerAuthor

    Ruling recorded: option (1), applied to every registrar. A registrar accepts the shipped original for a name it shipped with, so an override can be undone; every other foreign class is still refused as #1016 requires. The rule covers the extraction engine, reassembly, traceflow and dumper registrars, and also the protocol and option registrars.

    Sequencing:

  6. added
    wipWork in flight - a covering PR is open or an agent is actively on it
    and removed
    needs: decisionWaiting on the maintainer to decide — not blocked by other work
    on Oct 7, 2026
  7. JarryShaw commented on Oct 8, 2026

    @JarryShaw
    OwnerAuthor

    The foundation half of this ruling has landed in #1376, which has merged. The protocol-registrar half rides on #1362. Blocked until #1362 is fixed, and #1362 itself waits on #1390.

  8. added
    blockedDeferred pending another issue or decision; see the last comment for what unblocks it
    and removed
    wipWork in flight - a covering PR is open or an agent is actively on it
    on Oct 8, 2026
  9. JarryShaw commented on Oct 8, 2026

    @JarryShaw
    OwnerAuthor

    The protocol-registrar half has landed in #1400, which merged: restoring a shipped entry now warns once and is accepted. What remains is the class-storing register in protocols/protocol.py, which is #1364's code. Blocked until #1364 is fixed.

  10. JarryShaw commented on Oct 8, 2026

    @JarryShaw
    OwnerAuthor

    Unblocked: #1364 is fixed by #1415 (merged at 199bfcef8). The class-storing register in protocols/protocol.py now keeps a re-registered stored descriptor, so the protocol-registrar half of this question is settled.

    What remains is the question itself, for the owner. It concerns the public register_extractor_engine / _reassembly / _traceflow registrars (foundation/extraction.py). Should offering a built-in class back after an override:

    1. stay refused, making an override permanent for the process (and the docstring would say so), or
    2. be accepted with a RegistryWarning, the way fix(protocols): parser registrars warn only when the entry really changes (#1362) #1400 now treats protocol registrars?

    Moved to needs: decision.

  11. added
    needs: decisionWaiting on the maintainer to decide — not blocked by other work
    and removed
    blockedDeferred pending another issue or decision; see the last comment for what unblocks it
    on Oct 8, 2026
  12. JarryShaw commented on Oct 8, 2026

    @JarryShaw
    OwnerAuthor

    Your recommendations?

  13. JarryShaw commented on Oct 8, 2026

    @JarryShaw
    OwnerAuthor

    Correction first: my comment above was stale. Option 2 is already on main. #1376 (merged 2026-10-08) made the extractor registrars accept the built-in a name shipped with, #1400 did the same for the 14 parser registrars, and #1415 closed the last gap (#1364). Re-measured on 949b0a900:

    • register_extractor_engine('dpkt', MyEngine), then ('dpkt', DPKT): accepted, and the registry holds the shipped ModuleDescriptor again.
    • Same for reassembly ipv4 and traceflow tcp.
    • register_extractor_engine('foo', DPKT) still raises RegistryError. A built-in is only accepted back under the name it shipped with.
    • Tests: tests/foundation/registry/test_restore_builtin_unit.py.

    Recommendation: close this as answered. Keep both behaviours:

    One optional nit: restoring warns already registered, overwriting, the same as an override. I'd keep it, because it does replace the override. If you'd rather it say restoring shipped entry, that is a one-line message change.

  14. JarryShaw commented on Oct 8, 2026

    @JarryShaw
    OwnerAuthor

    all good. close it then.

  15. removed
    needs: decisionWaiting on the maintainer to decide — not blocked by other work
    on Oct 8, 2026
  16. JarryShaw commented on Oct 8, 2026

    @JarryShaw
    OwnerAuthor

    Closing as answered, per the owner's ruling: the restore behaviour is already on main and both halves stay as they are. A built-in is accepted back under the name it shipped with (#1376, #1400, #1415); under any other name the public door still refuses it (#513, #1016). The already registered, overwriting warning on a restore is kept as-is.

  17. added this to the PyPCAPKit v1.5 milestone on Oct 26, 2026
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

    questionIssues asking how something works rather than reporting a defect

    Projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions