Skip to content

feat: [#164] implement ReentrantMutex<RefCell<UserOutput>> pattern#166

Merged
josecelano merged 5 commits into
mainfrom
164-refactor-useroutput-remove-mutex
Nov 12, 2025
Merged

feat: [#164] implement ReentrantMutex<RefCell<UserOutput>> pattern#166
josecelano merged 5 commits into
mainfrom
164-refactor-useroutput-remove-mutex

Conversation

@josecelano

Copy link
Copy Markdown
Member

Overview

This PR implements the ReentrantMutex<RefCell<UserOutput>> pattern to resolve same-thread deadlock issues described in #164. The implementation allows nested calls to UserOutput methods within the same thread while maintaining thread safety.

Problem Statement

The previous Arc<Mutex<UserOutput>> pattern caused deadlocks when:

  • A command handler acquired the UserOutput mutex
  • Called a step that also needed to acquire the same mutex
  • The second acquisition blocked indefinitely on the same thread

Solution

Replaced Arc<Mutex<UserOutput>> with Arc<ReentrantMutex<RefCell<UserOutput>>> throughout the codebase:

  • ReentrantMutex: Allows same-thread multiple acquisitions (from parking_lot crate)
  • RefCell: Provides runtime borrow checking for interior mutability
  • Arc: Enables sharing across threads

Key Changes

Core Implementation

  • 🔄 Type Pattern: Arc<Mutex<UserOutput>>Arc<ReentrantMutex<RefCell<UserOutput>>>
  • 🔄 Access Pattern: .lock().unwrap().lock().borrow_mut() for mutable operations
  • 📦 Dependency: Added parking_lot = { version = "0.12.3", features = ["std"] }

Updated Components

  • Bootstrap Container (src/bootstrap/container.rs)
  • Application Layer (command handlers, steps, services)
  • Presentation Layer (controllers, dispatch, progress reporting)
  • Documentation Examples (19 doctests updated with correct type patterns)

Testing

  • Reentrancy Tests: Added comprehensive tests in tests/user_output_reentrancy.rs
  • Unit Tests: 1,196 tests passing
  • Integration Tests: 30 tests passing
  • Documentation Tests: 271 tests passing (all doctest examples fixed)
  • E2E Tests: Both provision/destroy and configuration test suites passing

Technical Details

Access Patterns

// Before (deadlock-prone)
let output = self.user_output.lock().unwrap();
output.success("Done!");

// After (reentrant-safe)  
let output = self.user_output.lock();
output.borrow_mut().success("Done!");

Type Definitions

// Container provides reentrant-wrapped UserOutput
pub fn user_output(&self) -> Arc<ReentrantMutex<RefCell<UserOutput>>> {
    self.user_output.clone()
}

Architecture Decision

This change is documented in docs/decisions/reentrant-mutex-useroutput-pattern.md with:

  • Problem analysis and root cause identification
  • Solution evaluation and trade-offs
  • Implementation approach and patterns
  • Testing strategy and validation

Quality Validation

All pre-commit checks passed:

  • Dependencies: No unused dependencies (cargo machete)
  • Linting: All linters passed (stable & nightly toolchains)
  • Tests: All test suites passing
  • Documentation: All examples compile and demonstrate correct usage
  • E2E Tests: Infrastructure and configuration validation complete

Breaking Changes

None - This is an internal implementation change that maintains all existing public APIs.

Migration Notes

No migration required for users of the application. The change is transparent to:

  • Command-line interface users
  • External integrations
  • Existing workflows and scripts

Future Considerations

  • Performance: ReentrantMutex has minimal overhead compared to standard Mutex
  • Maintainability: RefCell borrow checking provides runtime safety guarantees
  • Extensibility: Pattern supports future nested UserOutput scenarios

Related Issues

Closes #164

Checklist

  • Implementation complete and tested
  • All tests passing (unit, integration, documentation, E2E)
  • Documentation examples updated and validated
  • Architecture decision recorded
  • Pre-commit checks passed
  • No breaking changes to public APIs
  • Performance impact minimal

Replace Arc<Mutex<UserOutput>> with Arc<ReentrantMutex<RefCell<UserOutput>>>
throughout codebase to resolve same-thread reentrancy deadlock.

Key changes:
- Add parking_lot dependency for ReentrantMutex support
- Update Container, ExecutionContext, and all controllers
- Convert ProgressReporter to use ReentrantMutex pattern
- Fix test infrastructure compatibility with parking_lot
- Add comprehensive reentrancy verification tests
- Update error handling to use ReentrantMutex pattern
- Add architectural decision record for pattern choice

All core functionality maintained while eliminating deadlock risk.
Tests verify same-thread multiple lock acquisitions work correctly.
@josecelano josecelano requested a review from da2ce7 November 12, 2025 09:07
- Remove unused _user_output variables from all test functions
- Remove unused _working_dir parameter from create_test_context function
- Remove unused imports (TestUserOutput and std::path::Path)
- Simplify test setup by removing unnecessary user output instances
…tests

- Add create_test_dependencies() helper function to eliminate code duplication
- Remove repeated UserOutput, RepositoryFactory, and SystemClock creation in tests
- Improve test maintainability and readability
- All 4 test functions now use the centralized helper function
- Add comprehensive documentation for the helper function
- Systematic analysis of all test modules in src/presentation/user_output/core.rs
- Moved 22 unit tests to appropriate struct modules:
  - 6 type-safe wrapper tests → sinks/writers.rs
  - 16 verbosity filter tests → verbosity.rs
- Removed 7 duplicate theme tests from core.rs
- Preserved 8 integration test modules in core.rs (~65+ tests)
- All tests continue to pass, no functionality lost
- Clear separation: unit tests (single struct) in modules, integration tests (multiple structs) in core.rs
- Reduced core.rs from 2396 to 2282 lines while maintaining comprehensive test coverage
- Move 29 tests from core.rs to appropriate module files:
  - 3 tests → progress.rs (ProgressMessage tests)
  - 1 test → success.rs (SuccessMessage tests)
  - 1 test → error.rs (ErrorMessage tests)
  - 2 tests → result.rs (ResultMessage tests)
  - 1 test → warning.rs (WarningMessage tests)
  - 1 test → channel.rs (Channel enum tests)
  - 10 tests → steps.rs (StepsMessage tests)
  - 9 tests → info_block.rs (InfoBlockMessage tests)
  - 2 tests → composite.rs (CompositeSink tests)
  - 1 test → telemetry.rs (TelemetrySink tests)

- Remove all duplicate tests from core.rs (350+ lines reduction)
- Improve test organization by placing tests with their related types
- Maintain 100% test coverage with all 137 user_output tests passing
- Clean up module structure for better maintainability
@josecelano josecelano self-assigned this Nov 12, 2025
@josecelano josecelano marked this pull request as ready for review November 12, 2025 11:16
@josecelano

Copy link
Copy Markdown
Member Author

ACK f071644

@josecelano josecelano merged commit f4d9804 into main Nov 12, 2025
34 checks passed
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 UserOutput to remove Arc<Mutex<>> pattern

1 participant