Skip to content

Refactor: Replace .lock().unwrap() with proper error handling or expect() #289

@karthiknadig

Description

@karthiknadig

Summary

The codebase has 50+ occurrences of .lock().unwrap() which will panic if a thread holding the lock panics (causing mutex poisoning). While this may be acceptable for internal tools, better error handling would improve robustness.

Current Pattern

let mut environments = self.environments.lock().unwrap();

Proposed Improvements

Option 1: Use .expect() with meaningful message

let mut environments = self.environments
    .lock()
    .expect("environments mutex poisoned - previous thread panicked");

Option 2: Handle PoisonError gracefully

let mut environments = self.environments
    .lock()
    .unwrap_or_else(|poisoned| {
        log::warn!("Recovering from poisoned mutex");
        poisoned.into_inner()
    });

Option 3: Use parking_lot::Mutex which doesn't poison

// parking_lot::Mutex doesn't have PoisonError
use parking_lot::Mutex;
let mut environments = self.environments.lock();

Files with Most Occurrences

  1. crates/pet-conda/src/lib.rs - ~15 occurrences
  2. crates/pet-python-utils/src/cache.rs - ~10 occurrences
  3. crates/pet-poetry/src/lib.rs - ~8 occurrences
  4. crates/pet-linux-global-python/src/lib.rs - ~5 occurrences
  5. crates/pet-reporter/src/cache.rs - ~5 occurrences

Recommendation

For a JSONRPC server that should be reliable:

  1. At minimum, replace unwrap() with expect("meaningful message") for better debugging
  2. Consider parking_lot crate which has better performance and no poisoning semantics
  3. For critical paths (like the reporter), consider graceful recovery

Priority

Low - Current code works but could be more robust.

Metadata

Metadata

Labels

debtCode quality issues

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions