Skip to content

fix(corekit): pickle an Info or Schema under protocols 0 and 1 through the protocol-2 reduction (#2714) - #2722

Merged
JarryShaw merged 1 commit into
mainfrom
fix/2714-pickle-protocol-2
Nov 3, 2026
Merged

fix(corekit): pickle an Info or Schema under protocols 0 and 1 through the protocol-2 reduction (#2714)#2722
JarryShaw merged 1 commit into
mainfrom
fix/2714-pickle-protocol-2

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Nov 3, 2026

Copy link
Copy Markdown
Owner

What is the purpose of your pull request?

  • fix — corrects a defect

On GraalPy, no Info, Schema or reassembly engine can be pickled under protocol 0 or 1. Below protocol 2, copyreg._reduce_ex walks the bases to the first one that is not a heap type and calls it. On GraalPy that base is typing.Generic, so the call raises TypeError: typing.Generic() takes no arguments. That is 32 of the 80 GraalPy reassembly failures. The other 48 come from pytest's repr of the failing frame, which resolves the DeferredPacket; pickling never mutates the original.

  • Fix: a shared mixin, pcapkit.utilities.pickling.ReduceAsProtocol2, whose __reduce_ex__ uses the protocol-2 reduction for every protocol. Info, Schema and ReassemblyBase take it. There is no GraalPy-specific branch. ProtocolBase, FieldBase, Extractor and TraceFlowBase are left alone, because they fail on CPython too: they hold a BytesIO, a lambda or a local class.
  • Compatibility: pickles load across main and this PR in both directions, at protocols 0, 1, 2 and 5. The NO_VALUE sentinel identity holds, and a subclass's own __reduce__ still wins.
  • Tests: a CPython stand-in for GraalPy's base walk fails the new tests on main. The GraalPy gates in the 1842 and 1886 tests are removed and the 2417 test is widened, so this PR's GraalPy leg exercises protocols 0 and 1 directly.

The mixin is public and documented, as the other pcapkit.utilities helpers are. The review suggested _ReduceAsProtocol2 as an option; say if you prefer that.

Closes #2714

@JarryShaw JarryShaw added this to the PyPCAPKit v1.6 milestone Nov 3, 2026
@JarryShaw JarryShaw added bug Issues reporting a defect (set by the bug report template; a default, not an assessment) review: good-to-go Cross-review at the current head says ready; CI state is separate labels Nov 3, 2026
@JarryShaw

JarryShaw commented Nov 3, 2026

Copy link
Copy Markdown
Owner Author

GOOD TO GO at e2a5d7a01, from a Sonnet review.

  • Mechanism: the walk in copyreg._reduce_ex was read against CPython's source. The stand-in's error matches the GraalPy log.
  • Compatibility: pickles of Info, Schema, NO_VALUE and OrderedMultiInfo load across main and this PR in all four directions, at protocols 0, 1, 2 and 5.
  • Classes left without the fix: each is justified. EngineBase and TraceFlowBase on their own are unverified.
  • Tests and guards: the new tests fail on the base, and the targeted guards pass. The full regression sweep ran out of review time, so CI covers it.

@JarryShaw JarryShaw mentioned this pull request Nov 3, 2026
Closed
@JarryShaw JarryShaw moved this to In review in PyPCAPKit Nov 3, 2026
@JarryShaw

JarryShaw commented Nov 3, 2026

Copy link
Copy Markdown
Owner Author

let's name the module pickle and reuse the mixin in all other classes where we reinvented the wheel. and also, if possible, we extract the reinvented wheels into this pickle module as a mixin to share.

@JarryShaw

JarryShaw commented Nov 3, 2026

Copy link
Copy Markdown
Owner Author

Ruling taken. Three changes:

  1. The module becomes pcapkit.utilities.pickle. Under absolute imports it does not shadow the standard library's pickle.
  2. Every class that rolled its own reduction uses the shared mixin instead, wherever that mixin fits.
  3. The other hand-rolled reductions are pulled into pcapkit.utilities.pickle as shared mixins. Candidates include Deferred's (fix(reassembly): Deferred(Generic[_KT]) still fails to pickle under protocol 0/1 on GraalPy #2433), MultiInfo's and OrderedMultiDict's, and the sentinels' __reduce__. A class keeps its own only where its semantics truly differ, with the reason stated.

The original worker is amending this PR. review: needs-changes until the fold lands and is reviewed.

@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: good-to-go Cross-review at the current head says ready; CI state is separate labels Nov 3, 2026
@JarryShaw
JarryShaw force-pushed the fix/2714-pickle-protocol-2 branch from e2a5d7a to 30f3ada Compare November 3, 2026 04:08
@JarryShaw

JarryShaw commented Nov 3, 2026

Copy link
Copy Markdown
Owner Author

Pushed e2a5d7a01→30f3ada13, one commit on 596dcfaf8. It applies the ruling.

  • The module is renamed to pcapkit.utilities.pickle. It holds ReduceAsProtocol2, ReduceToName (singletons), ReduceToItems (mappings) and reduce_to (a reducer on one instance).
  • Classes moved onto the shared reductions: Info, Schema, ReassemblyBase, Deferred, FrameRecord, the sentinels, multidict's _missing, OrderedMultiDict and MultiInfo, plus both unregistered-member reducers.
  • Classes that keep their own reduction: MultiDict round-trips its state, the exported _OrderedMultiDict carries __excluded__, dpkt's RecordTimestamp would lose resolution under Decimal's reduction, and _TextField passes raw to __new__. Each one's semantics differ.
  • Removed name: numbers._reduce_unregistered_member shipped in v1.5.x and appeared in the docs, so it gets a versionremoved.
  • Compatibility: pickles load in all four directions between main and this PR, 72 of 72 each, at protocols 0, 1, 2 and 5.

A Sonnet re-review is running; review: running.

@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 Nov 3, 2026
@JarryShaw

JarryShaw commented Nov 3, 2026

Copy link
Copy Markdown
Owner Author

GOOD TO GO at 30f3ada13, from a Sonnet re-review.

  • The ruling is applied. Every __reduce__, __reduce_ex__, __getstate__, __setstate__ and copyreg hit in pcapkit/ either uses a shared reduction or is justified as genuinely different.
  • Compatibility was re-run independently. The reviewer dumped pickles at protocols 0, 1, 2 and 5 on main and on this PR, and loaded each on the other. That covered sentinels, multidicts, Info, an unregistered member, FrameRecord, Deferred, and an engine mid-datagram. Everything loads in all four directions. Sentinels, multidicts and unregistered members are byte-identical to main.
  • One small leftover is being folded in. Deferred.__getstate__ now returns exactly what object.__getstate__ does, so it is a remaining reinvented wheel. It goes; __setstate__ stays to read the legacy state.

@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 Nov 3, 2026
@JarryShaw
fix(corekit): share the pickle reductions in pcapkit.utilities.pickle…
e85ef22 
…, protocol 2's for Generic classes (#2714)

Below protocol 2, object.__reduce_ex__ hands an instance to
copyreg._reduce_ex, whose MRO walk stops at the first non-heap base and
calls it on the instance. GraalPy builds typing.Generic as a built-in, so
the walk stopped at Generic and raised "typing.Generic() takes no
arguments" for every Info, Schema and reassembly engine.

- pcapkit.utilities.pickle holds the reductions classes had each written:
  ReduceAsProtocol2 (protocol 2's under every protocol), ReduceToName
  (a singleton's global name), ReduceToItems (a call on the item pairs)
  and reduce_to (a per-instance reducer).
- Info, Schema, ReassemblyBase, Deferred and FrameRecord take
  ReduceAsProtocol2, and Deferred drops a __getstate__ matching Python's
  default; Sentinel and multidict's missing marker take ReduceToName;
  OrderedMultiDict and MultiInfo take ReduceToItems; the unregistered
  enum members of EnumRegistry and EnumField use reduce_to.
- New tests stand in GraalPy's walk for copyreg._reduce_ex on CPython;
  the MH timestamp and sentinel tests drop their GraalPy protocol-2 floor,
  and the TCP datagram pickle test runs every protocol.

Pickling never resolved the deferred packet: pytest's failure report
renders the datagram with repr, which does, so the tests that pickle one
datagram under each protocol found it resolved after protocol 0 failed.
@JarryShaw
JarryShaw force-pushed the fix/2714-pickle-protocol-2 branch from 30f3ada to e85ef22 Compare November 3, 2026 04:30
@JarryShaw

JarryShaw commented Nov 3, 2026

Copy link
Copy Markdown
Owner Author

Fold pushed: 30f3ada13→e85ef22dc, still one commit on 596dcfaf8.

  • Deferred.__getstate__ is removed and __setstate__ stays, because it still reads the legacy flat state. Python's default state equals the removed method's output on both 3.8 and 3.14, for plain, slotted, name-mangled, __weakref__ and __dict__ subclasses, so no version gate is needed. It was never documented, so it gets no version note.
  • The four-way pickle matrix against main passes 72 of 72 in every direction, on both 3.8 and 3.14. The pickle and reassembly tests pass, and I re-ran the key modules myself.

The verdict stays GOOD TO GO. This delta is the reviewer's own suggestion, applied.

@JarryShaw
JarryShaw merged commit 01f0634 into main Nov 3, 2026
56 of 57 checks passed
@JarryShaw
JarryShaw deleted the fix/2714-pickle-protocol-2 branch November 3, 2026 04:35
@github-project-automation github-project-automation Bot moved this from In review to Done in PyPCAPKit Nov 3, 2026
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Nov 3, 2026
JarryShaw added a commit that referenced this pull request Nov 3, 2026
@JarryShaw
docs(changelog): fold the makers' fixed-size octets, pcapkit.utilitie…
41f7e7e 
…s.pickle, FRAD's pad and the TCP and HIP maker fixes into 1.6.0 (#2449)

- Breaking: a fixed-size octet field given any other width raises, where
  1.5.x padded or cut it, and IPv4's Timestamp option refuses a slot count
  past what it holds, where 1.5.x cut it (#2691); folded into the IPv4 to HIP
  makers' range-check entry, with the ZigBee keys into the PCAP-NG entry. Old
  values measured against v1.5.1.
- Added: pcapkit.utilities.pickle, whose ReduceAsProtocol2 lets Info, Schema
  and the reassembly engines pickle under protocols 0 and 1 on GraalPy, and
  numbers._reduce_unregistered_member's removal (#2722).
- Fixed: TCP's raw-octet options, HIP's default version, HIP's ENCRYPTED IV
  and its cipher on parse (#2724).
- Folded into unreleased entries: FRAD's pad field and make(pad=) (#2721).
- No entry for the docstring-only 596dcfa, the test-only 149cced, or
  #2724's MH annotation.
- CHANGELOG.md regenerated; util/changelog_md.py --check passes.
@JarryShaw JarryShaw mentioned this pull request Nov 3, 2026
Open
1 of 5 tasks
JarryShaw added a commit that referenced this pull request Nov 3, 2026
@JarryShaw
docs: resolve the pad_length, _make_uint and copyreg references left by
6b63ee9 
#2704 and #2722

- protocol.rst linked pad_length at pcapkit.corekit.fields.strings, its home
  before #2704; it links pcapkit.corekit.schema.padding now.
- protocol.rst documented Protocol._make_uint twice, beside _make_index and
  among the value helpers; the value-helper entry stays, in the Protocol
  autoclass, the ProtocolBase method group and its link list alike.
- ReduceAsProtocol2 names copyreg._reduce_ex and copyreg.__newobj__ as
  literals, since intersphinx holds neither private name.
- HIP and IPv6_Route filed their #2676 note under versionremoved 1.6.0; it
  moves, unreworded, into each class's versionchanged 1.6.0 list.

Docstrings and .rst only; the AST is equal once docstrings are stripped.
This was referenced Nov 3, 2026
Merged
Closed
JarryShaw added a commit that referenced this pull request Nov 3, 2026
@JarryShaw
refactor(utilities): rename utilities.lazy to utilities.importlib (#2747
f45c82b 
) (#2748)

The owner asked to rename the module holding `lazy_exports`. `importlib`
follows the stdlib-domain naming `pcapkit.utilities.pickle` set (#2722);
`imp` was passed over as the name of the stdlib module Python 3.12 removed.

`lazy_exports` keeps its name. Every `pcapkit.const` package imports it
from the new path, and the API page (renamed with its toctree entry), the
const-vendor pairing convention and the tests follow. The old path never
shipped, so it gets no re-export, no shim and no deprecation note.

The module's own `import importlib` is absolute, so it still binds the
standard library module, and `sys.modules['importlib']` is unchanged after
`import pcapkit`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

bug Issues reporting a defect (set by the bug report template; a default, not an assessment)

Projects

PyPCAPKit
Status: Done

Milestone

PyPCAPKit v1.6

Development

Successfully merging this pull request may close these issues.

fix(corekit): an Info cannot be pickled under protocols 0 and 1 on GraalPy

1 participant

@JarryShaw