Skip to content

fix(protocols): a next-layer hit resolves its descriptor instead of memoising the class (#1559) - #1561

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1559-next-layer-reload-stale
Oct 10, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1559-next-layer-reload-stale

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 10, 2026 •

Copy link
Copy Markdown
Owner

Please follow the guide below

What is the purpose of your pull request?

Tick the commit type your subject line carries.

  • fix — corrects a defect
  • feat — adds a feature
  • perf — changes performance, not behaviour
  • refactor — changes neither behaviour nor performance
  • test — tests only
  • docs — documentation only
  • ci — workflows or build tooling
  • release — bumps the version or rolls up a distribution
  • chore — anything else

Description of your pull request and other information

Closes #1559.

ProtocolBase._lookup_next_layer (pcapkit/protocols/protocol.py:2183-2184) wrote the resolved class over a registered ModuleDescriptor. The fix resolves through ModuleDescriptor.klass, which reads sys.modules, on every lookup and writes nothing back, as the docstring already said it should. Six assertions that pinned the write-back now check the entry stays a descriptor. All six, plus the new tests, fail with the fix reverted.

Cost: a hit went from ~145 ns to ~294 ns. http.pcap resolves 4695 descriptors per extract(), about 0.7 ms on ~700 ms (0.1%). Interleaved wall-clock medians (main → fix): http.pcap 704.6 → 705.1 ms over 18 runs; tcp, ipv6, many_interfaces and test.pcap stayed within ±1%.

Behaviour change: Internet.register(TransType.UDP, UDP) used to warn "already registered" before any parse but not after a parse had reached UDP. It now warns both times. Pinned by test_registering_a_dispatched_code_warns_alike_before_and_after_a_parse.

Prose corrected: the docstrings in schema/link/ethernet.py and corekit/module.py (the count is re-measured: 296 resolutions on many_interfaces.pcapng, 48 of them fallbacks), test_dispatch_default_resolution_unit.py, and the 1.5.0 changelog entry, with CHANGELOG.md regenerated. test_layer_placement_unit.py drops isolate_modules(self), which only the write-back needed. Run last after its class's other tests, the test passes under pytest and plain unittest, and fails on main's protocol.py.

Known follow-ups, not fixed here: the same reload staleness exists at pcapkit/foundation/extraction.py:1354-1461 (__reassembly__/__traceflow__/__output__ write-backs), pcapkit/protocols/protocol.py:986 and pcapkit/protocols/misc/pcapng.py:1027-1035 (register-time resolution).

@JarryShaw JarryShaw added fix Pull requests that fix a defect (fix: subject prefix) review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Oct 10, 2026
@JarryShaw JarryShaw moved this to In review in PyPCAPKit Oct 10, 2026
…emoising the class (#1559)

ProtocolBase._lookup_next_layer wrote the class a registered ModuleDescriptor
resolved to back over the descriptor. After importlib.reload of the module,
dispatch kept reaching the pre-reload class; parsing mutated registries shared
by the whole process; and whether re-registering a shipped code warned
depended on what had been parsed.

- Every lookup now resolves through ModuleDescriptor.klass, which reads
  sys.modules, and nothing is written back. The docstring, which already said
  a memo would go stale, now matches the code.
- Prose describing the write-back is corrected: the ethernet schema callback,
  ModuleDescriptor.klass (count re-measured), two test docstrings, and the
  1.5.0 changelog entry, with CHANGELOG.md regenerated.
- Six assertions that pinned the write-back now check the entry stays a
  ModuleDescriptor; test_layer_placement_unit drops the isolation it needed
  only because of it; a new test covers reload, registry immutability and the
  register warning.

Cost: ~150 ns per hit, ~0.1% of extract() on http.pcap; not measurable in
wall clock. Narrow test selections and the protocols unittest leg pass.
@JarryShaw
JarryShaw force-pushed the fix/1559-next-layer-reload-stale branch from 2b075ca to 6c135cd Compare October 10, 2026 04:01
@JarryShaw JarryShaw added review: running A cross-review is in flight against the current head - no verdict yet and removed review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Oct 10, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: GOOD TO GO at 6c135cd59. Cross-reviewed on Sonnet; the author ran Opus.

  • The fix works. After a TransType.UDP hit, importlib.reload of the UDP module is picked up by the next lookup; on main the old class came back. I snapshotted the 7 next-layer registries and ran extract() over the 23 sample captures. On this head 0 entries changed. On main, 15 descriptors turned into classes (9 in Internet, 3 in Link, and 1 each in Frame, PCAPNG and TCP). I re-checked a subset myself: 0 of 24 Link/Internet entries changed.
  • The behaviour change covers every next-layer registry, not just UDP. Re-registering a code that has already been dispatched now warns both before and after a parse; on main it warned 1 then 0. Nothing in the repo relied on the silent re-register.
  • Fail-before: all six flipped assertions fail on main's protocol.py. test_protocol_base_unit.py:339 was masked by :265 in the same test, and fails once :265 is neutralised. None of them is weakened.
  • Cost: a hit takes 306–315 ns, up from 156–160 ns. Paired in-process runs put extract() on http.pcap at +3.1 ms (+0.46%), and a 12× capture at +0.38%.
  • Tests: these all pass: internet 475, link 89, application 242, transport 243, misc 230, top-level protocol tests, registry 43, interface 20, and the plain-unittest leg (1234 tests, 0 failures). tests/project/test_changelog_md.py passes too.

Non-blocking nits:

  • The 1.5.0 entry says the cost is "not measurable in extract() wall clock". Measured paired, it is about +0.4–0.5%, so "under 1%" would be accurate.
  • The PR body describes the change for Internet/UDP only.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 10, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Coverage: 90.02% (unit tier, Python 3.14, 6c135cd59, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 4 93.55%
pcapkit/const 18795 842 2356 756 92.09%
pcapkit/corekit 2240 67 722 34 96.32%
pcapkit/dumpkit 258 2 90 2 98.85%
pcapkit/foundation 2766 129 980 47 94.23%
pcapkit/interface 108 7 36 5 91.67%
pcapkit/protocols 16993 230 4546 191 98.02%
pcapkit/toolkit 635 98 198 5 83.31%
pcapkit/utilities 431 4 124 4 98.56%
pcapkit/vendor 4409 2342 1006 157 43.25%

Per-file detail: the coverage-html artifact of this run.

@JarryShaw
JarryShaw merged commit 3cbee6d into main Oct 10, 2026
39 checks passed
@JarryShaw
JarryShaw deleted the fix/1559-next-layer-reload-stale branch October 10, 2026 04:39
@github-project-automation github-project-automation Bot moved this from In review to Done in PyPCAPKit Oct 10, 2026
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix Pull requests that fix a defect (fix: subject prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

fix(protocols): _lookup_next_layer memoises the resolved class on a hit, so a reload leaves it stale

1 participant