Improve Thread Safety in Component and Container

[sfloess]

Summary

Code review identified several thread safety issues in Component.java and Container.java that could lead to race conditions in multi-threaded environments.

Critical Issues

1. Component.parent Field Not Protected by Lock

Location: Component.java line 28

Problem:

• parent field is NOT protected by renderLock
• getParent() and setParent() (lines 116-122) access without synchronization
• repaint() (line 228) walks the parent chain without locking

Impact: Race condition - repaint() could read null or stale parent during concurrent setParent() calls from Container.add()/remove().

Fix: Protect parent field access with renderLock or use AtomicReference<Component>.

2. Component.repaint() Walks Parent Chain Unsynchronized

Location: Component.java lines 226-234

Problem: Walks parent chain without any synchronization, creating race with Container.add()/remove() which call setParent().

Impact: Could read null parent or stale parent during tree mutation.

Fix: Acquire renderLock before walking parent chain, or use copy-on-write parent reference.

3. Container.modificationCount Not Volatile

Location: Container.java line 23

Problem:

• modificationCount field is NOT volatile
• Mutations in one thread may not be visible to other threads reading it in detectExternalMutation() (line 106)

Impact: Stale snapshot reuse - cache invalidation might not trigger, causing rendering bugs.

Fix: Make modificationCount volatile or use AtomicInteger.

4. Container.drawBorder() Accesses Width/Height Without Lock

Location: Container.java line 236

Problem:

• Accesses width/height fields directly without acquiring renderLock
• Early return check (line 237) reads dimensions without lock

Impact: TOCTOU (Time-Of-Check-Time-Of-Use) race - dimensions could change between check and buffer writes, causing ArrayIndexOutOfBoundsException.

Fix: Use getWidth()/getHeight() or acquire lock before reading dimensions.

5. MockNcursesBridge.setTerminalSize() Not Synchronized

Location: MockNcursesBridge.java lines 129-159

Problem:

• Does not synchronize access to width/height/screen fields
• Concurrent calls to getTerminalWidth()/getTerminalHeight() could read torn/inconsistent values

Impact: Concurrent resize could cause ArrayIndexOutOfBoundsException if screen buffer is swapped mid-operation.

Fix: Add synchronized modifier to setTerminalSize() and coordinate with other methods.

Lower Priority Issues

6. Component.mouseListeners Not Final

Location: Component.java line 29

Problem: mouseListeners field is protected but not final ArrayList - external code could replace it, breaking thread safety.

Fix: Make private final with defensive copies in add/remove.

7. Component.handleMouseEvent() Complex Lock Pattern

Location: Component.java lines 376-401

Problem: Complex lock/unlock/lock pattern (unlock at 386, lock at 394) could lead to deadlock if listeners acquire locks in different order.

Fix: Review lock acquisition order, consider holding lock for entire method or using try-finally.

8. Component.getBorderChars() Catches Generic Exception

Location: Component.java line 215

Problem: Catches generic Exception which is too broad.

Fix: Catch specific exceptions or let them propagate to avoid masking bugs.

Acceptance Criteria

• All field accesses protected by appropriate locks or made volatile/atomic
• Parent chain traversal synchronized
• modificationCount uses proper memory visibility guarantees
• Dimension reads in drawBorder() synchronized
• MockNcursesBridge.setTerminalSize() synchronized
• No TOCTOU races remain
• All tests pass with thread safety fixes
• Consider adding ThreadSanitizer or stress tests

Related Files

• src/main/java/org/flossware/curses/api/Component.java
• src/main/java/org/flossware/curses/api/Container.java
• src/test/java/org/flossware/curses/testutil/MockNcursesBridge.java

References

• Code review by Opus (score 8.5/10) identified these issues
• Review timestamp: 2026-06-10