Skip to content

fix(protocols): drop the warnings of a next layer replaced by Raw (#1580) - #1588

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1580-spb-truncation-warnings
Oct 10, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1580-spb-truncation-warnings

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 10, 2026 •

Copy link
Copy Markdown
Owner

Please follow the guide below

  • You will be asked some questions, please read them carefully and answer honestly

  • Put an x into all the boxes [ ] relevant to your pull request (like that [x])

  • Use Preview tab to see how your pull request will actually look like

  • Searched for similar pull requests

  • Followed the coding style (make pylint, make mypy, make isort) — Makefile flags on the changed modules, no new findings

  • make test passes, and a test case covers the change — narrow selections only, not the full suite

  • Added a changelog entry under docs/source/changelog/ and regenerated CHANGELOG.md, if the change is user-visible — N/A — centralised in docs(changelog): shared 1.5.0 changelog — long-lived, merges last (#610, #616, #617, #618, #620) #657

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

  • Root cause, not PCAP-NG-specific: ProtocolBase._parse_next_layer (pcapkit/protocols/protocol.py:2287 on main) parses the next layer on trial, then replaces it with Raw when its header was not captured (fix(protocols): a payload layer parsed from a short stream rebuilds default-filled octets #1170). The trial's warnings still reached the user. A SPB, EPB or Packet Block in either byte order, or a PCAP record, cut to 10 octets warned packet length < 0: -2/-4 for an Ethernet header the result does not have. This happened on parse, from_data and make.
  • Fix: hold_warnings (pcapkit/utilities/warnings.py) holds warn() calls in a ContextVar. _parse_next_layer uses it only for a layer captured shorter than its class's length hint; every other layer is parsed holding nothing, as before.
  • Kept and discarded layers: a kept layer replays its warnings where its trial ends, inside the same beholder guard, so under -W error the same layer becomes Raw. For example, Ethernet(IPv4(TCP[:10])) gives Ethernet:IPv4:TCP. A layer replaced by Raw drops its warnings.
  • Exceptions and restore: a BaseException passes through untouched. Restore is by token, plus a step over blocks left out of order (a plain token reset revives such a block).
  • Unchanged: a kept cut layer still warns (fix(pcap,pcapng): a cut PCAP header or PCAP-NG block rebuilds at full width #1470). A trial that raises and falls back to Raw through beholder keeps its warnings, so test.pcapng frame 3's two -2 stay.
  • Measured against main (all 23 sample captures, and each .pcap cut to 14 lengths, 238 files), with the fix(protocols): drop the warnings of a next layer replaced by Raw (#1580) #1588 reviewer's recorder:
    • Protocol chains and infos are identical under always, default and error.
    • Sample-capture warnings are identical: 27, in order.
    • The cut files drop 7,751 warnings, all from discarded trials.
  • Logger caveat: a held trial that raises now logs its error before its replayed warnings. Under error, main stopped at the first warning and never logged that error; the PR runs past it and logs it (test.pcapng and 48 cut files).
  • Overhead: 0.70 µs per next-layer parse in a micro-benchmark, about 3 ms per http.pcap extract (4,468 parses, none held). Over 3 runs of best-of-7, 3 extracts of http.pcap took 2.67 s on main and 2.73 s here (median), inside main's own 3% spread.
  • Tests: tests/protocols/test_trial_parse_warnings_1580_unit.py adds 16 tests, covering error-filter chains at every cut, BaseException, and blocks left out of order.

Closes #1580

@JarryShaw JarryShaw added fix Pull requests that fix a defect (fix: subject prefix) review: running A cross-review is in flight against the current head - no verdict yet labels Oct 10, 2026
@JarryShaw JarryShaw moved this to In review in PyPCAPKit Oct 10, 2026
@github-actions

github-actions Bot commented Oct 10, 2026 •

Copy link
Copy Markdown
Contributor

Coverage: 89.65% (unit tier, Python 3.14, 445e1ef4e, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 4 93.55%
pcapkit/const 18858 956 2366 870 91.04%
pcapkit/corekit 2334 67 746 35 96.43%
pcapkit/dumpkit 275 3 100 3 98.40%
pcapkit/foundation 2792 154 984 42 93.43%
pcapkit/interface 138 7 52 5 93.68%
pcapkit/protocols 16996 230 4540 191 98.02%
pcapkit/toolkit 708 96 234 6 85.35%
pcapkit/utilities 494 4 146 4 98.75%
pcapkit/vendor 4409 2342 1006 157 43.25%

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

@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: NEEDS CHANGES at 244bf1821. Cross-reviewed on Sonnet; the author ran Opus.

Needed:

  1. Under -W error, a whole subtree is demoted where only the leaf used to be.
    • What happens: a held warning is now replayed at the outermost hold, so the inner @beholder guards no longer catch it.
    • Example: Ethernet(IPv4(TCP cut to 10)) gives Ethernet:IPv4:TCP on main but Ethernet:Raw here. The same happens to options-internet.pcap frame 114, options-ipv6.pcap frame 13 and test.pcapng frame 3.
    • Scale: over 238 truncated variants, 5,294 frames got shorter chains and none got longer.
    • Fix: replay a kept warning where it used to surface, so the chains under error filters match main. Pin that in a test.
  2. A BaseException is replaced by the replay. KeyboardInterrupt, SystemExit or GeneratorExit unwinding through a hold that holds a warning gets replaced by SchemaWarning under an error filter, and beholder then swallows it. Only replay when exc_type is None or issubclass(exc_type, Exception).
  3. A suspended generator leaks its hold. _HELD.set(outer) restores by value, so a generator suspended inside a hold leaks the hold to its caller. Restore with the ContextVar token instead. No pcapkit code path hits this today.

Kept as ruled: a trial that raises and falls back to Raw through beholder still warns, as in test.pcapng frame 3. The ruling covers only a trial the #1170 guard replaces.

Confirmed:

  • Sample warnings: the 27 warnings across the 23 samples match main in category, message, file and line.
  • Dropped warnings: 7,751 of 37,223 warnings were dropped, exactly the contents of discarded holds.
  • Filters and dedup: warnings.filters and dedup are untouched under always, default, module and once.
  • Fail-before: 10 tests fail.
  • Merge: the PR merges cleanly into main.

@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 10, 2026
)

ProtocolBase._parse_next_layer parses a next layer on trial and keeps it
as Raw when its header was not captured (#1170), but the trial's warnings
were reported anyway. A PCAP-NG SPB, EPB or Packet Block, or a PCAP
record, cut to 10 octets warned "packet length < 0" twice for an Ethernet
header the result does not have, on parse, from_data and make.

* pcapkit/utilities/warnings.py: hold_warnings, a ContextVar-backed block
  that holds warn() calls with the frame each names. On keep it replays
  them where it is left, so an error filter raises inside the same guard;
  on discard it drops them. It lets a BaseException through, restores by
  token, steps over blocks left out of order, and touches no filter.
* pcapkit/protocols/protocol.py: _parse_next_layer holds only a layer
  captured shorter than its class's length hint and discards on the Raw
  fallback; every other layer is parsed holding nothing.
* tests: tests/protocols/test_trial_parse_warnings_1580_unit.py.
@JarryShaw
JarryShaw force-pushed the fix/1580-spb-truncation-warnings branch from 244bf18 to 445e1ef Compare October 10, 2026 07:47
@JarryShaw JarryShaw added review: running A cross-review is in flight against the current head - no verdict yet and removed review: needs-changes Cross-review at the current head says changes are required; see the verdict comment labels Oct 10, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: GOOD TO GO at 445e1ef4e. Round 2 was a Sonnet delta review; the author ran Opus. It merges cleanly into main and pairwise with #1594 and #1595.

  • Chains are unchanged. Under error-noeof, always and default there are 0 differences in protocol chain, info, frame count or error text across 23 samples and 2,210 truncated variants. That includes every cut length from 1 to 130 on all 17 .pcap files. I checked the -W error case Ethernet(IPv4(TCP[:10])) myself: Ethernet:IPv4:TCP, the same as main.
  • Sample warnings are byte-identical to main in category, message, file, line and order (27). The PR emits no warning main lacks, and the dropped ones are exactly the trials replaced by Raw.
  • The length-hint gate has no gap. __length_hint__ is a constant on all 44 protocol classes, including the 8 IPv6_Ext overrides. Every one of 26,369 held trials was discarded, and nothing was discarded without first being held.
  • BaseException passes through. KeyboardInterrupt, SystemExit and GeneratorExit propagate unchanged under error, and leave no hold behind.
  • The out-of-order generator case leaves _HELD as None, and later warnings are still emitted.
  • Fail-before: 12 tests fail on 244bf1821.

Corrections and nits:

  • Logging order. On test.pcapng and some cuts, a held trial's CRITICAL log record now precedes its replayed warnings under every filter, not only error as the PR body says. The warnings channel is unchanged.
  • Undocumented drop. A caller warning raised while a generator is suspended inside its hold is dropped when the generator closes. The docs mention the holding but not the drop.

@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
@JarryShaw
JarryShaw merged commit 589414e into main Oct 10, 2026
39 checks passed
@JarryShaw

Copy link
Copy Markdown
Owner Author

class wi the lower snake case name?

@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 10, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

On the naming question: hold_warnings was named after warnings.catch_warnings, the stdlib context-manager class it mirrors, so the call site reads with hold_warnings(...). But it breaks the house convention:

  • The only one: it is the only lowercase class pcapkit defines itself. The other, cached_property at pcapkit/utilities/compat.py:75, is a verbatim stdlib backport.
  • It needs a lint escape: it takes # pylint: disable=invalid-name at pcapkit/utilities/warnings.py:217.

It has never been released, since this PR added it, so renaming costs nothing. I've filed #1599 to rename it to HoldWarnings, and a worker is on it. Say if you would rather keep the stdlib-style name.

JarryShaw added a commit that referenced this pull request Oct 10, 2026
`hold_warnings`, added in #1588, was the only lowercase class pcapkit defines
itself, and needed `# pylint: disable=invalid-name`. Rename it to
`HoldWarnings` in `pcapkit/utilities/warnings.py`, its one caller in
`pcapkit/protocols/protocol.py`, the two docs pages and #1588's test file,
and drop the disable. It has not shipped, so no alias is kept.

`HoldWarnings` is documented and imported from another module, so it is
public: list it in `pcapkit.utilities.warnings.__all__`, beside `warn`.

The `pcapkit_held_warnings` ContextVar and the test's `_HoldNothing` stand-in
keep their names: neither embeds the class name.

A new test pins that `HoldWarnings` is importable and exported, and that
neither `pcapkit.utilities.warnings` nor `pcapkit.protocols.protocol` has a
`hold_warnings` attribute.
@JarryShaw JarryShaw added this to the PyPCAPKit v1.5 milestone Oct 26, 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): a trial next-layer parse that is replaced by Raw still emits its SchemaWarnings

1 participant