Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Jan 21, 2026

The codebase had 70+ occurrences of .lock().unwrap() which panic without context on mutex poisoning, making debugging difficult.

Changes

  • Replace all .lock().unwrap() and .read()/.write().unwrap() in production code with .expect("descriptive message")
  • Use consistent naming: "<field_name> mutex poisoned" (e.g., "environments mutex poisoned")
  • Test files left unchanged—unwrap() is acceptable there

Files Updated (16)

pet-conda, pet-python-utils, pet-poetry, pet-linux-global-python, pet-reporter, pet-pyenv, pet-windows-store, pet-uv, pet-windows-registry, pet-core, pet

Example

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

// After  
let mut environments = self
    .environments
    .lock()
    .expect("environments mutex poisoned");

This follows the issue's Option 1 recommendation as the minimal improvement to provide better debugging context if a thread holding a lock panics.

Original prompt

This section details on the original issue you should resolve

<issue_title>Refactor: Replace .lock().unwrap() with proper error handling or expect()</issue_title>
<issue_description>## 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.</issue_description>

<agent_instructions>Fix the issue. See if the recommendation makes sense and create a fix.</agent_instructions>

Comments on the Issue (you are @copilot in this section)


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Replaced all occurrences of .lock().unwrap() and .read()/.write().unwrap()
in production source files with .expect("meaningful message") to provide
better debugging information if mutex poisoning occurs.

This refactoring improves code robustness by providing clear context about
which mutex was poisoned, making it easier to diagnose issues if a thread
holding a lock panics.

Files updated:
- crates/pet-conda/src/lib.rs
- crates/pet-python-utils/src/cache.rs
- crates/pet-python-utils/src/env.rs
- crates/pet-poetry/src/lib.rs
- crates/pet-linux-global-python/src/lib.rs
- crates/pet-reporter/src/cache.rs
- crates/pet-reporter/src/stdio.rs
- crates/pet-reporter/src/collect.rs
- crates/pet-pyenv/src/lib.rs
- crates/pet-windows-store/src/lib.rs
- crates/pet-uv/src/lib.rs
- crates/pet-windows-registry/src/lib.rs
- crates/pet-core/src/os_environment.rs
- crates/pet/src/jsonrpc.rs
- crates/pet/src/find.rs
- crates/pet/src/lib.rs

Co-authored-by: karthiknadig <3840081+karthiknadig@users.noreply.github.com>
Copilot AI changed the title [WIP] Refactor lock handling with proper error management Replace .lock().unwrap() with .expect() for better mutex poisoning diagnostics Jan 21, 2026
Copilot AI requested a review from karthiknadig January 21, 2026 00:24
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.

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

2 participants