fix(schema): stop EnumSchema.registry from retaining a lookup miss (#555) - #560
Merged
Merged
Conversation
Reading X.registry[code] for a code nobody registered inserted the default schema under that code, permanently: Option.registry is a class-level defaultdict, so one bare subscript -- Option.registry[OptionNumber(156)] -- made an unassigned TCP option number read back as registered for the rest of the process. Identical to the __proto__ dispatch defect #421/#425/#428 fixed one layer down, at the schema layer instead of the protocol layer. The fallback is deliberate -- it is how an unknown option, chunk or block falls back to its Unknown*/Unassigned* schema -- so only the retention is removed, not the fallback. - Add _EnumRegistry, a defaultdict subclass whose __missing__ returns the default without writing it. - EnumSchema.__init_subclass__ builds __enum__ as one for the common auto-created case, and swaps a manually-declared plain defaultdict (as PCAPNG.Option's outer namespaced mapping and TCP.MPTCP use) for the same safe type the moment the class body finishes -- before any external code can hold a reference to the original object, so .registry keeps returning the identical object on every later read. - Document the new class in docs/source/pcapkit/protocols/index.rst, alongside SchemaMeta/EnumMeta, since EnumMeta.registry and EnumSchema.registry's docstrings now cross-reference it. Does not cover PCAPNG.Option's per-namespace inner dicts, which are nested values rather than the __enum__ attribute itself and remain a plain, retention-unsafe defaultdict; fixing those belongs to pcapng.py. Adds tests/protocols/schema/test_enum_schema_registry_unit.py, each proven to fail without the fix. Closes #555.
This was referenced Sep 21, 2026
Merged
Closed
JarryShaw
added a commit
that referenced
this pull request
Sep 21, 2026
…port_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.
JarryShaw
added a commit
that referenced
this pull request
Sep 21, 2026
…port_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.
JarryShaw
added a commit
that referenced
this pull request
Sep 21, 2026
…port_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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #555.
Reading
X.registry[code]for a code nobody registered inserted the default schema under that code permanently.Option.registryis a class-leveldefaultdict, so one bare subscript was enough:Two consequences: unbounded growth, since every unrecognised code met while parsing is retained for the process lifetime; and cross-capture contamination, since the registry is class-level, so entries created parsing one file are visible to the next and behaviour becomes order-dependent.
This is the same defect #421/#425/#428 fixed for the
__proto__dispatch registries one layer down. #428's own review asked for the schema-layer follow-up and it was never filed until now, so the fix deliberately follows their shape rather than inventing a parallel one.The fallback is deliberate and is preserved. Returning a default on a miss is how an unknown option, chunk or block degrades to its
Unknown*/Unassigned*schema. Only the retention is removed._EnumRegistry, adefaultdictsubclass whose__missing__returns the default without writing it.EnumSchema.__init_subclass__builds__enum__as one for the common auto-created case, and swaps a manually-declared plaindefaultdict(as PCAP-NGOption's outer namespaced mapping and TCPMPTCPuse) for the same safe type as the class body finishes — before external code can hold a reference to the original.docs/source/pcapkit/protocols/index.rstdocuments the new class, which is why that file is in the diff.5 files, +241/-4, one commit, on top of
4c6b12189.Provenance, stated plainly: this was written by an agent whose session was killed by a host OOM before it could report. The commit and its rebase onto current
mainare verified by me; its test results are not — the author never handed back a fails-without proof, so the exit-code evidence this repository normally requires is missing. Treat the CI run and the cross-review as the only verification, and do not merge on the strength of the description alone.