Skip to content

Schema subclasses share one ABCMeta cache on Python <=3.10, so isinstance/issubclass return whichever answer was asked first #439

Description

@JarryShaw

On Python ≤ 3.10, every Schema subclass shares Schema's _abc_impl instead of getting its own. One ABCMeta subclass-check cache therefore serves the entire schema family, and because it is keyed only on the class being tested and not on the class it is tested against, the first answer for a given class is cached and returned for every later question about it.

The result: issubclass and isinstance against schema classes return whichever answer was asked for first.

Reproduction

Two fresh 3.10 interpreters, same tree, differing only in the order of the two questions:

import sys; sys.path.insert(0, '<tree>')
from pcapkit.protocols.schema.schema import Schema
from pcapkit.protocols.schema.internet.mh import (
    ANIGeoLocationSuboption as G, ANINetworkIdentifierSuboption as N)
order asked first answer second answer
issubclass(G, Schema) then issubclass(G, N) True True ✘ (siblings)
issubclass(G, N) then issubclass(G, Schema) False False ✘ (it is a Schema)
Python 3.10.20:  G._abc_impl is Schema._abc_impl  ->  True
Python 3.14.7 :  G._abc_impl is Schema._abc_impl  ->  False   (and both answers correct)

The first question is always right; the second always inherits the first's answer. No instances are involved, so this is not about __eq__, __hash__ or value equality — it is a shared cache object. SchemaMeta derives from abc.ABCMeta (pcapkit/protocols/schema/schema.py:136) and every schema descends from collections.abc.Mapping.

Verified on 3.10.20 and 3.14.7. CPython reworked the _abc caches across the 3.11/3.12 line, which is why 3.14 is unaffected — but 3.10 is in the supported matrix (pyproject.toml claims >=3.6, and CI runs 3.10 through 3.15).

Why this is worth fixing rather than avoiding

It surfaced through #437, where eight new Mobility Header helpers dispatched sub-options through a chain of isinstance(schema, Schema_X) tests. On 3.10 a false positive sent a sub-option down a sibling's branch (three AttributeErrors), and a cached negative made ListField.pack's isinstance(item, Schema) false, producing FieldValueError and zero-filled reconstructions — six CI failures on 3.10 while 3.14 was entirely green. Those helpers have been changed to dispatch on the wire type code, which is what the rest of that module does, so #437 no longer depends on this. The underlying defect is untouched.

grep finds 39 isinstance(…, Schema…) sites under pcapkit/ on main. Most ask a single question and so cannot see the bug. The ones that ask several in sequence can, and the clearest candidate is HTTP/2 frame dispatch:

pcapkit/protocols/application/httpv2.py:316   if isinstance(frame, Schema_UnassignedFrame):
pcapkit/protocols/application/httpv2.py:318   if isinstance(frame, Schema_DataFrame):
pcapkit/protocols/application/httpv2.py:321   if isinstance(frame, Schema_HeadersFrame):
pcapkit/protocols/application/httpv2.py:325   if isinstance(frame, Schema_PriorityFrame):
pcapkit/protocols/application/httpv2.py:327   if isinstance(frame, Schema_RSTStreamFrame):

That is the same shape that failed in MH. Whether it actually misbehaves on 3.10 depends on which question the process asked first, which makes it order- and import-dependent — the worst kind of latent defect, since it can pass CI and fail for a user. Others worth checking in the same pass: corekit/fields/collections.py:111, corekit/fields/misc.py:326, protocols/protocol.py:467, protocols/internet/ipv4.py:1195, protocols/internet/hopopt.py:1266, protocols/internet/esp.py:1321.

Two ways to fix, and they are not equivalent

  1. Make each schema class get its own _abc_impl — the root fix. It restores correct isinstance/issubclass everywhere at once, but it touches SchemaMeta, which every protocol in the tree depends on, so it needs its own change with a full byte-identical sweep behind it.
  2. Stop dispatching on isinstance where a type code is available — what protocols: complete the Mobility Header registry -- all 24 message types, 70 of 71 options, all 4 CGA extensions #437 did. Narrower, and arguably better design regardless, since dispatching on the wire code is what the registry already keys on. It does not fix the 37 other sites.

Both are probably wanted: (2) removes the dependency wherever a code exists, (1) stops the trap being re-laid by the next person who writes an isinstance chain.

Deliberately not folded into #437 — the blast radius belongs in its own change rather than in a Mobility Header PR.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions