From a2801562c1aa3fb5be8acd5ea8d1113a2434940e Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Sat, 3 Oct 2026 17:07:49 -0400 Subject: [PATCH] docs(changelog,protocols): credit the registry-leak fixes to the right layer * Three separate registry-leak defects were fixed at three layers -- :issue:`421`/:pr:`426` for next-layer dispatch, :issue:`425`/:pr:`428` for the option, chunk and block registries, :issue:`555`/:pr:`560` at the schema layer. Four sites credited the wrong pair. * `1.5.0.rst:540` named :issue:`425`/:pr:`428` as the fix "at this layer" in a passage about next-layer dispatch; it now names all three. * `schema/schema.py` twice called #421 and #425 jointly "the protocol-layer ``__proto__`` family"; #425 is the option, chunk and block family. Two test docstrings carried the same conflation, and one called #421/#425/#428 the schema-layer form, which is #555. * `protocol.py`'s "are all that same defect" overstated its clause: the three fixed registries retaining a lookup *miss*, where the sentence is about a memoised *resolved class* going stale under reload. * `corekit/sentinels.py` cited that note, by those three issue numbers, as evidence that reload staleness is tracked. Narrowing the note would have made the two contradict, so the pointer now names what the note argues. * Regenerated `CHANGELOG.md`. tests/project/ 268 passed / 864 subtests; tests/protocols/schema/ test_enum_schema_registry_unit.py and tests/protocols/ test_dispatch_default_resolution_unit.py 16 passed; tests/protocols/misc/test_pcapng_unit.py 93 passed / 1773 subtests. --- CHANGELOG.md | 2 +- docs/source/changelog/1.5.0.rst | 5 +++-- pcapkit/corekit/sentinels.py | 4 ++-- pcapkit/protocols/protocol.py | 4 ++-- pcapkit/protocols/schema/schema.py | 10 ++++++---- tests/protocols/misc/test_pcapng_unit.py | 2 +- .../protocols/schema/test_enum_schema_registry_unit.py | 5 +++-- 7 files changed, 18 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7a9dec86f..5a13eb545 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,7 +41,7 @@ Preceded by `1.5.0a1` (2026-09-15), `1.5.0b1` and `1.5.0b2` (both 2026-09-18) an #### Changed - `Probe`, `CipherSuite` and `IntegritySuite` are `Info` subclasses rather than `typing.NamedTuple`, and no `NamedTuple` remains in the package. They are Mappings now, so `len()` and iteration yield field names rather than values. -- `ModuleDescriptor.klass` reads an already-imported module out of `sys.modules` rather than re-entering `importlib.import_module`, which matters because next layer dispatch resolves a descriptor there on a per-frame path. A registry *hit* holding a `ModuleDescriptor` is resolved once and written back, but a *miss* deliberately is not -- recording a miss in a class-level `collections.defaultdict` is the defect [#425](https://github.com/JarryShaw/PyPCAPKit/issues/425)/[#428](https://github.com/JarryShaw/PyPCAPKit/pull/428) fixed at this layer and [#560](https://github.com/JarryShaw/PyPCAPKit/pull/560) fixed at the schema layer -- so every unrecognised frame resolved the same fallback descriptor again: 48 of the 52 `ModuleDescriptor.klass` resolutions an extraction of `many_interfaces.pcapng` performs, and 4 of the 7 on `ipv4.pcap`. `import_module` keeps real per-call work for a module `sys.modules` already holds, so that resolution now costs ~117 ns rather than ~436 ns and the whole miss path ~526 ns rather than ~883 ns, on CPython 3.14.7. **The scale is worth stating plainly: this is not measurable in** `extract()` **wall clock.** 48 avoided calls is ~17 us against a ~37 ms extraction, two orders of magnitude inside this host's run-to-run variance, and the "~40% of cumulative time" reading that prompted the work was an artifact of `_import_next_layer` being a recursive-descent dispatcher -- its *self* time is 0.58%, while `aenum.extend_enum` is 16.7%. What the change is taken for is its shape rather than its speed: nothing memoises the resolved class, anywhere, so `sys.modules` stays the only module cache in play and its invalidation is the interpreter's. A memo of the class would serve the pre-reload class after an `importlib.reload` forever, and an instance of it fails `isinstance` against the live one. Proposed by `@Ts-Boom` in [#563](https://github.com/JarryShaw/PyPCAPKit/pull/563), whose profiling found the miss path; the implementation differs because that one added a second, never-invalidated cache of resolved classes ([#574](https://github.com/JarryShaw/PyPCAPKit/issues/574)). +- `ModuleDescriptor.klass` reads an already-imported module out of `sys.modules` rather than re-entering `importlib.import_module`, which matters because next layer dispatch resolves a descriptor there on a per-frame path. A registry *hit* holding a `ModuleDescriptor` is resolved once and written back, but a *miss* deliberately is not -- recording a miss in a class-level `collections.defaultdict` is the defect [#421](https://github.com/JarryShaw/PyPCAPKit/issues/421)/[#426](https://github.com/JarryShaw/PyPCAPKit/pull/426) fixed at this layer, [#425](https://github.com/JarryShaw/PyPCAPKit/issues/425)/[#428](https://github.com/JarryShaw/PyPCAPKit/pull/428) fixed for the option, chunk and block registries, and [#555](https://github.com/JarryShaw/PyPCAPKit/issues/555)/[#560](https://github.com/JarryShaw/PyPCAPKit/pull/560) fixed at the schema layer -- so every unrecognised frame resolved the same fallback descriptor again: 48 of the 52 `ModuleDescriptor.klass` resolutions an extraction of `many_interfaces.pcapng` performs, and 4 of the 7 on `ipv4.pcap`. `import_module` keeps real per-call work for a module `sys.modules` already holds, so that resolution now costs ~117 ns rather than ~436 ns and the whole miss path ~526 ns rather than ~883 ns, on CPython 3.14.7. **The scale is worth stating plainly: this is not measurable in** `extract()` **wall clock.** 48 avoided calls is ~17 us against a ~37 ms extraction, two orders of magnitude inside this host's run-to-run variance, and the "~40% of cumulative time" reading that prompted the work was an artifact of `_import_next_layer` being a recursive-descent dispatcher -- its *self* time is 0.58%, while `aenum.extend_enum` is 16.7%. What the change is taken for is its shape rather than its speed: nothing memoises the resolved class, anywhere, so `sys.modules` stays the only module cache in play and its invalidation is the interpreter's. A memo of the class would serve the pre-reload class after an `importlib.reload` forever, and an instance of it fails `isinstance` against the live one. Proposed by `@Ts-Boom` in [#563](https://github.com/JarryShaw/PyPCAPKit/pull/563), whose profiling found the miss path; the implementation differs because that one added a second, never-invalidated cache of resolved classes ([#574](https://github.com/JarryShaw/PyPCAPKit/issues/574)). - `SeekableReader.truncate()` raises instead of returning a size, and the misspelled `writeable()` is now spelled `writable()`. Both are breaks for an external caller and neither breaks anything inside the package. `io.IOBase` documents one gate over two methods -- "If False, write() and truncate() will raise OSError" -- and this reader's `writable()` answers False, so a caller that checked it first, which is exactly what the contract invites, got a surprise either way round: `write` and `writelines` raised, and `truncate` returned its new size. What settled the question is that this is not a pure-Python nicety of `_pyio` that the accelerated path skips -- every ordinary read-only file object in CPython raises here, `open(path, 'rb').truncate()` giving `io.UnsupportedOperation: truncate` off the C `BufferedReader`, and `_pyio.BufferedReader` the same type from `_BufferedIOMixin.truncate`'s `_checkWritable()`. The refusal is `UnsupportedOperation('truncate')`, the same one `write` already raises, and no trade-off was needed between the house exception and the ABC's: pcapkit's `UnsupportedOperation` subclasses `io.UnsupportedOperation`, which subclasses `OSError`, so the in-library exception *is* the one the contract names. The resizing `truncate` used to perform is not deleted, only made private as `_truncate_buffer`. It never touched the underlying stream -- it resizes a private lookback window, which is the counter-argument the issue itself raised -- and it is the only route to the "position sits before the window" state that `seek` and the four buffered read paths must refuse, which [#643](https://github.com/JarryShaw/PyPCAPKit/issues/643) and [#644](https://github.com/JarryShaw/PyPCAPKit/issues/644) landed tests for; deleting it would have taken their mechanism with them. `writeable()`, separately, was never an override of anything: `'writable' in SeekableReader.__dict__` was False and `SeekableReader.writable is io.IOBase.writable` was True, so `io`, `shutil` and any third-party caller read the inherited value and never saw the one defined in this file. Both answered False, which is the coincidence that hid it -- there was no symptom to notice, and editing the misspelled method would silently have had no effect. The value reported is unchanged and was always honest, the reader genuinely being unable to write; only where the method was defined was wrong. Nothing in the package called either method, confirmed by grep, so the risk is entirely to external callers, which is what made this a question asked before it was acted on. The new test asserts the property rather than the behaviour -- that a `SeekableReader` raises the same exception type a read-only `io.BufferedReader` raises for the same call -- with that type captured by running the call rather than named in the test, so it tracks CPython across the 3.10--3.15 matrix instead of restating a belief about it. 31 to 34 tests and 35 to 41 subtests over `tests/corekit/test_io.py`, the module at 100% coverage before and after ([#645](https://github.com/JarryShaw/PyPCAPKit/issues/645)). - **a breaking change to** `@final`, now enforced at runtime on `Info` and `Schema`. `info_final` and `schema_final` both end `return final(cls)`, so every finalised class already carried `typing.final`'s `__final__` marker and nothing read it: the decorator was a promise to the type checker that the interpreter was free to ignore. **`@final` alone on an `Info` or `Schema` subclass -- without `@info_final` or `@schema_final` -- now raises `InfoError` or `SchemaError` at first construction**, the class being marked final but never finalised and so carrying no generated `__init__` and nothing usable. `__init_subclass__` cannot catch that, `final` being applied after class creation, so the check is nested inside the existing one-shot `FinalisedState.NONE` branch and a finalised class pays nothing for it. **Deriving from a finalised class raises too**, from new `Info.__init_subclass__` and `Schema.__init_subclass__` hooks, where before only `EnumSchema` had one. **`SchemaError` is a `ValueError`**, so code catching `TypeError` around a subclass declaration will not see it. `@info_final @final` and `@final @info_final` both finalise silently, order being unable to matter, which is why the re-entry check keys on `__finalised__` rather than `__final__` -- only the former records *the decorator* having run, and decorators apply bottom-up, so a `__final__` test reads a class marked by `final` an instant earlier as already finalised and skips the generation. `@info_final` twice warns and hands back the finalised class unchanged. Every check reads its marker out of the class's own `__dict__`, both markers being ordinary class attributes and so inheriting, and a class that merely descends from a finalised one has not been mismarked by anybody. `pcapkit.utilities.compat` takes `final` from `typing_extensions` below 3.11 rather than below 3.8, because `typing.final` only records `__final__` from 3.11 on and the guards were otherwise a silent no-op on the 3.10 leg. `EnumSchema.__init_subclass__` calls the base hook first so a refused declaration cannot leave `__enum__` pointing at a discarded class, and `pcapkit.protocols.schema.misc.pcapng`'s `Option.__init_subclass__` is reordered to match, having otherwise let a refused `Option` subclass displace a built-in schema first. Additive in tree: of `Info`'s 488 descendants 455 carry `__final__` and none is subclassed, of `Schema`'s 445, 408 do and none is, and no class carries `__final__` without `FinalisedState.FINAL`, so the bare-`@final` guard cannot fire on the library itself ([#778](https://github.com/JarryShaw/PyPCAPKit/issues/778)). diff --git a/docs/source/changelog/1.5.0.rst b/docs/source/changelog/1.5.0.rst index 9f08d73c6..ef8bce567 100644 --- a/docs/source/changelog/1.5.0.rst +++ b/docs/source/changelog/1.5.0.rst @@ -537,8 +537,9 @@ Changed matters because next layer dispatch resolves a descriptor there on a per-frame path. A registry *hit* holding a ``ModuleDescriptor`` is resolved once and written back, but a *miss* deliberately is not -- recording a miss in a - class-level ``collections.defaultdict`` is the defect :issue:`425`/:pr:`428` fixed at this - layer and :pr:`560` fixed at the schema layer -- so every unrecognised frame resolved + class-level ``collections.defaultdict`` is the defect :issue:`421`/:pr:`426` fixed at this + layer, :issue:`425`/:pr:`428` fixed for the option, chunk and block registries, and + :issue:`555`/:pr:`560` fixed at the schema layer -- so every unrecognised frame resolved the same fallback descriptor again: 48 of the 52 ``ModuleDescriptor.klass`` resolutions an extraction of ``many_interfaces.pcapng`` performs, and 4 of the 7 on ``ipv4.pcap``. ``import_module`` keeps real per-call work for a module diff --git a/pcapkit/corekit/sentinels.py b/pcapkit/corekit/sentinels.py index 3606da6da..8d8bd9c21 100644 --- a/pcapkit/corekit/sentinels.py +++ b/pcapkit/corekit/sentinels.py @@ -363,8 +363,8 @@ class NoDefaultType: compares by value rather than identity, and reload staleness is a *tracked* defect class here for other constructs -- see :meth:`pcapkit.protocols.protocol.ProtocolBase._lookup_next_layer`'s own - docstring note citing GitHub issues :issue:`421`, :issue:`425` and - :issue:`555`, and :mod:`tests.protocols.test_dispatch_default_resolution_unit`'s own + docstring note on why it declines to memoise a resolved class, and + :mod:`tests.protocols.test_dispatch_default_resolution_unit`'s own ``test_no_stale_class_survives_a_module_reload``, which reloads a module deliberately to pin the fix for exactly that class of bug elsewhere. A *future* comparison site written the vulnerable way -- a bare ``is diff --git a/pcapkit/protocols/protocol.py b/pcapkit/protocols/protocol.py index 1f9d0766b..af40bb74b 100644 --- a/pcapkit/protocols/protocol.py +++ b/pcapkit/protocols/protocol.py @@ -1743,8 +1743,8 @@ class here instead, whether under ``proto``, in ``registry``'s default factory, or in a cache beside the registry, would retain a class that :func:`importlib.reload` then makes stale; :issue:`421` at this layer, :issue:`425` for the option, chunk and block registries - beside it, and :issue:`555` at the schema layer are all that same - defect. + beside it, and :issue:`555` at the schema layer are the same retention + defect, of a lookup miss rather than of a resolved class. """ protocol = ProtocolBase._lookup_registry(registry, proto) diff --git a/pcapkit/protocols/schema/schema.py b/pcapkit/protocols/schema/schema.py index 481b81a73..834628b34 100644 --- a/pcapkit/protocols/schema/schema.py +++ b/pcapkit/protocols/schema/schema.py @@ -1064,9 +1064,10 @@ class _EnumRegistry(collections.defaultdict): This is the schema-layer instance of the defect :meth:`ProtocolBase.\ _lookup_registry ` - fixed for the protocol-layer ``__proto__`` family in GitHub issues :issue:`421` and - :issue:`425`; see GitHub issue :issue:`555`. The fallback itself is deliberate -- it - is how an unknown option, chunk or block falls back to its + fixed for the protocol-layer ``__proto__`` family in GitHub issue + :issue:`421`, and for the option, chunk and block registries in + :issue:`425`; see GitHub issue :issue:`555`. The fallback itself is + deliberate -- it is how an unknown option, chunk or block falls back to its ``Unknown*``/``Unassigned*`` schema -- so this subclass keeps returning it, it just stops recording it. @@ -1304,7 +1305,8 @@ def register(cls, code: '_ET', schema: 'Type[Self]') -> 'None': looked up, so parsing a single packet carrying an unknown code would have made the next legitimate registration for that code warn about an entry no caller ever asked for -- the defect fixed for this layer - in :issue:`555`, and for the parser-layer ``__proto__`` family in :issue:`421` and + in :issue:`555`, for the parser-layer ``__proto__`` family in + :issue:`421`, and for the option, chunk and block registries in :issue:`425`. That fix is what makes this guard safe to add. :class:`pcapkit.protocols.schema.misc.pcapng.Option` overrides this diff --git a/tests/protocols/misc/test_pcapng_unit.py b/tests/protocols/misc/test_pcapng_unit.py index 801a59585..890a3da65 100644 --- a/tests/protocols/misc/test_pcapng_unit.py +++ b/tests/protocols/misc/test_pcapng_unit.py @@ -4919,7 +4919,7 @@ def test_the_collision_check_does_not_insert_a_default(self) -> None: per-namespace ones are plain :class:`collections.defaultdict`\\ s, so reading ``Option.registry[ns][code]`` to see whether a code is there *inserts* ``UnknownOption`` for it -- the schema-layer form of the - #421/#425/#428 defect. + protocol-layer #421/#425 defect, which #555 fixed for ``EnumSchema``. """ from pcapkit.const.pcapng.option_type import OptionType diff --git a/tests/protocols/schema/test_enum_schema_registry_unit.py b/tests/protocols/schema/test_enum_schema_registry_unit.py index 589e6bcb7..2f450fa9c 100644 --- a/tests/protocols/schema/test_enum_schema_registry_unit.py +++ b/tests/protocols/schema/test_enum_schema_registry_unit.py @@ -4,8 +4,9 @@ :class:`collections.defaultdict`-backed mapping of enumeration codes to schema classes. Reading it with a bare ``registry[code]`` for a code nobody registered used to *insert* that code -- with whatever the default factory produced -- the -same defect fixed at the protocol layer's ``__proto__`` family by GitHub issues -#421 and #425/#428. These tests cover both the auto-created +same defect fixed at the protocol layer's ``__proto__`` family by GitHub issue +#421 and its option, chunk and block registries by #425. These tests cover +both the auto-created :attr:`EnumSchema.__enum__` (the shape used by e.g. :class:`pcapkit.protocols.schema.transport.tcp.Option`) and a manually seeded one declared directly in a subclass's own class body (the shape used by