chore: Define and use impl From<PoisonError> for Error - #3312
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3312 +/- ##
==========================================
+ Coverage 90.30% 90.33% +0.03%
==========================================
Files 251 250 -1
Lines 89039 89036 -3
Branches 89039 89036 -3
==========================================
+ Hits 80404 80432 +28
+ Misses 5704 5668 -36
- Partials 2931 2936 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
This is a clean, well-scoped refactor. The new impl<T> From<std::sync::PoisonError<T>> for Error is sound: it is generic only over the guarded type, matches the concrete PoisonError, and sits alongside the other From impls that exist to make ? work across the crate. There is no coherence or overlap risk, and it transparently covers RwLock poisoning too. Moving poisoned-lock failures out of the over-used Generic bucket into Internal is a reasonable classification. The test-side .unwrap()/.expect() to ? conversions all sit in DeltaResult-returning contexts and preserve assertion strength; the untouched .lock().unwrap() at scan/tests.rs:1698 was correctly left alone because it returns a plain Vec.
No blocking issues.
Non-blocking notes
Nit1. kernel/src/error.rs:548 (the new From<PoisonError<T>> impl). Because both Error and PoisonError are public, this impl is permanent public surface and makes "poison is always Internal" an ambient crate-wide decision that call sites can no longer override without giving up ?. That looks like the intended tradeoff, but the adjacent From<Infallible> impl carries a comment explaining why the conversion exists; a one-line comment here recording the intentional invariant would match that precedent.
Raised by: architecture-reviewer
Suggested fix: add a short doc comment above the impl noting that lock poisoning is deliberately surfaced as Error::Internal crate-wide.
Nit2. kernel/src/error.rs:550. Collapsing every site to the message "poisoned mutex" drops the per-callsite context (for example "poisoned scan-metadata iterator mutex"). internal_error calls Backtrace::capture(), which only yields a usable trace when backtraces are enabled, so the PR description's claim that Internal "anyway grabs a backtrace" does not hold in a default build where the specific site is then unrecoverable. This is a diagnosability tradeoff, not a correctness problem.
Raised by: maintainer-codex-reviewer, maintainer-claude-reviewer
Suggested fix: none required; consider softening the PR description wording about backtraces.
Nit3. ffi/src/scan.rs:498. lock_iter() is now a trivial Ok(self.data.lock()?) wrapper, yet scan_metadata_next_arrow_impl inlines data.data.lock()? while scan_metadata_next still calls lock_iter(). The helper carries the doc comment about concurrent-next blocking, so the two callsites reading differently is slightly confusing.
Raised by: maintainer-codex-reviewer, maintainer-claude-reviewer
Suggested fix: route both callsites through lock_iter(), or drop the helper and inline both.
Summary
A behavior-preserving cleanup that removes repetitive lock-error plumbing via a single, correct From<PoisonError> conversion. No blocking issues. The FFI error-kind change from Generic to Internal is real and observable to C consumers, but it is intentional and already disclosed in the PR description, and no existing FFI test depends on the old kind. Remaining points are minor.
Automated review - workflow run
| } | ||
| } | ||
|
|
||
| impl<T> From<std::sync::PoisonError<T>> for Error { |
There was a problem hiding this comment.
Nit1 This From<PoisonError> impl is permanent public surface (both Error and PoisonError are public), making "poison is always Internal" a crate-wide decision no call site can override without giving up ?. Looks intended, but the adjacent From impl documents why it exists; a one-line comment here would record this invariant as deliberate. Raised by: architecture-reviewer. Suggested fix: add a short doc comment noting lock poisoning is intentionally surfaced as Error::Internal crate-wide.
|
|
||
| impl<T> From<std::sync::PoisonError<T>> for Error { | ||
| fn from(_error: std::sync::PoisonError<T>) -> Self { | ||
| Self::internal_error("poisoned mutex") |
There was a problem hiding this comment.
Nit2 Collapsing all sites to "poisoned mutex" drops per-callsite context. internal_error uses Backtrace::capture(), which only yields a trace when backtraces are enabled, so the PR description's claim that Internal "anyway grabs a backtrace" does not hold in a default build and the specific site becomes unrecoverable. Diagnosability tradeoff, not a correctness issue. Raised by: maintainer-codex-reviewer, maintainer-claude-reviewer. Suggested fix: none required; consider softening the PR description wording.
There was a problem hiding this comment.
flyby: ^ I'm cool with the PoisonError conversion so long as backtraces are captured 👍
| self.data | ||
| .lock() | ||
| .map_err(|_| Error::generic("poisoned scan-metadata iterator mutex")) | ||
| Ok(self.data.lock()?) |
There was a problem hiding this comment.
Nit3 lock_iter() is now a trivial Ok(self.data.lock()?) wrapper, but scan_metadata_next_arrow_impl inlines data.data.lock()? while scan_metadata_next still calls lock_iter(). The helper carries the concurrency doc comment, so the split usage is slightly confusing. Raised by: maintainer-codex-reviewer, maintainer-claude-reviewer. Suggested fix: route both callsites through lock_iter(), or drop the helper and inline both.
Benchmark results: ✅ PassSummary: 🚀 0 · ✅ 8 · ☑️ 3 · 🚧 4 · ❌ 0 Per-benchmark results (15 rows)
Legend: 🚀 ≥1.15x faster · ✅ faster or unchanged · ☑️ ≤1.03x slower · 🚧 1.03x-1.15x slower · ❌ ≥1.15x slower |
3 participants
What changes are proposed in this pull request?
As per title.
The upshot is that code like this:
Can now just use
?normally:This saves a surprising number of LoC across the various callsites, because
fmtlikes to wrap themap_errorcalls across several lines.The main possible controversy of this change is that all error messages become the generic "poisoned mutex", and FFI no longer surfaces poisoned mutex errors as
Error::Generic. The former seems fine, becauseError::Internalanyway grabs a backtrace. The latter is arguably a Good Thing because the generic bucket is horribly over-used.How was this change tested?
Existing unit tests.