Fix deadlock in BufferedTextWriter caused by nested locking

[thomhurst]

The deadlock occurred due to double synchronization:

1. BufferedTextWriter uses ReaderWriterLockSlim for thread safety
2. Console.Out/Error are SyncTextWriter instances with their own locks

This created a lock ordering deadlock where:

• AutoFlush threads: BufferedTextWriter lock → SyncTextWriter lock
• Test threads: SyncTextWriter lock → BufferedTextWriter lock

Fixed by:

1. Modified BufferedTextWriter to release locks before calling external code
2. Used Console.OpenStandardOutput/Error() to bypass SyncTextWriter wrapper

These changes eliminate the deadlock by preventing nested lock acquisition and ensuring consistent lock ordering.

[claude]

Claude finished @thomhurst's task —— View job

---

PR Review Complete

I've thoroughly analyzed the deadlock fix implementation. Here's my comprehensive review:

✅ Overall Assessment

The deadlock fix is well-implemented and addresses the core issue effectively. The solution demonstrates solid understanding of the lock ordering problem and implements appropriate countermeasures.

---

🎯 Deadlock Fix Analysis

Root Cause Identified Correctly:

• BufferedTextWriter's ReaderWriterLockSlim + Console's SyncTextWriter locks created lock ordering deadlock
• AutoFlush threads: BufferedTextWriter lock → SyncTextWriter lock
• Test threads: SyncTextWriter lock → BufferedTextWriter lock

Solution Strategy:

1. Lock-then-release pattern: Collect work under lock, release lock, then execute external calls
2. Bypass SyncTextWriter: Use Console.OpenStandardOutput/Error() to avoid wrapper locks

---

🔍 Code Quality & Best Practices

Strengths:

• Consistent pattern: All flush methods (Flush(), FlushAsync(), AutoFlush(), Dispose()) use the same lock-then-release approach
• Clear separation: Data collection (under lock) vs. external calls (outside lock)
• Performance-conscious: Batching content reduces I/O operations
• Error handling: AutoFlush properly catches exceptions to prevent crashes
• Modern C#: Good use of ValueTask, collection expressions, proper async patterns

Excellent Implementation Details:

• BufferedTextWriter.cs:285-308: The Flush() method exemplifies the fix - collects content under lock, writes outside lock
• StandardOutConsoleInterceptor.cs:18-23: Smart bypass of SyncTextWriter with clear documentation
• Consistent batching logic prevents excessive I/O calls

---

⚠️ Potential Issues

1. Thread-Local Buffer Limitations

• FlushAllThreadBuffers() only flushes current thread's buffer
• Other thread buffers may remain unflushed until their threads access them
• Impact: Minor - auto-flush timer provides eventual consistency

2. Bounded Batch Processing

// BufferedTextWriter.cs:372
while (_flushQueue.TryDequeue(out var queuedContent) && contentToWrite.Count < 20)

• Arbitrary limit of 20 items per batch
• Impact: Low - prevents excessive memory usage, remaining items processed in next cycle

3. Memory Pressure Handling

• High-volume scenarios could cause queue growth between timer intervals
• Impact: Low - 100ms timer provides regular pressure relief

---

⚡ Performance Considerations

Positive Impacts:

• Reduced lock contention: External calls moved outside critical sections
• I/O batching: Multiple writes combined into fewer system calls
• ThreadLocal buffers: Excellent for multi-threaded scenarios

Minimal Overhead:

• Additional List allocation for batching (negligible compared to I/O savings)
• Queue operations are highly optimized (ConcurrentQueue)

---

🔒 Security Assessment

• No security concerns identified
• Proper disposal patterns maintained
• No sensitive data exposure in error paths
• Thread-safety preserved throughout

---

🧪 Test Coverage Recommendations

Critical Test Scenarios Needed:

1. Concurrent stress test: Multiple threads writing while auto-flush runs
2. Deadlock regression test: Simulate the original deadlock scenario
3. Flush ordering test: Verify content appears in correct sequence
4. Dispose race condition test: Ensure clean shutdown under load

Suggested Test Structure:

[Test]
public async Task BufferedTextWriter_ConcurrentWriteAndFlush_NoDeadlock()
{
    // Simulate original deadlock conditions
    // Multiple threads writing + timer-based flushing
}

---

🎯 Minor Suggestions

1. Consider making batch size configurable rather than hardcoded 20
2. Add metrics/logging for queue depth monitoring in high-volume scenarios
3. Document the lock-ordering strategy in class-level XML docs

---

✅ Conclusion

This is a high-quality fix that:

• ✅ Correctly identifies and resolves the deadlock
• ✅ Maintains thread safety and performance
• ✅ Uses consistent, well-reasoned patterns
• ✅ Follows modern C# best practices
• ✅ Includes proper error handling

Recommendation: APPROVE with consideration for adding the suggested tests to prevent regressions.