fix(foundation): give no_eof a way to stop, so extract() returns (#620) - #639
Conversation
|
Note on landing order — these two PRs touch the same file and will conflict textually, trivially. #636 (input-stream ownership, #610) is independent of this one in substance: different function, different root cause, no shared logic. But both add a private helper in the same place, between the
One substantive interaction worth knowing, in this direction: this PR makes a |
fe83ed0 to
cf0d404
Compare
|
Amended to
So Re-verified after the amend: |
Cross-review: NEEDS CHANGES — now addressed in
|
| Claim | Verdict |
|---|---|
C1 reproduction real on main |
CONFIRMED |
| C2 exactly three affected loops, all three fixed | CONFIRMED |
| C3 termination signal sound | derivation CONFIRMED; soundness claim REFUTED for seekable inputs |
C4 no_eof not reduced to a no-op |
CONFIRMED (mutation-tested both directions) |
| C5 live-pipe measurement | measurement CONFIRMED to the centisecond; conclusion REFUTED as generalised |
C6 except Exception absorption |
outcome partly confirmed; mechanism REFUTED |
| C7 tests fail on baseline, 5 behavioural / 6 API-absence | CONFIRMED, split exact |
| C8 nothing regressed | CONFIRMED, exit 0 |
C9 EOFError from __call__ defensible |
CONFIRMED as defensible |
Worth noting on C4: it did not take the retry test on trust, it mutation-tested it. _note_eof_progress → return False (the flag silently a no-op) produced 6 failures; → return True (the original bug) produced 9. Both directions are genuinely pinned.
Required change 1 — a real regression, which I reproduced myself
The soundness argument was made on a pipe, where a blocking read masks the question, and I generalised it to all inputs. A seekable regular file does not block: it reports end of stream at once, so the two probes fall microseconds apart and any pause in the writer ends the extraction.
I re-ran this independently rather than accepting the report, driving through __next__, appending on a record boundary 0.6s in:
BASE 6c3d1b0d9 [read] frames 1-5 at t<=0.01s, frame 6 at t=0.61s VERDICT ALL-SIX
HEAD [read] frames 1-5 at t<=0.01s, StopIteration VERDICT CUT-SHORT-AT-5
Confirmed. And the old _flag_n comment named this case verbatim — "useful when the input file is a live capture".
The reviewer offered two remedies: a retry budget/backoff, or narrow the claim. I took the second, deliberately, because the first is worse: the previous behaviour was unbounded by construction — that is the defect — so some stopping rule had to be chosen, and a timed grace period would make the cut-off intermittent rather than absent, which is harder to reason about than a crisp rule and would reintroduce a hang for a slow writer. Following a growing file wants a policy of its own (grace, poll interval, or an explicit follow mode), and that is a feature rather than part of a hang fix.
So: the claim is narrowed in all five places it appeared (both no_eof docstrings, _note_eof_progress, the _flag_n declaration, and both changelogs), and there is now a test — NoEOFOverAGrowingFileStopsAtWhatIsThere — pinning the decision so it stays a decision rather than becoming folklore. It fails on the baseline too, by hanging, so it doubles as another reproduction.
One thing that bounds how much this costs, which the reviewer also found and I verified on both trees: an append landing mid-record raises ValueError: read length must be non-negative or -1 on 6c3d1b0d9 and here. So no_eof over a growing file only ever worked for a writer flushing whole records. Pre-existing, untouched, now recorded.
Required change 2 — my stated mechanism was not supported by measurement
I claimed the in-process deadline fails because the parse path's two except Exception handlers absorb the SIGALRM-raised TimeoutError. The reviewer falsified that properly:
- It tapped the logger hierarchy across the retries and saw zero records from
pcapkit.utilities.decoratorsorpcapkit.protocols.protocol— with a positive control proving the tap worked. Reading the code confirms why:prepareraises end of stream before any next-layer decode, so neither handler is reached. time_limitmostly held — 5/5 standalone, 4/4 across four isolating conditions, and 3/3 on a pytest re-run. The escape I saw is real but not reproducible, and it was specific to the__call__form; the other two forms were bounded bytime_limitin the same run, which directly contradicts "it does not hold for a case that parses a real capture".- The 13.2 GB is better explained by the retry loop emitting tens of thousands of
EOF reachedrecords a second and the runner retaining them — its own log held 463,770 of them — not by handler logging, of which there was none.
Corrected in the test module docstring, both changelogs and the commit message, with the two wrong guesses recorded as ruled out so nobody makes them again. The engineering conclusion is unchanged and, as the reviewer notes, stronger than I argued it: an intermittently delivered in-process deadline against a hang is worse than none, because the run it misses is a wedged suite rather than a red test.
Required change 3 — done
tests/project/test_documentation_claims.py::test_documented_sense_matches_the_code's docstring still described the condition as if self._flag_n: continue, which is no longer what ships — in a file whose entire purpose is stopping documentation from stating untrue things. Corrected, and while there I recorded the reviewer's sharper observation: that AST pin no longer distinguishes the defect from the fix (it passed before and after), so it pins the documented sense of the flag, not that the loop ends. The ending is pinned by tests/foundation/test_extraction_no_eof.py.
Also, its _no_eof_line compares only the single line beginning no_eof:, so "pinned to agree" in my PR body overstated it — the continuation text is not covered by that check, though the reviewer verified it is in fact identical.
The other findings, acted on
_eof_progressedwas a predicate with a hidden write — a laterlogger.debug(f'{self._eof_progressed()}')would silently consume a retry. Renamed to_note_eof_progress, so the name states the mutation, with an explicitWarning:that it is not idempotent.- 3 commits behind
origin/main— both this branch and fix(foundation): close the input stream pcapkit opened, not the caller's (#610) #636 are rebased ontocfb81d3f6and are each exactly 1 commit ahead, 0 behind. pcapkit -verified through the real CLI, which the reviewer did and I had not:cat examples/captures/in.pcap | python -m pcapkit -goes from killed at 25s to exit 0 with a byte-identical 19,641-byte dump. Added to the changelog.- Missing full stops on the two
no_eofparagraphs — left as they are, deliberately: noArgs:entry in either file ends with one, so adding them would be the inconsistency.
Figures after the corrections
HEAD cf0d40461 : tests/foundation/test_extraction_no_eof.py tests/foundation/test_extraction.py
tests/project/ tests/cli/ tests/integration/test_frame_iteration.py
137 passed, 227 warnings in 18.06s exit 0
BASE 6c3d1b0d9 : tests/foundation/test_extraction_no_eof.py (bounded)
12 failed, 1 passed, 1 warning in 181.26s (0:03:01) exit 1
Now 6 behavioural failures on the baseline (five child-bounded non-terminations, plus the TimeoutError on the retry-while-growing case) against 6 API-absence, up from 5 — the growing-file test is the new behavioural one.
Left for a follow-up rather than fixed here
- Following a growing file, which needs a deliberate policy as above.
- The mid-record
ValueError: read length must be non-negative or -1, pre-existing on both trees and not in this PR's file. - The order-dependent
TypeError: type 'ProtocolBase' is not subscriptablefailures intests/project/. Note this reviewer did not see them, in either plugin configuration, where my own runs and fix(foundation): close the input stream pcapkit opened, not the caller's (#610) #636's reviewer did — so the trigger is narrower than "runs after another directory" and it wants its own investigation.
cf0d404 to
6e15a98
Compare
Rewritten: changelog entry moved out, rebased onto current
|
The bullet #639 originally carried, moved here verbatim so that #639 touches only `pcapkit/foundation/extraction.py`, `pcapkit/interface/core.py` and its three test files. Covers: `extract(..., no_eof=True)` never returning, and the progress check that now ends it -- including the deliberate narrowing for a seekable input still being appended to. 43 lines added to the entry file; `CHANGELOG.md` regenerated, not edited.
07270e8 to
c1c8ab6
Compare
Cross-review of
|
| mutant | result |
|---|---|
| baseline (unmodified) | 6/6 pass |
always return False (flag silently a no-op) |
5/6 fail |
always return True (the original #620 bug) |
6/6 fail + 1 error |
position >= previous (off-by-one) |
2/6 fail + 1 error (mock exhausted retrying past the expected stop point) |
The off-by-one variant the brief specifically asked about is caught. except (OSError, ValueError): return False — confirmed via the existing suite's closed stream /
unseekable subtests, and independently: a real os.pipe() wrapped directly in
io.BufferedReader (not yet wrapped in SeekableReader) raises OSError: [Errno 29] Illegal seek on .tell(). In the real Extractor.__init__ flow this raw reader gets
wrapped in SeekableReader before _note_eof_progress ever sees it, and my own pipe
test (below) confirms SeekableReader.tell() does not raise for a live pipe — so
this except clause is a defensive backstop (a seekable stream closed mid-extraction),
not something a live pipe normally hits. False is the safe answer either way: it
errs toward stopping rather than reintroducing the unbounded loop #620 reports.
The pipe-safety claim — independently reproduced with a real pipe, not accepted from the description
Built a real os.pipe() with a background thread that writes 5 frames, sleeps 1.2s,
then writes the rest and closes. Ran the actual Extractor over it with no_eof=True,
instrumented _note_eof_progress itself:
frames: [(1, 0.0), (2, 1.2), (3, 1.21), (4, 1.21), (5, 1.21), (6, 1.21)]
_note_eof_progress call log (time, returned): [(1.214, True), (1.214, False)]
_note_eof_progress was called exactly twice, both after the pause ended — never
during it. That's a direct confirmation that the blocking read absorbs the entire pause
before EOF-detection is ever reached, exactly as the docstring claims. All 6 frames came
back despite the 1.2s pause.
One gap this surfaced: the checked-in suite doesn't have this test.
test_a_pipe_whose_writer_closed_terminates only covers a non-seekable stream that's
already fully written (terminates-on-exhaustion), not a real pause-then-resume with
timing. I could verify the claim myself, but nothing in CI would catch a future
regression to this specific behavior. Worth a follow-up test, not a blocker — the
growing-file narrowing (the actual behavior change) is pinned, with real threading and
a real 0.6s sleep, and it passed in my run.
Test suite run against this branch, exit codes read from file
tests/foundation/test_extraction_no_eof.py tests/foundation/test_extraction.py tests/project/
119 passed, 7 warnings, 499 subtests passed in 17.49s
EXIT:0
No SUBFAIL under any PASSED line in either run.
Test discrimination
tests/project/test_documentation_claims.py::test_documented_sense_matches_the_code
honestly can't and doesn't claim to distinguish the fix from the #620 bug — its own
AST walk looks for if on _flag_n with a continue in the body, which the
pre-fix if self._flag_n: continue also satisfies. It pins the documented sense of
the flag, exactly as its docstring says, and nothing more.
EOFProgressSignal and NoEOFStillRetriesAnInputThatGrows do the real discrimination,
per the mutation table above.
Not independently re-verified
- The exact frame-count numbers in the PR body's own measurements (6 vs. 5 on
6c3d1b0d9for the growing-file case) — I didn't check out the pre-fix tree myself;
the current-tree behavior is verified above and the test passes. - Behavior on Python versions other than 3.14.7 (only interpreter with dependencies
installed here). Nothing in the diff looks version-sensitive to me, but per the
standing caution here, flagging rather than asserting it. resource.RLIMIT_ASin the new test file'sCHILD_PREAMBLEis POSIX-only
(import resourcedoesn't exist on Windows). Harmless today —lint.ymland
unit-tests.ymlboth runubuntu-latestonly — but worth knowing if the matrix ever
grows a Windows job.
Bottom line
No functional defect found. Two minor, non-blocking items: the missing blank line
(E301) left by the rebase resolution, and the missing real-pipe-pause regression test.
Neither changes the verdict.
) * All three loops that handle end of stream -- `record_frames`, `__next__` and `__call__` -- read `if self._flag_n: continue` with nothing else to stop them, so `no_eof=True` suppressed the error that ended the loop without supplying any other ending and an exhausted input was retried forever. * Added `Extractor._note_eof_progress`, which compares the input's position against its position at the previous end of stream. `prepare` restores the position before raising, so two ends of stream at the same position mean nothing arrived between them. Each handler now retries only while the input is still producing. * A pipe is unaffected: a read there blocks while the writer is open but idle rather than reporting end of stream, so a paused pipe never reaches the check. * Deliberate narrowing, documented and pinned by a test: a *seekable* input does not block, so a file still being appended to now ends at the data present when the extraction reached it -- six frames before, five after. The old behaviour was unbounded by construction, which is the defect, so a stopping rule had to be chosen; a timed grace would make the cut-off intermittent rather than absent. * `pcapkit -` hung on any finished stdin as well, which #620 does not mention. * Both docstrings for `no_eof` now describe the retry, its end and its limits. They are pinned to agree by `tests/project/test_documentation_claims.py`, whose own stale description of the condition is corrected here too. The end-to-end tests bound themselves with a child process rather than `tests._support.time_limit`, which proved *intermittent* on this loop: usually on time, once escaping entirely and reaching 13.2 GB RSS. Two guesses at the cause are recorded as ruled out rather than asserted. Measured on 6c3d1b0: 12 failed / 1 passed before, 11 passed after.
c1c8ab6 to
49d9577
Compare
Fixes #620
The defect
extract(..., no_eof=True)never returned. End of stream was detected correctly —ExtractionWarning: EOF reachedfired, as the issue notes — but all three loopsthat handle it read the same thing:
no_eofsuppressed the error that ended the loop without supplying any otherending. The issue reproduced two of the three;
__call__carries its own copy, sothat is covered here too. A
grepfor_flag_nconfirms there is no fourth site.Real line numbers on
6c3d1b0d9, the issue's having shifted:record_framesatextraction.py:698-708,__next__at1017-1030,__call__at1041-1054.Re-verified on
mainFrom an immutable
git archivesnapshot (extraction.pysha256ebe7967079ca…,unchanged afterwards), CPython 3.14.7:
Where the termination condition comes from
The issue asks what "no EOF" should mean when the stream really is exhausted, and
says outright that its own mechanism is inference rather than measurement. So this
was traced.
End of stream originates in one place —
pcapkit/utilities/decorators.py:270,raise StreamEOFError('prepare: end of stream', quiet=True)— reached whenpreparemeasures the bytes remaining and gets zero. It restores the positionbefore raising, so the position at end of stream is stable:
Two consecutive ends of stream at the same position therefore mean nothing arrived
between them.
Extractor._note_eof_progressis that check, and each handler nowreads
if self._flag_n and self._note_eof_progress(): continue.What is safe, and what this narrows
A pipe is unaffected.
pcapkit/__main__.pysetsno_eof = args.fin == '-', sopcapkit -reads a live capture off stdin — and a read on a pipe whose writer isopen but idle blocks; it does not report end of stream. Measured, a pipe fed in
two halves with a 1.5s pause and then closed:
So a paused pipe never reaches the check at all, and the last two lines are a
second bug the issue does not mention:
pcapkit -hung on any finished stdin.Verified through the real CLI —
cat examples/captures/in.pcap | python -m pcapkit -goes from killed at 25s to exit 0 with a byte-identical 19,641-byte dump.
A seekable input is narrowed, deliberately. This is the correction: a regular
file does not block, so its two probes fall microseconds apart and a file still
being appended to now ends at the data present when the extraction reached it.
Measured both ways, appending on a record boundary 0.6s in:
6c3d1b0d9That is a behaviour change and it is chosen rather than overlooked. The previous
behaviour was unbounded by construction — it is the defect #620 reports — so some
stopping rule had to exist. A timed grace period was considered and rejected: it
would make the cut-off intermittent rather than absent, reintroducing the hang
for a slow writer while being harder to reason about than a crisp rule. Following a
growing file wants a policy of its own — a grace, a poll interval, an explicit
follow mode — which is a feature rather than part of a hang fix. The decision is
pinned by
NoEOFOverAGrowingFileStopsAtWhatIsThere, which also fails on thebaseline by hanging.
Bounding how much that costs: an append landing mid-record raises
ValueError: read length must be non-negative or -1on both trees, sono_eofover a growing file only ever worked for a writer flushing whole records.
Pre-existing, and not addressed here.
__call__still raisesEOFError, deliberately__call__must return a frame or raise, and once the input is finished there is noframe — so
EOFErroris the only truthful answer. That matches the plain form,which
tests/integration/test_frame_iteration.py:79pins on an extractor builtwithout
no_eof, so nothing in the suite contradicts the new behaviour.Why the tests use a child process, not the suite's own deadline
This is the second correction. The first version claimed
tests/_support.time_limitfails because the parse path's two
except Exceptionhandlers absorb theSIGALRM-raisedTimeoutError. That is wrong, and was falsified by tapping thelogger hierarchy across the retries: zero records from
pcapkit/utilities/decorators.pyorpcapkit/protocols/protocol.py, with apositive control proving the tap worked.
prepareraises end of stream beforeany next-layer decode, so neither handler is entered.
What is actually true is narrower and still decisive: the in-process deadline is
delivered intermittently here. It usually expires on time, and once escaped
entirely, running past ten minutes and reaching 13.2 GB RSS before I killed it.
The escape has not reproduced. The memory is the retry loop emitting tens of
thousands of
EOF reachedrecords a second, which the runner retains — not handlerlogging, of which there is none.
An intermittent guard against a hang is worse than no guard, because the run it
misses is a wedged suite rather than a red test. So the end-to-end cases are
bounded from outside the interpreter: a child process with a
subprocesswall-clock timeout, which cannot be absorbed or missed, plus an
RLIMIT_AScap of2 GiB so a child that does hang cannot hurt the host while it waits to be killed.
The child also asserts which tree it imported — this project is normally installed
editable, and the editable finder will happily serve a different checkout. The
cases driven by a stand-in engine keep
time_limit, which is sound there becauseno parse path is involved.
Evidence
Both trees with
pcapkit.__file__printed into the pytest header.Baseline
6c3d1b0d9— exit code 1, read from a file, bounded, 3:01, no wedge:Being honest about the 12: 6 are behavioural — five child-bounded
non-terminations plus the
TimeoutErroron the retry-while-growing case — and6 are merely
_note_eof_progressnot existing yet, which proves nothing aboutbehaviour. The 6 are the regression test.
This branch — exit code 0:
The retry direction is not merely asserted, it is mutation-tested by the
cross-review:
_note_eof_progress → return False(the flag silently a no-op) fails6 tests;
→ return True(the original bug) fails 9.Coverage cannot show the change:
if self._flag_n:already executed, and it is thebranch that moved. The evidence axis is the case count — 11 cases where there were
none, 6 of which fail behaviourally without the fix.
Notes for the reviewer
tests/foundation/test_extraction.py's_bare_extractorgainsextractor._eof_mark = None. The EOF handlers consult it, so an instance built byhand and driven through them has to carry it.
tests/project/test_documentation_claims.py's docstring described the condition asif self._flag_n: continue, which is no longer what ships, in a file whose purposeis stopping documentation from stating untrue things. Corrected — and note its AST
pin passes both before and after this change, so it pins the documented sense of
the flag rather than that the loop ends.
_note_eof_progressrecords the mark as a side effect, so it is not idempotent— a second call for one end of stream spends the retry. The name says so and the
docstring warns; the three call sites each call it once.
cfb81d3f6; one commit, 0 behind.