Skip to content

fix(corekit): copy SeekableReader's octets and feed MultiDict a dict, and gate GraalPy's limits (#1963) - #1974

Merged
JarryShaw merged 6 commits into
mainfrom
fix/1963-graalpy-gaps
Oct 20, 2026
Merged

JarryShaw merged 6 commits into
mainfrom
fix/1963-graalpy-gaps

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner

What is the purpose of your pull request?

  • fix — corrects a defect

Description of your pull request and other information

Sorts #1963's GraalPy-only failures into pcapkit defects and interpreter gates. Every failure was reproduced on a real GraalPy 25.4.4 before the change.

  • fix(corekit) SeekableReader. On GraalPy, BytesIO reads alias an exported buffer, so a later buffer slide rewrote octets already returned. Reads now copy out of the view.
  • fix(corekit) MultiDict. On GraalPy, dict.__init__/dict.update store pairs through the overridden __setitem__, which nested each value list. __init__ and __setstate__ now pass a dict.
  • Tests. tests/corekit/test_graalpy_semantics_1963_unit.py reproduces both behaviours on CPython, and fails without the fixes.
  • GraalPy gates cover pickle protocols 0 and 1, PCAP claims above sys.maxsize, resource.setrlimit and _abc_impl. Each gated test still runs on CPython.

Left open on #1963: GraalPy's speed, and a read clamp for incl_len claims above sys.maxsize on 32-bit-Py_ssize_t builds.

Refs #1963

… and gate GraalPy's limits (#1963)

- SeekableReader copies buffered octets out of its view: GraalPy's BytesIO
  read/read1/readline from an exported buffer's start can return bytes
  sharing its memory, which the next buffer slide overwrote
- MultiDict hands dict.__init__ and dict.update a dict, not pairs: GraalPy
  stores pairs through the overridden __setitem__, nesting each value list
- new test_graalpy_semantics_1963_unit reproduces both on CPython with
  stand-ins for GraalPy's behaviour
- gate in the tests: pickle protocols 0/1 (typing.Generic is not a heap
  type), the >sys.maxsize PCAP claim, resource.setrlimit and _abc_impl;
  bound the huge BytesField length by sys.maxsize

Refs #1963
@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 20, 2026
@JarryShaw JarryShaw moved this to In review in PyPCAPKit Oct 20, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

NEEDS CHANGES at f286da096, two small fixes and one question for you. A Sonnet review reproduced both bugs on a real GraalPy 25.4.4: 7 failed with main's files, 28 passed with the PR's. The SeekableReader copy covers every returning path, including readinto and peek, and timeit shows no measurable cost. The gates use sys.implementation and sys.maxsize, and every gated test still runs on CPython.

Question. MultiDict.__init__ and __setstate__ are the werkzeug 2.3.8 code with only annotations and a renamed variable. I checked the wheel: upstream has the same dict.__init__(self, ((k, l[:]) for ...)) and dict.update(self, value). The PR description's "already differ" is wrong for these two blocks. Do you accept diverging from upstream here, with a comment naming GraalPy, or should the port stay verbatim and GraalPy's MultiDict be gated instead?

Fixes:

  • Two cases in the new test (OrderedMultiDict in test_a_copy_keeps_the_value_lists, and the dict state case) pass without the fix, so they add no coverage. Drop them or say why they are there.
  • The module docstring opens with a count ("Two GraalPy behaviours").

@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment needs: decision Waiting on the maintainer to decide — not blocked by other work and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 20, 2026
@JarryShaw JarryShaw moved this from In review to Needs decision in PyPCAPKit Oct 20, 2026
#1963)

- drop the OrderedMultiDict copy and the plain-dict state cases, which
  passed without the fixes
- say why the stand-in check stays: it guards the stand-in, not the fix
- drop the count from the module docstring
- correction to the first commit's review: werkzeug 2.3.8's
  MultiDict.__init__ and __setstate__ match pcapkit's apart from the
  annotations and the l -> v rename

Refs #1963
@JarryShaw

Copy link
Copy Markdown
Owner Author

Correction to my last comment. Neither this PR's description nor its commits claim the MultiDict blocks "already differ" from upstream. That claim was only in the worker's report to me. The upstream comparison itself stands, and the question on it is still open. 9dced1730 drops the two test cases that passed without the fix and removes the count from the docstring. The new test still fails without the fixes on CPython and on GraalPy 25.4.4 (5 failed), and passes with them. multidict.py is untouched until you decide.

@github-actions

github-actions Bot commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Coverage: 90.07% (unit tier, Python 3.15, 1258482df, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 2 20 2 96.77%
pcapkit/const 19597 881 2618 867 92.11%
pcapkit/corekit 2524 70 800 36 96.57%
pcapkit/dumpkit 276 3 100 3 98.40%
pcapkit/foundation 2916 164 1016 34 93.59%
pcapkit/interface 138 7 52 5 93.68%
pcapkit/protocols 17411 224 4580 189 98.10%
pcapkit/toolkit 872 177 298 8 77.18%
pcapkit/utilities 522 2 158 4 99.12%
pcapkit/vendor 4431 2341 1018 156 43.64%

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

@JarryShaw

Copy link
Copy Markdown
Owner Author

Not sure i followed. But MultiDict may have been outdated from Wekzeug's latest implementation, we can upgrade to match. For GraalPy support, if necessary, we can cherrypick and gate certain features or branch certain feature's implementations.

@JarryShaw

Copy link
Copy Markdown
Owner Author

Thanks, that settles it. My question was whether to keep the werkzeug 2.3.8 copy verbatim or hand-patch it. Following your ruling, a worker is upgrading the MultiDict port to match the latest werkzeug release. If upstream already avoids the pairs path that GraalPy routes through __setitem__, the fix becomes plain upstream code. Anything GraalPy still needs gets the narrowest gate or branch, with a comment. The new head will get a fresh review.

@JarryShaw JarryShaw removed the needs: decision Waiting on the maintainer to decide — not blocked by other work label Oct 20, 2026
@JarryShaw JarryShaw moved this from Needs decision to In review in PyPCAPKit Oct 20, 2026
- port MultiDict, OrderedMultiDict and iter_multi_items from werkzeug 3.1.9
  rather than 2.3.8: dict reached through super(), any Mapping and set
  values accepted, | and |= added, setdefault/setlistdefault simplified
- keep the local adaptations: dict base without TypeConversionDict,
  MissingKeyError/UnsupportedCall, get() and getlist() without type=,
  OrderedMultiDict.deepcopy order fix, and no DeprecationWarning on
  OrderedMultiDict
- werkzeug 3.1.9 still feeds pairs to dict.__init__ and update, which
  GraalPy routes through __setitem__; keep the two dict-building lines as
  the only behavioural divergence, noted in place
- the GraalPy stand-in now sits in the MRO behind MultiDict, since
  super() no longer reads a module-global dict
- test the new Mapping/set handling and the | operators

tests/corekit and every test module importing the multidict pass on
CPython; the #1963 and multidict tests pass on GraalPy 25.4.4.

Refs #1963
@JarryShaw

Copy link
Copy Markdown
Owner Author

Pushed a81cc5225. The MultiDict port now matches werkzeug 3.1.9, the latest release, keeping pcapkit's local adaptations.

  • Upstream does not avoid GraalPy's pairs path. 3.1.9 still hands pairs to super().__init__ and super().update, so the two dict-building lines stay, each with a GraalPy note. They run on every interpreter, because CPython's result is the same either way.
  • Behaviour changes inherited from upstream:
    • MultiDict | x now returns a MultiDict.
    • MultiInfo | x raises UnsupportedCall.
    • set values expand into separate values.
    • Any Mapping is read by its items.
  • Tests: tests/corekit passes on CPython (666 passed). The four multidict and test: sort the remaining GraalPy-only failures into defects and gates #1963 modules pass on GraalPy 25.4.4 and fail there with main's multidict.py.

A cross-review is starting.

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

Copy link
Copy Markdown
Owner Author

GOOD TO GO at a81cc5225. A Sonnet review diffed every ported class against werkzeug 3.1.9 and found only the stated adaptations and the two GraalPy lines. No pcapkit caller uses | on these types or passes set values. With the GraalPy lines reverted, the regression tests fail on CPython. GraalPy itself was re-run only by the worker.

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

Copy link
Copy Markdown
Owner Author

dont add versionchanged in MultiDict, it's a port from the existing source code. aim to be vertibam as possible to the original source.

@JarryShaw

Copy link
Copy Markdown
Owner Author

Understood. The worker is removing the versionchanged notes this PR added and bringing the ported classes back to werkzeug 3.1.9's text wherever pcapkit's functional adaptations allow. The GraalPy dict-building lines stay, with a one-line comment each. The new head will get a fresh review.

@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 Oct 20, 2026
- drop the versionchanged notes and explanatory prose this PR added
- restore werkzeug 3.1.9's docstrings, comments, overloads, annotations
  (as strings) and code layout for the ported classes
- keep pcapkit's adaptations: dict + Generic base, MissingKeyError and
  UnsupportedCall, no type= on getlist, local get() and _missing, the
  #1295 OrderedMultiDict.deepcopy, poplist's [] default, and no
  OrderedMultiDict deprecation warning
- keep the two GraalPy dict-building lines, with shorter comments

tests/corekit passes on CPython; the multidict and #1963 modules pass on
CPython and GraalPy 25.4.4.

Refs #1963
@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 20, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

NEEDS CHANGES at 3164cafd5. Project tests fails the #1923 pragma-name guard: multidict.py:177 uses useless-parent-delegation, a real pylint message (W0246) that is missing from the guard's KNOWN_MESSAGES. The pragma sits on upstream's __iter__ workaround, so it stays. The fix adds the name to the list.

@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 Oct 20, 2026
- add useless-parent-delegation (W0246) to KNOWN_MESSAGES: the pragma on
  MultiDict.__iter__ keeps upstream's CPython 87412 fast-path workaround

Refs #1963
@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 20, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

GOOD TO GO at 8f98e6b11. A Sonnet delta review found the diff is just the one KNOWN_MESSAGES entry, which is alphabetical. It also showed the pragma is needed: with it stripped, pylint reports W0246 on __iter__. The pragma-name guard passes.

@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 20, 2026
- add __tier__ = 'ALL': the module pins GraalPy-only semantics, so it has
  to keep running on every interpreter leg once ALL becomes opt-in (#1978)

Refs #1963
@JarryShaw

JarryShaw commented Oct 20, 2026 •

Copy link
Copy Markdown
Owner Author

Pushed 1258482df. It adds only __tier__ = 'ALL', with its reason comment, to test_graalpy_semantics_1963_unit.py. That keeps the module on every leg once #1978 makes ALL opt-in. Under main's rule it already resolves to ALL, so nothing changes today. The tier guard passes (128 passed) and the module passes. A delta review is running.

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

Copy link
Copy Markdown
Owner Author

GOOD TO GO at 1258482df. A Sonnet delta review found the diff is only the marker, in the guard's accepted shape, and that it resolves to ALL. Merged with #1978, two modules this PR gates on GraalPy need __tier__ declared: test_fallback_cycle_unit and test_mh_timestamp_info_1886_unit. #1978 will add both markers, so this PR stays as it is.

@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 20, 2026
JarryShaw added a commit that referenced this pull request Oct 20, 2026
…#1958)

- Opt in the modules the review found on LATEST despite the criterion:
  pickling and copying, the warning machinery and exception hooks,
  struct and int boundary tests, and the modules #1974 gates on
  GraalPy: fallback cycle, MH timestamp and the two #1541 overrun
  tests that compare against sys.maxsize.
- Opt Scapy's engine and toolkit modules into ALL like the other
  engines, and PyShark's toolkit and non-packet modules with them.
- State each demoted vendor module's LATEST tier and reason.
- branches_on_interpreter() also sees sys.flags, sys._is_gil_enabled,
  sys.maxsize comparisons, sysconfig.get_config_var() and
  hasattr/getattr probes of sys; its
  docstring and the guard's message say the check is syntactic and
  best-effort.
- _tiers.py and workflows.rst "Test Tiers" give the Scapy and vendor
  reasons.

Tier guard, #1732, #1954, #1958 pins and every touched module pass on
CPython 3.14.
@JarryShaw
JarryShaw merged commit 62e274f into main Oct 20, 2026
59 checks passed
@JarryShaw
JarryShaw deleted the fix/1963-graalpy-gaps branch October 20, 2026 15:15
@github-project-automation github-project-automation Bot moved this from In review to Done in PyPCAPKit Oct 20, 2026
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 20, 2026
JarryShaw added a commit that referenced this pull request Oct 20, 2026
* ci(unit-tests): run the whole suite on latest CPython (#1958)

- test, integration: a step on the latest leg alone installs the whole
  suite's extras (Scapy, PyShark, vendor, docs, cli on top of the base);
  every other leg keeps its install line and runs the ALL tier.
- project-tests, gate: install the same set.
- tests/_dependency_gates.py: read a latest-only install step as that
  leg's extras; drop the exclusions it closes (HAS_SCAPY, HAS_PYSHARK,
  HAS_RUNTIME, HAS_VENDOR_DEPS, HAS_SPHINX, HAS_DOCUTILS, HAS_FURO).
- tests/project/test_whole_suite_on_latest_1958.py pins the lines, the
  leg, the gates closed and the docs; tier-guard, #1732 and #1954 pins
  follow the new reading.
- workflows.rst describes it; unit-tests.yml line citations re-anchored.

Selection steps and tests/_tiers.py are unchanged.

* ci(unit-tests): say where the latest leg gets cli, crypto and NGAP (#1958)

- test, integration: the install comments said cli, and crypto and NGAP,
  are not installed; the latest leg's whole-suite step installs them.
- workflows.rst, releasing.rst: unit-tests.yml line citations re-anchored.

* test(tiers): make ALL a curated opt-in tier (#1958)

- tests/_tiers.py: module_tier() is the declared tier, else DEFAULT_TIER
  (LATEST); rule_tier() goes. all_tier_paths() lists ALL, and
  branches_on_interpreter() flags a module that compares the interpreter
  itself. The docstring states the curation criterion (#1963).
- Curated modules opt in with `__tier__ = 'ALL'` and a reason: version
  gates, GC/weakref, int/struct limits, pickling and copying, metaclass
  and enum machinery, IO, threading and hooks, engines whose releases
  vary by interpreter, the hand-picked round trips, and a smoke slice
  per protocol family. Six vendor markers with no such reason are gone.
- tests/test_tier_guard.py: ALL is opt-in, every ALL module declares it,
  and a module that branches on the interpreter must state its tier;
  the mixed-directory split test no longer assumes tests/integration is
  all ALL. test_sweeps_on_latest_only_1732 compares what the ALL
  fixture selection drops with the LATEST modules it reaches.
- tests/project/test_curated_all_tier_1958.py pins the opt-in rule, the
  strict subset, the ruling-B round trips and the docs.
- workflows.rst "Test Tiers" and the `test` job's `Run unit tests`
  comment describe it.

Tier guard, #1732, #1954 and #1958 pins pass on CPython 3.14.

* test(tiers): opt the remaining interpreter-sensitive modules into ALL (#1958)

- Opt in the modules the review found on LATEST despite the criterion:
  pickling and copying, the warning machinery and exception hooks,
  struct and int boundary tests, and the modules #1974 gates on
  GraalPy: fallback cycle, MH timestamp and the two #1541 overrun
  tests that compare against sys.maxsize.
- Opt Scapy's engine and toolkit modules into ALL like the other
  engines, and PyShark's toolkit and non-packet modules with them.
- State each demoted vendor module's LATEST tier and reason.
- branches_on_interpreter() also sees sys.flags, sys._is_gil_enabled,
  sys.maxsize comparisons, sysconfig.get_config_var() and
  hasattr/getattr probes of sys; its
  docstring and the guard's message say the check is syntactic and
  best-effort.
- _tiers.py and workflows.rst "Test Tiers" give the Scapy and vendor
  reasons.

Tier guard, #1732, #1954, #1958 pins and every touched module pass on
CPython 3.14.
JarryShaw added a commit that referenced this pull request Oct 20, 2026
…#1975) (#1979)

* fix(protocols): handle declared lengths above sys.maxsize as overruns (#1975)

Where Py_ssize_t is 32 bits wide (GraalPy, 32-bit CPython) a read or seek
past sys.maxsize raises OverflowError instead of coming back short, so a
PCAP incl_len or PCAP-NG length reaching that far crashed the reader before
the overrun handling could keep the record as captured.

- Schema.unpack bounds its payload and field reads at sys.maxsize.
- Frame.read bounds the record read, and a next-record offset past
  sys.maxsize becomes the end of the octets read.
- PCAPNG.read and _read_block_cut bound the block read.
- SeekableReader._fill bounds a READ_CHUNK of -1 at sys.maxsize.
- Lift #1974's sys.maxsize skips from the #1541 tests, and add a test
  that runs the narrow build's limit on any interpreter.

Claims that fit read exactly as before.

Closes #1975

* fix(protocols): cite #1975 as #NNN in plain comments (#1975)

* fix(protocols): advance past a record longer than sys.maxsize instead of truncating (#1975)

A record or block the file really holds past sys.maxsize was read only to
the limit, and the next one was then parsed from inside it.

- corekit.io.advance moves a stream forward any distance: relative seeks of
  at most sys.maxsize, or chunked read-and-discard on a SeekableReader or a
  non-seekable stream.
- Frame.read and PCAPNG.read call it when the bounded read came back whole,
  so the next record starts at the declared end; a short read is unchanged.
- The PCAP and PCAP-NG engines cap the read-ahead at sys.maxsize, and the
  PCAP frame schema reads no more than the header and payload can hold in
  one read, so a SeekableReader keeps the record start in its window.
- SeekableReader._fill bounds any READ_CHUNK at sys.maxsize.
- Tests cover a sparse file holding a record past a lowered limit, seekable
  and through a pipe, for PCAP and PCAP-NG.

Closes #1975
@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.

1 participant