Skip to content

Fix cancelled native StreamReader waiters and preserve buffered data - #107

Merged
egorsmkv merged 5 commits into
masterfrom
fix/stream-reader-cancellation-106
Oct 10, 2026
Merged

egorsmkv merged 5 commits into
masterfrom
fix/stream-reader-cancellation-106

Conversation

@egorsmkv

@egorsmkv egorsmkv commented Oct 10, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #106.

A cancelled native stream read leaves its Future registered in ReadWaiter. Retrying before data arrives raises the concurrent-reader ValueError; later feeds can instead consume and discard bytes for the cancelled Future. Fragmented readexactly() additionally owns bytes in a private accumulator.

This change:

  • releases cancelled waiters through a weak-reference done callback plus cancellation checks before reads and data/EOF/error processing;
  • restores only the initialized exact-read prefix ahead of newer buffered data and reapplies flow control;
  • checks Future identity so an old queued callback cannot clear a newer waiter;
  • preserves active concurrent-read rejection and the immediate buffered-read path.

Adds differential cancellation/timeout/retry/EOF/error tests on asyncio, rsloop and uvloop; a real private Redis server integration test for redis-py 5.3.1 + hiredis (SET/GET, both pipeline modes, connection reuse and PING); a Linux CI job; a reproducible benchmark. Updates an existing regression that incorrectly expected cancelled exact reads to lose bytes.

Actual local validation

Linux x86_64, CPython 3.14.7, nightly-2026-09-25, default-feature release extension, Redis 7.2.5, redis-py 5.3.1, hiredis 3.4.2, uvloop 0.23.0.

  • Source baseline f16e637: zero-timeout retry, TCP reproduction and Redis SET all fail with the reported ValueError. asyncio/uvloop reference scenarios pass.
  • Targeted cancellation + Redis + existing stream tests: 226 passed.
  • Full pytest: 453 passed, 3 skipped, 2 failed, 98 deselected. The two Unix-socket failures are EPERM and also reproduce with the baseline extension.
  • Rust default: 319 passed, 15 failed. All failures are io_uring driver creation rejected with EPERM.
  • Rust all features: 415 passed, 19 failed, likewise blocked io_uring construction (including four filesystem tests).
  • Clippy all targets/all features with -D warnings, rustfmt, Ruff and focused Pyright passed.

The full suites did not pass in this environment. Network tests exercised mio/epoll fallback. Actual io_uring, macOS, Windows, other Python versions and free-threaded Python were not validated locally; CI remains a separate gate.

Performance tradeoff

Same-toolchain release builds, baseline/fix/fix/baseline, CPU-pinned lifecycle benchmark:

  • Ready buffered reads show no observed slowdown; small differences are not claimed as improvements.
  • Pending read rounds cost +35–50% in the synthetic benchmark. Exact/until reads add approximately 0.43–0.50 µs per round; read() uses two pending reads per round and adds 0.84 µs. This is measurable cancellation-check/callback overhead, not a performance optimization.
  • TCP benchmark results changed direction across repetitions; no reliable end-to-end regression or improvement is established.

CI follow-up (2026-10-10)

Merged master at 439d288, preserving the existing PR commits and file removals.

  • Added the 3 missing cancellation profiling hooks and 7 hooks for the reactor/STARTTLS code brought in from master. The existing audit now reports 2076 function definitions audited; 0 missing hooks. No exclusions or coverage thresholds were relaxed.
  • Native test output now goes to regular log files before being printed, avoiding the Windows/MSYS pipe failure seen in run 94's predecessor (the failure pattern is documented in rust-lang/rust#98947). All-feature and isolated-feature tests still preserve nonzero exit status, full logs and artifacts; no tests were removed.
  • Restricted the private reader._transport identity assertion to rsloop. Older asyncio keeps a different internal transport; every backend still checks encrypted round trips, writer replacement, EOF, callback count and shutdown. Reproduced both failing reference cases locally on CPython 3.12.3 before this adjustment; both pass afterward.

Local follow-up validation:

  • Release builds with profile and hotpath-alloc-profile succeeded.
  • Each build: 252 passed, 3 skipped, 1 deselected for profiler/stream/cancellation/TLS tests. Redis tests were skipped because redis-server is unavailable on PATH; the known restricted Unix-socket case was deselected.
  • Cancellation profiling produced a report in both modes.
  • Coverage-parser Rust tests: 3 passed. Profiling tooling tests: 15 passed.
  • All-target/all-feature Clippy with warnings denied, rustfmt, Ruff, focused Pyright and diff whitespace checks passed.
  • Checked both modified shell loops with successful and failing commands: stdout/stderr remain in the logs and exit code 23 remains a failure.

On GitHub Actions, run 94 passed the standard profiling job, Windows native/isolated-feature job, both Rust suites and Redis integration. It exposed the older-asyncio assertion fixed in the final commit. The allocation-profile job was cancelled when that commit started run 95; final CI results are pending.

Add all 10 missing profiling hooks after the merge. Capture native test output to regular files to avoid Windows MSYS pipe aborts while preserving exit status and full logs.
Check private reader/transport identity only for rsloop. Retain encrypted round trips, EOF, writer transport replacement and server shutdown assertions for every backend.
@egorsmkv
egorsmkv merged commit 0fbc3e3 into master Oct 10, 2026
38 checks passed
@egorsmkv
egorsmkv deleted the fix/stream-reader-cancellation-106 branch October 10, 2026 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cancelled StreamReader.read() leaves the reader 'waiting': next read() raises ValueError (breaks redis-py/hiredis)

1 participant