From b6c314fa5a7e181c146ddf5fca3643917b5f6a40 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Mon, 21 Sep 2026 16:35:24 -0400 Subject: [PATCH] perf(corekit): resolve a ModuleDescriptor through sys.modules, not import_module (#574) (#586) Proposed by @Ts-Boom in #563. A next layer code nobody registered resolves to the fallback ModuleDescriptor the registry's default factory produces, and _lookup_next_layer deliberately does not write that back -- recording a miss in a class-level defaultdict is the defect #425/#428 fixed at this layer and #560 fixed at the schema layer. So every unrecognised frame resolved the same descriptor again: 48 of the 52 ModuleDescriptor.klass resolutions an extraction of many_interfaces.pcapng performs, each re-entering importlib.import_module for a module sys.modules already held. - ModuleDescriptor.klass now reads sys.modules first, and enters import_module only when the module is not loaded yet -- or when the loaded module does not have the attribute, which is a body still executing (a circular import, or another thread part way through importing it) and is what import_module's per-module lock exists to wait for. Measured on CPython 3.14.7: klass 436 -> 117 ns, the whole miss path 883 -> 526 ns, and import_module calls during extract() 48 -> 0 on many_interfaces.pcapng and 4 -> 0 on ipv4.pcap. The hit path is untouched (124 -> 127 ns, inside noise), since it is already memoised by the registry write-back. - Nothing memoises the resolved class, which is the deliberate part. The class is re-read with getattr on every access, so sys.modules stays the only module cache in play and its invalidation is the interpreter's: importlib.reload rebinds the class inside the same module object, and sys.modules.pop() replaces the object outright. #563's class-level _MODULE_CACHE followed neither, and an instance built from the class it kept fails isinstance against the live one. - Records in _lookup_next_layer's docstring why no memo lives there, so the next reader does not add one. Honest about the scale: 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 single-digit-millisecond run-to-run variance, and repeated A/B pairs flipped sign. #563's "~40% of cumulative time" does not reproduce: _import_next_layer's self time is 0.58% against its 90.2% cumulative, because it is a recursive-descent dispatcher that has the whole nested parse beneath it. aenum.extend_enum at 16.7% self time is where the real time is (#575). Also found, not fixed here: the function-level `from ... import NoPayload` statements on the protocol layer, one of which is _import_next_layer's length == 0 fast path, run 890 times on http.pcap at ~162 ns against ~58 ns for the sys.modules equivalent -- ~92 us on a ~540 ms extraction, so left alone rather than paid for with a second resolution path. Adds tests/protocols/test_dispatch_default_resolution_unit.py, and four cases to tests/corekit/test_module.py. The two that assert the saving fail on the pre-fix code (5 import_module calls for 5 lookups); the three guards fail against the designs this one rejects -- the reload guard against #563's cache, and all three against writing the resolved fallback back under the missed code. Closes #574. --- CHANGELOG.md | 1 + docs/source/changelog/1.5.0.rst | 25 +++ pcapkit/corekit/module.py | 47 ++++- pcapkit/protocols/protocol.py | 10 + tests/corekit/test_module.py | 115 ++++++++++ .../test_dispatch_default_resolution_unit.py | 198 ++++++++++++++++++ 6 files changed, 393 insertions(+), 3 deletions(-) create mode 100644 tests/protocols/test_dispatch_default_resolution_unit.py diff --git a/CHANGELOG.md b/CHANGELOG.md index b944d395f2..e528b210d3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,6 +43,7 @@ The largest release since 1.0, and the first recorded here as it happened rather - **Fixed** -- which exception a malformed TCP SACK option raised depended on unrelated process state: a clean interpreter raised `ProtocolError` as documented, but a process that had already popped `pcapkit.corekit.fields.misc` from `sys.modules` -- which the `#439` ABC-cache regression tests do in every case's `setUp`/`tearDown` -- raised `FieldValueError` instead, from a different layer entirely, before the documented check was even reached (#525). The cause was `ListField.unpack` resolving `SchemaField` through a function-local import re-run on every call; a module popped and reimported mid-process comes back as a second, distinct class, so `isinstance` against it silently misclassified the field and billed each item by its declared length instead of by what it actually consumed. **Any caller relying on the previously-observed** `FieldValueError` **for this case now gets** `ProtocolError` **instead, deterministically**, matching the method's own docstring. Fixed by importing at module level instead. - **Fixed** -- two dropped-keyword/wrong-cast defects flagged in review during this release and never filed until now: HIP's `_make_param_encrypted` passed `cipher=` to a schema with no such field, so the value was silently dropped and an AES-cipher `ENCRYPTED` parameter built through `make` packed without its IV; and IPv6-Route's `RPL.post_process`, which runs on every `Schema.pack` and not only after a parse, assumed `self.addresses` was still the concatenated `bytes` a parse leaves it as, and raised slicing the `list[bytes]` a `make`-built multi-address header actually holds there (#556). - **Fixed** -- two more defects #541 exposed rather than caused, both since it let construction reach code that had never run before. `MPTCP.subtype` was still `typing.TYPE_CHECKING`-only, an annotation rather than a field, so `TCP(options=[(Enum_Option.Multipath_TCP, ...)])` raised `AttributeError: ... has no attribute 'subtype'` for every subtype but `MP_JOIN`: the convenience constructor builds a schema in memory and reads it straight back through `_read_mptcp_*` with no byte round trip, so `_MPTCP.post_process` -- the only code that ever set `subtype` -- never ran. Fixed on the construction path (`TCP._make_mode_mp`) rather than by adding a third real field the way `kind`/`length` got in #541: unlike those two, `subtype` is already packed as 4 bits of each subtype's own `test` bitfield, and a second, independent field for the same bits would either double-encode them or need a "derive, don't pack" field kind this library does not have (#566). Separately, `_make_mptcp_capable` wrote `length=20 if rkey is None else 32` where [RFC 8684](https://datatracker.ietf.org/doc/html/rfc8684) section 3.1 gives 12 and 20, and `MPTCPCapable.rkey`'s own condition (`pkt['length'] != 32`) dropped the receiver's key for exactly the length the maker used to mean "key present" -- so a spec-correct, key-absent MP_CAPABLE could not be built at all, and a key-present one silently lost its key on the wire. Both, and the matching guard in `_read_mptcp_capable`, now agree on 12/20. **This changes MP_CAPABLE's packed output**: a 20-octet, key-present option built or parsed under the old code becomes 12 octets with no key, or 20 octets with the key actually present, depending on which the caller meant (#567). +- **Changed** -- `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/#428 fixed at this layer and #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, whose profiling found the miss path; the implementation differs because that one added a second, never-invalidated cache of resolved classes (#574). Preceded by `1.5.0a1` (2026-09-15), `1.5.0b1` and `1.5.0b2` (both 2026-09-18) and `1.5.0b3` (2026-09-19), all published as prereleases and so resolved only by `pip install --pre`. `1.5.0b1` half-shipped: the tag, the GitHub release and the Conda deployments landed, but PyPI rejected the wheel because `twine check` found a Sphinx-only `:mod:` role in `README.rst`, which `pyproject.toml` declares as the dynamic long description. `1.5.0b2` is what reshipped it -- the release workflow is version-driven, so an existing version cannot republish -- and `1.5.0b3` followed the CI change that stops a TestPyPI outage from costing a release its wheels (#497, #498). diff --git a/docs/source/changelog/1.5.0.rst b/docs/source/changelog/1.5.0.rst index 5a1f45f788..deb8d78285 100644 --- a/docs/source/changelog/1.5.0.rst +++ b/docs/source/changelog/1.5.0.rst @@ -420,6 +420,31 @@ pull requests between #326 and #509. packed output**: a 20-octet, key-present option built or parsed under the old code becomes 12 octets with no key, or 20 octets with the key actually present, depending on which the caller meant (#567). +* **Changed** -- ``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/#428 fixed at this + layer and #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, whose profiling found the miss + path; the implementation differs because that one added a second, + never-invalidated cache of resolved classes (#574). Preceded by ``1.5.0a1`` (2026-09-15), ``1.5.0b1`` and ``1.5.0b2`` (both 2026-09-18) and ``1.5.0b3`` (2026-09-19), all published as prereleases and so diff --git a/pcapkit/corekit/module.py b/pcapkit/corekit/module.py index b41c72e81b..f63ab3464a 100644 --- a/pcapkit/corekit/module.py +++ b/pcapkit/corekit/module.py @@ -11,6 +11,7 @@ """ import collections import importlib +import sys from typing import TYPE_CHECKING, Generic, TypeVar __all__ = ['ModuleDescriptor'] @@ -34,6 +35,46 @@ class can be imported by ``from module import name``.""" @property def klass(self) -> 'Type[_T]': - """Import class from module.""" - module = importlib.import_module(self.module) - return getattr(module, self.name) + """Import class from module. + + Important: + The module is read from :data:`sys.modules` first, and + :func:`importlib.import_module` is entered only when it is not + loaded yet. That matters because this property is on a *per-frame* + dispatch path: a next layer code nobody registered falls back to a + :class:`ModuleDescriptor` for + :class:`~pcapkit.protocols.misc.raw.Raw` which + :meth:`ProtocolBase._lookup_next_layer + ` + deliberately does not write back, so every unrecognised frame + resolves the same descriptor again -- 48 of the 52 resolutions an + extraction of :file:`many_interfaces.pcapng` performs. + :func:`~importlib.import_module` keeps real per-call work for an + already-imported module (locks, :class:`~importlib.machinery.ModuleSpec` + checks, the ``fromlist`` walk), so each repeat cost ~436 ns where + this property now costs ~117 ns. + + The class is still read off the module with :func:`getattr` on + every access, and nothing is memoised here. That is the point: + :data:`sys.modules` *is* the module cache, and it is the only one + whose invalidation the interpreter maintains -- + :func:`importlib.reload` rebinds the class in place and + ``sys.modules.pop()`` drops the entry, both of which this sees + immediately. Holding the resolved class instead would serve the + pre-reload class forever, and an instance of it fails + :func:`isinstance` against the live one. + + """ + module = sys.modules.get(self.module) + if module is not None: + try: + return getattr(module, self.name) + except AttributeError: + # ``sys.modules`` also holds modules whose body is still + # executing -- a circular import, or another thread part way + # through importing this one. ``import_module`` waits on the + # per-module import lock, so defer to it rather than reporting + # the attribute missing; a name that really is absent raises + # from the ``getattr`` below instead, with the same message. + pass + return getattr(importlib.import_module(self.module), self.name) diff --git a/pcapkit/protocols/protocol.py b/pcapkit/protocols/protocol.py index 916d7f2292..0432389a9a 100644 --- a/pcapkit/protocols/protocol.py +++ b/pcapkit/protocols/protocol.py @@ -1370,6 +1370,16 @@ def _lookup_next_layer(registry: 'DefaultDict[int, ModuleDescriptor[ProtocolBase :meth:`self._lookup_registry ` exists to avoid. + So a miss resolves its fallback descriptor again on every frame, and + what keeps that affordable is :attr:`ModuleDescriptor.klass + ` reading + :data:`sys.modules` instead of re-entering + :func:`importlib.import_module` -- see #574. Memoising the resolved + 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; #425 and #428 + at this layer and #560 at the schema layer are all that same defect. + """ protocol = ProtocolBase._lookup_registry(registry, proto) if isinstance(protocol, ModuleDescriptor): diff --git a/tests/corekit/test_module.py b/tests/corekit/test_module.py index dbba23e677..0d888f1ec4 100644 --- a/tests/corekit/test_module.py +++ b/tests/corekit/test_module.py @@ -1,5 +1,6 @@ from __future__ import annotations +import sys import types import unittest from unittest import mock @@ -12,6 +13,19 @@ def setUp(self) -> None: purge_modules(['pcapkit']) self.module = load_module('pcapkit.corekit.module', 'pcapkit/corekit/module.py') + def _register(self, name: str, module: types.ModuleType) -> None: + """Put ``module`` in :data:`sys.modules` for the duration of a test.""" + original = sys.modules.get(name) + sys.modules[name] = module + + def restore() -> None: + if original is None: + sys.modules.pop(name, None) + else: + sys.modules[name] = original + + self.addCleanup(restore) + def test_klass_imports_target_attribute(self) -> None: target_module = types.SimpleNamespace(Target=dict) with mock.patch('importlib.import_module', return_value=target_module) as importer: @@ -20,6 +34,107 @@ def test_klass_imports_target_attribute(self) -> None: importer.assert_called_once_with('demo.module') + def test_klass_reads_an_already_loaded_module_out_of_sys_modules(self) -> None: + """An already-imported module must not go through the import machinery. + + :attr:`~pcapkit.corekit.module.ModuleDescriptor.klass` sits on a + per-frame dispatch path -- see GitHub issue #574 -- and + :func:`importlib.import_module` retains real per-call work even when + :data:`sys.modules` already holds the module, so re-entering it for + every unrecognised frame costs ~436 ns against the ~117 ns this + property takes reading :data:`sys.modules` directly. + + """ + target_module = types.ModuleType('demo.loaded') + target_module.Target = dict # type: ignore[attr-defined] + self._register('demo.loaded', target_module) + + descriptor = self.module.ModuleDescriptor('demo.loaded', 'Target') + with mock.patch('importlib.import_module', + side_effect=RuntimeError('import machinery re-entered')) as importer: + for _ in range(3): + self.assertIs(descriptor.klass, dict) + + importer.assert_not_called() + + def test_klass_follows_a_rebound_class_rather_than_keeping_the_first_one(self) -> None: + """Nothing may be memoised, because a reload rebinds the class. + + The two reload idioms differ and both have to be followed. + :func:`importlib.reload` re-executes the module body into the *same* + module object, so the class it defines is a new object while + ``sys.modules[name]`` is unchanged -- which is why a memo validated + against the module object's identity would still serve the old class. + Popping :data:`sys.modules` and importing again replaces the module + object as well. Both are modelled here directly; GitHub pull request + #563's class-level cache followed neither, and instances built from the + class it kept fail :func:`isinstance` against the live one. + + """ + class Old: + pass + + class New: + pass + + target_module = types.ModuleType('demo.reloadable') + target_module.Target = Old # type: ignore[attr-defined] + self._register('demo.reloadable', target_module) + + descriptor = self.module.ModuleDescriptor('demo.reloadable', 'Target') + self.assertIs(descriptor.klass, Old) + + # what importlib.reload does: same module object, new class object + target_module.Target = New # type: ignore[attr-defined] + self.assertIs(descriptor.klass, New) + + # what popping sys.modules and importing again does: new module object + replacement = types.ModuleType('demo.reloadable') + replacement.Target = Old # type: ignore[attr-defined] + sys.modules['demo.reloadable'] = replacement + self.assertIs(descriptor.klass, Old) + + def test_klass_defers_to_import_module_for_a_partially_initialised_module(self) -> None: + """A module whose body is still executing is not a resolution failure. + + :data:`sys.modules` holds a module from the moment its body *starts* + executing, so a circular import -- or another thread part way through + importing the same module -- can see it without the class defined yet. + :func:`importlib.import_module` waits on the per-module import lock, + which is the behaviour the fast path has to fall back to rather than + reporting the attribute missing. + + """ + partial = types.ModuleType('demo.partial') # body still running: no Target + self._register('demo.partial', partial) + + complete = types.ModuleType('demo.partial') + complete.Target = dict # type: ignore[attr-defined] + + descriptor = self.module.ModuleDescriptor('demo.partial', 'Target') + with mock.patch('importlib.import_module', return_value=complete) as importer: + self.assertIs(descriptor.klass, dict) + + importer.assert_called_once_with('demo.partial') + + def test_klass_still_raises_attributeerror_for_a_name_that_is_not_there(self) -> None: + """A genuinely absent name must still fail, and say so. + + The fallback above swallows one :exc:`AttributeError` to retry through + :func:`importlib.import_module`. A descriptor naming a class that does + not exist has to come back out of that retry as the same + :exc:`AttributeError` it always raised, rather than as :data:`None` or + as a second, more confusing error. + + """ + target_module = types.ModuleType('demo.incomplete') + self._register('demo.incomplete', target_module) + + descriptor = self.module.ModuleDescriptor('demo.incomplete', 'Missing') + with mock.patch('importlib.import_module', return_value=target_module): + with self.assertRaisesRegex(AttributeError, 'Missing'): + descriptor.klass # pylint: disable=pointless-statement + if __name__ == '__main__': unittest.main() diff --git a/tests/protocols/test_dispatch_default_resolution_unit.py b/tests/protocols/test_dispatch_default_resolution_unit.py new file mode 100644 index 0000000000..d09fccd3aa --- /dev/null +++ b/tests/protocols/test_dispatch_default_resolution_unit.py @@ -0,0 +1,198 @@ +"""Regression tests for GitHub issue #574. + +A next layer code nobody registered resolves to the fallback +:class:`~pcapkit.corekit.module.ModuleDescriptor` the registry's default factory +produces -- normally :class:`~pcapkit.protocols.misc.raw.Raw`. That resolution is +deliberately **not** written back into the registry, because the registry is a +class-level :class:`collections.defaultdict` and recording a miss in it is the +defect GitHub issues #425/#428 fixed at this layer and #560 fixed at the schema +layer. The cost of not writing it back is that every unrecognised frame resolves +the same descriptor again: 48 of the 52 resolutions an extraction of +:file:`many_interfaces.pcapng` performs. + +Proposed by @Ts-Boom in GitHub pull request #563, which paid that cost with a +class-level cache of resolved classes and no invalidation -- so a +:func:`importlib.reload` left it serving the pre-reload class forever. These +tests pin the shape taken instead: the repeats are cheap because +:attr:`ModuleDescriptor.klass ` +reads :data:`sys.modules` rather than re-entering +:func:`importlib.import_module`, and nothing anywhere retains the class. + +Three properties, and the split is deliberate -- the first says the cost is +gone, the other two say what it was not allowed to cost: + +:meth:`DefaultDescriptorResolutionTests.test_repeated_miss_does_not_re_enter_the_import_machinery` + The saving itself, as a call count rather than as a timing. Fails on the + unfixed tree with one :func:`~importlib.import_module` call per lookup. + +:meth:`DefaultDescriptorResolutionTests.test_a_missed_code_can_still_be_registered_without_a_warning` + The half that any write-back-on-miss implementation breaks: a code that has + only ever *missed* is still unregistered, so a later genuine + :meth:`~pcapkit.protocols.protocol.ProtocolBase.register` for it must not + warn that it is already registered. That warning is the #426/#428 symptom. + +:meth:`DefaultDescriptorResolutionTests.test_no_stale_class_survives_a_module_reload` + The half that any class-caching implementation breaks, #563's included. + Both reload idioms are covered, because they differ: + :func:`importlib.reload` rebinds the class inside the *same* module object, + so validating a memo against the object's identity would not notice it, + while popping :data:`sys.modules` and importing again mints a new one. +""" +from __future__ import annotations + +import collections +import importlib +import importlib.util +import sys +import unittest +import warnings +from unittest import mock + +from tests._support import purge_modules + +RUNTIME_DEPS = ('tbtrim', 'aenum', 'chardet', 'dictdumper') +HAS_RUNTIME = all(importlib.util.find_spec(name) is not None for name in RUNTIME_DEPS) + +#: Module and class name of the fallback every next layer registry declares. +RAW_MODULE = 'pcapkit.protocols.misc.raw' + + +@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') +class DefaultDescriptorResolutionTests(unittest.TestCase): + def setUp(self) -> None: + purge_modules(['pcapkit']) + + def tearDown(self) -> None: + # ``test_no_stale_class_survives_a_module_reload`` leaves a reloaded + # module behind, whose ``Raw`` is a different object from the one the + # rest of the imported tree holds. Purging here keeps that confined to + # the test that did it, rather than handing it to whatever imports + # :mod:`pcapkit` next. + purge_modules(['pcapkit']) + + def _dummy_protocol(self) -> type: + """Build a protocol class with a registry of its own. + + Returns: + A :class:`~pcapkit.protocols.protocol.ProtocolBase` subclass whose + ``__proto__`` is a fresh, empty :class:`collections.defaultdict` + declaring the same :class:`~pcapkit.corekit.module.ModuleDescriptor` + fallback the real registries do. Fresh, so that a miss recorded by + accident is visible as a key rather than hidden among the ~90 + registrations a real table carries. + + """ + from pcapkit.corekit.module import ModuleDescriptor + from pcapkit.protocols.protocol import ProtocolBase + + class Dummy(ProtocolBase): + __proto__ = collections.defaultdict( + lambda: ModuleDescriptor(RAW_MODULE, 'Raw'), + ) + + return Dummy + + def test_repeated_miss_does_not_re_enter_the_import_machinery(self) -> None: + """A miss must resolve its fallback without importing it again. + + :func:`importlib.import_module` keeps real per-call work for a module + that is already imported -- the import lock, the + :class:`~importlib.machinery.ModuleSpec` check, the ``fromlist`` walk -- + so a registry miss that re-enters it per frame pays ~436 ns per + resolution where the :data:`sys.modules` path costs ~117 ns. + + Asserted as a call count rather than as a timing on purpose: the + absolute saving is tens of microseconds against a multi-hundred + millisecond extraction, which no wall clock on this host can resolve, + while the count is exact and reproducible. + + """ + Dummy = self._dummy_protocol() + from pcapkit.protocols.misc.raw import Raw + + imported = [] # type: list[str] + real_import = importlib.import_module + + def counting_import(name, package=None): # type: ignore[no-untyped-def] + imported.append(name) + return real_import(name, package) + + with mock.patch('importlib.import_module', counting_import): + for _ in range(5): + self.assertIs(Dummy._lookup_next_layer(Dummy.__proto__, 99), Raw) + + self.assertEqual([name for name in imported if name == RAW_MODULE], []) + + # and the resolution is still not recorded, which is what it is paying + # the repeats for in the first place + self.assertNotIn(99, Dummy.__proto__) + self.assertEqual(set(Dummy.__proto__), set()) + + def test_a_missed_code_can_still_be_registered_without_a_warning(self) -> None: + """Resolving the fallback must not register the code. + + :meth:`~pcapkit.protocols.protocol.ProtocolBase.register` warns + ``already registered, overwriting`` for any code already in + ``__proto__``. So an implementation that memoised the fallback under the + missed code -- the obvious way to stop the repeats -- would make a + later, entirely legitimate registration for that code warn about an + entry that no caller ever asked for. That is the #426/#428 symptom, and + it is why the repeats are made cheap rather than removed. + + """ + Dummy = self._dummy_protocol() + from pcapkit.protocols.misc.null import NoPayload + from pcapkit.protocols.misc.raw import Raw + + for _ in range(5): + self.assertIs(Dummy._lookup_next_layer(Dummy.__proto__, 99), Raw) + self.assertNotIn(99, Dummy.__proto__) + + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter('always') + Dummy.register(99, NoPayload) + self.assertEqual([str(record.message) for record in caught], []) + + # the registration took, and dispatch now reaches it rather than the + # fallback -- a memoised fallback would have been overwritten silently + # here, but only after the warning above + self.assertIs(Dummy.__proto__[99], NoPayload) + self.assertIs(Dummy._lookup_next_layer(Dummy.__proto__, 99), NoPayload) + + def test_no_stale_class_survives_a_module_reload(self) -> None: + """Dispatch must reach the live class, not the one resolved first. + + This is the property GitHub pull request #563's ``_MODULE_CACHE`` gave + up: warm it, reload the module, and it serves the pre-reload class for + the life of the process, so an instance built from it fails + :func:`isinstance` against the live one. + + Both reload idioms are exercised because a memo can pass one and fail + the other. :func:`importlib.reload` re-executes the body into the *same* + module object, so the new class is a new object while + ``sys.modules[name]`` is unchanged -- a memo validated against the + module's identity would keep serving the old class. Popping + :data:`sys.modules` and importing again replaces the module object too. + + """ + Dummy = self._dummy_protocol() + raw_module = importlib.import_module(RAW_MODULE) + + warm = Dummy._lookup_next_layer(Dummy.__proto__, 99) + self.assertIs(warm, raw_module.Raw) + + importlib.reload(raw_module) + self.assertIsNot(raw_module.Raw, warm) # the reload really did mint a class + self.assertIs(sys.modules[RAW_MODULE], raw_module) # in the same module object + self.assertIs(Dummy._lookup_next_layer(Dummy.__proto__, 99), raw_module.Raw) + + reloaded = raw_module.Raw + sys.modules.pop(RAW_MODULE) + fresh_module = importlib.import_module(RAW_MODULE) + self.assertIsNot(fresh_module, raw_module) # a new module object this time + self.assertIsNot(fresh_module.Raw, reloaded) + self.assertIs(Dummy._lookup_next_layer(Dummy.__proto__, 99), fresh_module.Raw) + + +if __name__ == '__main__': + unittest.main()