Extends Locked with MREW capabilities (Delphi 11+)

[gabr42]

Adds TLightweightMREWEx, an extension to TLightweightMREW (Delphi 11+ only), to provide more granular locking mechanisms.

Improves the Locked record by integrating TLightweightMREWEx for enhanced read/write synchronization when available (Delphi 11+). Adds Enter/Leave methods for simplified locking and unlocking.

Adds methods BeginRead, TryBeginRead, EndRead, BeginWrite, TryBeginWrite, EndWrite to Locked (Delphi 11+ only).

Summary by Bito

• This pull request introduces a new feature that enhances the locking mechanism in the Locked record by integrating TLightweightMREWEx, which provides more granular read/write synchronization capabilities for Delphi 11 and above.
• It introduces several new methods, including Enter, Leave, BeginRead, TryBeginRead, EndRead, BeginWrite, TryBeginWrite, and EndWrite, to improve synchronization.
• The documentation has been updated to reflect these changes and provide necessary information for users.
• The changes aim to simplify the locking processes and enhance performance in multi-threaded environments.
• Overall, this pull request introduces new synchronization features and methods in the Locked record, focusing on performance improvements in multi-threaded environments.

[claude]

Pull Request Review: TLightweightMREWEx and Enhanced Locked

Thank you for this substantial enhancement to OtlSync! This PR adds valuable reader-writer lock capabilities with reentrancy support. Here's my detailed review:

✅ Strengths

1. Well-structured implementation: The separation of concerns between TLightweightMREWEx (record), ILightweightMREWEx (interface), and TLightweightMREWExImpl (implementation) is clean and follows good design patterns.

2. Excellent documentation: The changelog is comprehensive and includes a reference to the blog post explaining the implementation rationale.

3. Backward compatibility: The conditional compilation approach ensures older Delphi versions continue to work seamlessly.

4. Consistent API: The new methods follow naming conventions consistent with the existing codebase.

🐛 Potential Bugs and Issues

Critical Issues

1. Race condition in BeginRead (OtlSync.pas:1683-1687)

   procedure TLightweightMREWEx.BeginRead;
   begin
     FRWLock.BeginRead;
     FReadLockCount.Increment;
   end;

   Issue: The read lock count is incremented after acquiring the lock. If BeginRead blocks, and the thread is terminated/interrupted between these operations, the counter won't be incremented but the lock will be held. This could cause IsLocked to return incorrect results.

   Recommendation: Consider if this ordering is intentional for performance reasons, or if it could lead to inconsistencies in lock state tracking.

2. Missing MemoryBarrier in GetLockOwner (OtlSync.pas:1656-1664)

   function TLightweightMREWEx.GetLockOwner: TThreadID;
   begin
     {$IFDEF CPUX64}
     Result := TThreadID(TInterlocked.Read(Int64(FLockOwner)));
     {$ELSE}
     Result := TThreadID(TInterlocked.Read(Integer(FLockOwner)));
     {$ENDIF}
     MemoryBarrier;
   end;

   Issue: The memory barrier is placed after the read. However, TInterlocked.Read already provides acquire semantics on most platforms. The barrier placement suggests defensive programming, but placing it after the read doesn't prevent reordering of operations that come before the read.

   Recommendation: Either remove the barrier (if TInterlocked.Read provides sufficient guarantees) or clarify its purpose with a comment. If the intent is to ensure visibility, the barrier should be before dependent operations.

3. Potential deadlock scenario in BeginWrite (OtlSync.pas:1689-1698)

   procedure TLightweightMREWEx.BeginWrite;
   begin
     if GetLockOwner = TThread.Current.ThreadID then
       FWriteLockCount.Increment
     else begin
       FRWLock.BeginWrite;
       SetLockOwner(TThread.Current.ThreadID);
       FWriteLockCount.Value := 1;
     end;
   end;

   Issue: Between checking GetLockOwner and calling FRWLock.BeginWrite, another thread could acquire and release the write lock, potentially causing SetLockOwner(0) to execute. While unlikely, this could lead to edge cases where the lock owner is incorrectly set.

   Recommendation: This appears to be acceptable given that write locks are mutually exclusive. However, adding a comment explaining why this race is not problematic would be helpful.

Moderate Issues

4. TryBeginRead counter leak on failure (OtlSync.pas:1722-1727)

   function TLightweightMREWEx.TryBeginRead: boolean;
   begin
     Result := FRWLock.TryBeginRead;
     if Result then
       FReadLockCount.Increment;
   end;

   Good: This correctly only increments on success. However, ensure TryBeginRead with timeout (line 1730-1735) has the same pattern - it does ✅.

5. Generic exception in EndWrite (OtlSync.pas:1706-1715)

   if GetLockOwner <> TThread.Current.ThreadID then
     raise Exception.Create('Not an owner');

   Issue: Using generic Exception class. Consider creating a specific exception type like EOmniThreadLockError for better error handling.

   Recommendation: Define a custom exception class or use an existing one from the OTL exception hierarchy.

6. Missing initialization of FRWLock (OtlSync.pas:1676-1681)

   class operator TLightweightMREWEx.Initialize(out dest: TLightweightMREWEx);
   begin
     Dest.SetLockOwner(0);
     Dest.FReadLockCount.Value := 0;
     Dest.FWriteLockCount.Value := 0;
   end;

   Issue: FRWLock (TLightweightMREW) is not explicitly initialized. While record initialization might handle this, it's not explicit.

   Question: Does TLightweightMREW have its own Initialize operator that gets called automatically? If not, this could be a bug.

⚠️ Design Considerations

7. Locked backward compatibility change (OtlSync.pas:459)

   FLock : {$IFDEF OTL_HasLightweightMREW}ILightweightMREWEx{$ELSE}TOmniCS{$ENDIF};

   Concern: This changes Locked<T> from using a critical section to a reader-writer lock on Delphi 11+. While this adds features, it changes the performance characteristics:

   • Benefit: Allows concurrent readers
   • Cost: Interface indirection and heap allocation (reference counting overhead)
   • Behavior change: Acquire now acquires a write lock instead of an exclusive lock

   Recommendation: Document this behavioral change in the PR description and consider whether existing code using Locked<T>.Acquire expects exclusive access or is prepared for the overhead of a write lock.

8. IsLocked implementation (OtlSync.pas:1717-1720)

   function TLightweightMREWEx.IsLocked: boolean;
   begin
     Result := (FReadLockCount.Value > 0) or (FWriteLockCount.Value > 0);
   end;

   Issue: This doesn't use atomic operations or memory barriers. While reads of aligned 32-bit integers are atomic on x86/x64, without proper synchronization, you might see stale values.

   Recommendation: Either document that this is "best effort" or add TInterlocked.Read for both counters.

🔒 Security Concerns

9. No overflow protection: If a thread calls BeginWrite recursively many times (e.g., 2^31 times on 32-bit), FWriteLockCount could overflow, leading to incorrect lock state and potential deadlocks or race conditions.

   Recommendation: Add overflow checks or document the limitation.

🚀 Performance Considerations

10. Interface overhead in Locked: Using ILightweightMREWEx instead of direct record storage adds reference counting overhead and heap allocation. For high-frequency, low-contention scenarios, this might be slower than the original TOmniCS.

11. Memory barriers in Get/SetLockOwner: The double barriers (one in each method) might be overly conservative. Profile to see if they're necessary.

📋 Test Coverage

12. Missing unit tests: I don't see any new tests added for:
    • TLightweightMREWEx reentrant write locks
    • TLightweightMREWEx read/write lock interaction
    • Locked<T>.BeginRead/EndRead functionality
    • Locked<T>.Enter/Leave convenience methods
    • Edge cases (e.g., multiple nested writes, mixed read/write locks)

Recommendation: Add comprehensive unit tests to TestOtlSync1.pas covering:

• Reentrant write locks
• Concurrent readers
• Read-write interaction
• Try* methods with timeouts
• IsLocked behavior
• Exception handling in EndWrite

💡 Code Quality Improvements

13. Missing inline directive on BeginWrite/EndWrite: Methods like BeginRead are inline, but BeginWrite and EndWrite are not (OtlSync.pas:402, 406). This seems inconsistent.

14. Inconsistent use of overload keyword: In the declarations (lines 397, 403, 412, 417, 429, 434), the overload keyword is placed differently:

    function TryBeginRead: boolean; {$IF defined(LINUX) or defined(ANDROID)}overload;{$IFEND} inline;
    function TryBeginWrite: boolean; {$IF defined(LINUX) or defined(ANDROID)}overload;
    function TryBeginWrite(timeout: cardinal): boolean; overload;

    The second TryBeginWrite without timeout is missing inline.

15. Platform-specific code could use comments: The {$IF defined(LINUX) or defined(ANDROID)} sections would benefit from comments explaining why Windows doesn't have timeout support or why it's implemented differently.

✏️ Minor Issues

16. Missing semicolon (OtlSync.pas:3032): Check line 3032 in TLightweightMREWExImpl.TryBeginRead - there might be a missing semicolon based on the diff pattern.

17. Typo in comment: "Readers-writ" in the blog URL (line 45) - though this is the actual URL so it's correct.

🎯 Recommendations Summary

Before merging:

1. ✅ Review the race condition in BeginRead/EndRead counter updates
2. ✅ Clarify or fix the memory barrier usage in GetLockOwner/SetLockOwner
3. ✅ Explicitly initialize FRWLock or document why it's safe
4. ✅ Add unit tests for the new functionality
5. ✅ Consider making BeginWrite/EndWrite inline for consistency
6. ✅ Document the performance trade-offs of the Locked<T> changes
7. ✅ Add overflow protection or document limitations
8. ✅ Fix IsLocked to use proper atomic reads

Consider for future:

• Custom exception types for lock errors
• Performance benchmarks comparing old vs new Locked<T>
• Example code showing the new read/write capabilities

Overall Assessment

This is a well-implemented feature that adds significant value to the library. The code follows established patterns and maintains backward compatibility well. The main concerns are around thread safety edge cases and test coverage. Once the critical issues are addressed and tests are added, this will be a solid addition to OmniThreadLibrary.

Great work! 👍

[claude]

Code Review - PR #208: Extends Locked with MREW capabilities (Delphi 11+)

Summary

This PR adds enhanced read/write locking capabilities to the Locked<T> generic record and introduces TLightweightMREWEx, a reentrant extension of Delphi 11's TLightweightMREW. The implementation is well-structured and addresses a real need for more granular locking, but there are several important issues that should be addressed.

---

Critical Issues

1. Race Condition in TLightweightMREWEx.SetLockOwner (OtlSync.pas:1671)

procedure TLightweightMREWEx.SetLockOwner(value: TThreadID);
begin
  Assert(CAS32(FLockOwner, value, FLockOwner), 'Failed to set new lock owner - check for race conditions!');
end;

Problem: This CAS operation is incorrect. CAS32(FLockOwner, value, FLockOwner) compares FLockOwner with FLockOwner (old value = current value), so it will always succeed unless interrupted. The assertion doesn't actually detect race conditions.

Expected behavior: Should be CAS32(old_expected_value, new_value, FLockOwner) where you explicitly specify what the old value should be (likely 0 when acquiring, or current thread ID when releasing).

Recommendation: Replace with proper interlocked operations:

procedure TLightweightMREWEx.SetLockOwner(value: TThreadID);
begin
  {$IFDEF MSWINDOWS}
  InterlockedExchange(integer(FLockOwner), integer(value));
  {$ELSE}
  TInterlocked.Exchange(FLockOwner, value);
  {$ENDIF}
end;

2. Missing FReadLockCount Reference in Linux/Android Path (OtlSync.pas:1722)

{$IF defined(LINUX) or defined(ANDROID)}
function TLightweightMREWEx.TryBeginRead(timeout: cardinal): boolean;
begin
  Result := FRWLock.TryBeginRead(timeout);
  if Result then
    FReadLockCount.Increment;  // ← FReadLockCount field doesn't exist!
end;
{$IFEND LINUX or ANDROID}

Problem: The code references FReadLockCount, but this field is not declared in TLightweightMREWEx. This will cause compilation errors on Linux/Android platforms.

Impact: Breaking change for non-Windows platforms.

Recommendation: Either remove this line (read locks don't need counting in MREW pattern) or add the field if tracking is needed.

---

Major Issues

3. Breaking Change to Locked<T> Internal Structure

The PR changes Locked<T>.FLock from TOmniCS to ILightweightMREWEx on Delphi 11+. This is a binary compatibility break for any code that:

• Allocates Locked<T> in shared memory
• Serializes Locked<T> structures
• Uses SizeOf(Locked<T>) for memory calculations

Recommendation: Document this as a breaking change in the release notes. Consider whether a major version bump is warranted.

4. Inconsistent Lock Semantics

The old implementation used a critical section (exclusive lock), while the new implementation uses BeginWrite (which is also exclusive but with different performance characteristics). This changes the semantics when mixing Acquire/Release with the new BeginRead/EndRead methods.

Scenario: Thread A calls BeginRead(), Thread B calls Acquire(). Thread B will block waiting for write lock, which is correct. However, the documentation doesn't clearly state that Acquire/Release now acquire a write lock, not just an exclusive lock.

Recommendation: Add clear documentation that Acquire/Release acquire write locks when OTL_HasLightweightMREW is defined.

---

Code Quality Issues

5. Redundant Lock Implementation Check in Locked<T>.Release (OtlSync.pas:1993)

procedure Locked<T>.Release;
begin
  Leave;  // Leave already has the conditional compilation
end;

The Release method just calls Leave, which has the same conditional compilation logic. Consider marking Release as deprecated in favor of the clearer Enter/Leave naming.

6. Inconsistent Error Messages

In TLightweightMREWEx.EndWrite (line 1705):

if GetLockOwner <> TThread.Current.ThreadID then
  raise Exception.Create('Not an owner');

This should use a more specific exception type (e.g., EInvalidOperation) and a more descriptive message (e.g., 'EndWrite called by non-owner thread').

7. DEBUG-Only Lock Count Tracking May Hide Issues

The FLockCount field in Locked<T> is only tracked in DEBUG mode. This means:

• Race conditions in lock usage may only surface in debug builds
• Production code won't catch mismatched lock/unlock calls

Recommendation: Consider making this tracking available in all builds, or at least provide a conditional define to enable it in release builds during testing.

---

Performance Considerations

8. Memory Overhead

Each Locked<T> instance now includes:

• ILightweightMREWEx interface (1 pointer)
• IOmniCounter for debug tracking (1 pointer)
• Plus the interface reference counts and vtable lookups

Old size: sizeof(TOmniCS) ≈ 12-16 bytes
New size: At least 2 pointers + interface objects ≈ 40+ bytes

For applications with many Locked<T> instances, this is a significant increase.

Recommendation: Document this in the migration guide. Consider providing a compiler define to opt back into the old critical section implementation if needed.

9. Interface Method Call Overhead

Every lock/unlock operation now goes through an interface method call rather than a direct call. This adds virtual dispatch overhead.

Mitigation: The performance gain from using SRW locks (when contended) likely outweighs this overhead, but it should be measured in real-world scenarios.

---

Missing Test Coverage

10. No Tests for New Functionality

The PR adds significant new functionality but doesn't include tests for:

• TLightweightMREWEx reentrancy
• Locked<T>.BeginRead/EndRead with concurrent readers
• Locked<T>.Enter/Leave methods
• Mixed usage of old and new APIs
• Thread ownership verification in EndWrite
• Timeout variants on Linux/Android (when FReadLockCount bug is fixed)

Recommendation: Add comprehensive unit tests, especially for:

• Reentrant write locking
• Multiple concurrent readers
• Mixed read/write access patterns
• Error conditions (e.g., calling EndWrite from wrong thread)

---

Documentation Issues

11. Insufficient Documentation of Behavioral Changes

The history notes describe what changed but not:

• When to use BeginRead vs BeginWrite vs Acquire
• Performance implications of the new implementation
• Migration considerations for existing code
• Platform differences (Windows vs Linux/Android)

Recommendation: Add comprehensive documentation, possibly as a separate markdown file or expanded XML comments.

---

Security Considerations

12. Thread ID Reuse

The implementation stores TThreadID for ownership tracking. Thread IDs can be reused by the OS after a thread terminates. If:

1. Thread A acquires write lock
2. Thread A terminates without releasing (abnormal termination)
3. Thread B gets assigned Thread A's old TID
4. Thread B calls BeginWrite

Then Thread B might incorrectly think it owns the lock.

Mitigation: This is an inherent limitation of TID-based ownership and generally acceptable for user-mode locks. However, it should be documented as a limitation.

---

Minor Issues

13. Typo in Comment (OtlSync.pas:54)

"funcionally" → "functionally"

14. Copyright Year Update

Copyright changed from 2020 to 2025 (line 7), but current year is 2024 based on commit timestamp.

15. Missing overload Directive in TryBeginRead

Line 399: function TryBeginRead: boolean; should have {$IF defined(LINUX) or defined(ANDROID)}overload;{$IFEND} for consistency with the implementation.

---

Positive Aspects

✅ Good conditional compilation usage for Delphi 11+ features
✅ Backward compatibility maintained for older Delphi versions
✅ Clear naming for new methods (Enter/Leave, BeginRead/EndRead)
✅ Proper use of inline directives for performance-critical methods
✅ Documentation includes blog post reference for background
✅ Reentrant write lock implementation is useful for real-world scenarios

---

Recommendations Summary

Must Fix Before Merge:

1. Fix the race condition in SetLockOwner (Issue files in the snapshot have wrong names #1)
2. Fix compilation error for Linux/Android (Issue IOmniTaskControl.Terminate does not kill the task after timeout #2)

Should Fix:
3. Document breaking changes clearly (Issue #3)
4. Add unit tests for new functionality (Issue #10)
5. Clarify lock semantics in documentation (Issue #4)

Consider:
6. Provide opt-out mechanism for memory-sensitive applications (Issue #8)
7. Make lock count tracking available in release builds (Issue #7)
8. Use more specific exception types (Issue #6)

---

Overall, this is a valuable addition to OTL, but the critical bugs (especially the Linux/Android compilation error) must be fixed before merging.

[claude]

Pull Request Review: TLightweightMREWEx and Locked Enhancements

Thank you for this contribution! I've conducted a thorough review of PR #208. Below are my findings across code quality, potential issues, performance, and testing.

---

Critical Issues

1. Race Condition in TLightweightMREWEx.BeginWrite (OtlSync.pas:1687-1697)

Severity: HIGH

procedure TLightweightMREWEx.BeginWrite;
begin
  if GetLockOwner = TThread.Current.ThreadID then
    // We are already an owner so no need for locking.
    // If another thread executes BeginWrite at this moment, it would enter
    // the 'else' part below and block in the call to FRWLock.BeginWrite.
    FWriteLockCount.Increment
  else begin
    FRWLock.BeginWrite;
    SetLockOwner(TThread.Current.ThreadID);
    FWriteLockCount.Value := 1;
  end;
end;

Problem: There's a Time-of-Check-Time-of-Use (TOCTOU) race condition between checking the lock owner and acquiring the lock.

Scenario:

• Thread A calls BeginWrite, checks GetLockOwner, sees it's not the owner
• Thread A is preempted before calling FRWLock.BeginWrite
• Thread B calls BeginWrite, acquires the lock, sets owner to Thread B
• Thread A resumes, calls FRWLock.BeginWrite (blocks correctly)
• Thread B calls EndWrite, releases the lock and sets owner to 0
• Thread A acquires lock, sets owner to Thread A (CORRECT so far)
• Thread A calls BeginWrite again (nested), checks owner, increments counter (CORRECT)
• BUT: Thread A's first call set FWriteLockCount.Value := 1, second call incremented it to 2
• Thread A calls EndWrite twice: first decrement makes it 1 (doesn't release), second makes it 0 (releases)
• This is CORRECT behavior

Actually, on deeper analysis, the TOCTOU issue is not as severe as initially thought because:

• The FRWLock.BeginWrite() provides the mutual exclusion
• The owner check is an optimization for nested locks

However, there IS still a subtle issue: Between checking GetLockOwner and calling FRWLock.BeginWrite, if the current thread already owns the lock via a different code path, we could have problems.

Recommendation: Consider using a thread-local flag or ensuring the pattern is documented clearly.

2. Missing overload directive in TryBeginRead (Linux/Android) (OtlSync.pas:1724)

Severity: MEDIUM

{$IF defined(LINUX) or defined(ANDROID)}
function TLightweightMREWEx.TryBeginRead(timeout: cardinal): boolean;
begin
  Result := FRWLock.TryBeginRead(timeout);
  if Result then
    FReadLockCount.Increment;  // <-- FReadLockCount does not exist!
end;

Problem: The code references FReadLockCount which is not declared in the TLightweightMREWEx record. This will cause compilation errors on Linux/Android platforms.

Fix: Remove the non-existent field reference:

function TLightweightMREWEx.TryBeginRead(timeout: cardinal): boolean;
begin
  Result := FRWLock.TryBeginRead(timeout);
end;

3. Inconsistent Locking Semantics Change in Locked (OtlSync.pas:1794-1802)

Severity: MEDIUM

The PR changes Locked<T> from using a critical section (TOmniCS) to using ILightweightMREWEx in write mode for Acquire/Release.

Breaking Change Concerns:

• Performance characteristics change (SRW locks vs Critical Sections)
• Re-entrancy behavior changes (now supports nested write locks)
• This is a behavior change that could affect existing code relying on the old semantics

Recommendation:

• Document this behavioral change clearly in release notes
• Consider whether this should be opt-in rather than automatic
• Ensure all existing uses of Locked<T> are compatible with nested locking

---

Code Quality Issues

4. Incomplete Debug Lock Counting (OtlSync.pas:459-461)

{$IFDEF DEBUG}
FLockCount: IOmniCounter;
{$ENDIF DEBUG}

The debug lock counter is a good idea, but:

• It counts ALL lock acquisitions (read + write) together
• This makes it less useful for debugging reader/writer issues
• Consider separate counters or a more sophisticated tracking mechanism

5. Redundant Wrapper Class (OtlSync.pas:3030-3076)

TLightweightMREWExImpl is essentially a boilerplate wrapper around TLightweightMREWEx.

Question: Why use an interface-based wrapper instead of making the record ref-counted or using it directly? The interface adds:

• Extra allocation
• Indirection overhead
• No apparent value beyond reference counting

If reference counting is needed, consider documenting WHY this pattern was chosen.

---

Performance Considerations

6. Interlocked Operations Overhead (OtlSync.pas:1667-1677)

function TLightweightMREWEx.GetLockOwner: TThreadID;
begin
  {$IFDEF MSWINDOWS}
  Result := InterlockedCompareExchange(integer(FLockOwner), 0, 0);
  {$ELSE}
  Result := TInterlocked.Read(FLockOwner);
  {$ENDIF ~MSWINDOWS}
end;

Observation: Using InterlockedCompareExchange(value, 0, 0) to read a value is clever but adds unnecessary memory barrier overhead on every read check.

Recommendation: On x86/x64, aligned 32/64-bit reads are atomic. Consider:

Result := FLockOwner; // Simple read, sufficient for the check

The interlocked operation is only needed when SETTING the owner.

7. Unnecessary Assertion in SetLockOwner (OtlSync.pas:1679-1682)

procedure TLightweightMREWEx.SetLockOwner(value: TThreadID);
begin
  Assert(CAS32(FLockOwner, value, FLockOwner), 'Failed to set new lock owner...');
end;

Problem: This CAS will ALWAYS fail when value != FLockOwner! The assertion logic seems inverted.

Expected Logic:

procedure TLightweightMREWEx.SetLockOwner(value: TThreadID);
begin
  FLockOwner := value;  // Simple assignment under lock protection
end;

Since SetLockOwner is only called while holding the write lock, no CAS is needed.

---

Design Concerns

8. Missing Documentation for Enter/Leave (OtlSync.pas:484-485)

The new Enter/Leave methods are added without XML documentation comments explaining:

• When to use Enter/Leave vs Acquire/Release vs BeginRead/EndRead
• Return value semantics of Enter
• Thread safety guarantees

9. Inconsistent Naming

• Acquire/Release - Critical section style
• Enter/Leave - Simplified style
• BeginRead/EndRead, BeginWrite/EndWrite - MREW style

Having three different naming patterns for locking in the same type could be confusing. Consider documenting the intended use cases for each.

---

Test Coverage

10. No Tests for New Functionality

The PR adds significant new functionality but includes no test cases for:

• ✗ Nested write lock acquisition/release
• ✗ Read lock while write lock is held
• ✗ Multiple concurrent readers
• ✗ Enter/Leave methods
• ✗ TryBeginRead/TryBeginWrite timeout behavior
• ✗ Thread ID tracking in TLightweightMREWEx
• ✗ Stress testing with mixed read/write operations

Recommendation: Add comprehensive tests, especially for the nested locking behavior which is the key feature of this PR. See unittests/TestOtlSync1.pas for examples.

---

Security Concerns

11. Potential Deadlock with Nested Locks

While the code supports nested write locks, there's no protection against:

• Thread acquires read lock, then tries to acquire write lock → DEADLOCK
• This is a well-known MREW limitation but should be documented

---

Minor Issues

12. Typo in Comment (OtlSync.pas:54-55)

///       - Locked<T>.Access/Release now implement locking with a

Should probably be Acquire/Release not Access/Release.

13. Copyright Year Update

Good: Copyright updated to 2025 (OtlSync.pas:7)

---

Positive Aspects

• Good: Conditional compilation properly isolates Delphi 11+ features
• Good: Version history is well-documented
• Good: Blog reference provides context for the implementation
• Good: Debug assertions help catch misuse during development
• Good: Backward compatible for older Delphi versions

---

Recommendations Summary

Must Fix:

1. Fix FReadLockCount compilation error on Linux/Android (OtlSync.pas:1727)
2. Fix SetLockOwner assertion logic (OtlSync.pas:1679)

Should Fix:
3. Add comprehensive test coverage for new MREW functionality
4. Document the behavioral changes in Locked<T>
5. Add XML documentation for new public methods
6. Review and document the race condition safety in BeginWrite

Consider:
7. Optimize GetLockOwner to avoid unnecessary interlocked operations
8. Provide usage guidelines for when to use each locking method
9. Document deadlock scenarios (read→write upgrade)

---

Conclusion

This PR adds valuable functionality for advanced locking scenarios in Delphi 11+. However, there are critical bugs that will prevent compilation on Linux/Android and logic errors in the lock owner tracking. The PR also needs comprehensive test coverage before merging.

Recommendation: Request Changes - The bugs must be fixed and tests added before this can be safely merged.

---

Would you like me to help create test cases for the new functionality?

[claude]

Code Review - PR #208: Extends Locked with MREW capabilities (Delphi 11+)

Summary

This PR introduces TLightweightMREWEx, an extension to TLightweightMREW that adds support for nested write locks, and integrates it into the Locked<T> record. The implementation provides more granular read/write synchronization for Delphi 11+.

---

Findings

1. Critical Bug: Race Condition in TLightweightMREWEx.GetLockOwner ⚠️

Location: OtlSync.pas:1667-1673

The GetLockOwner method has a platform-dependent implementation issue:

function TLightweightMREWEx.GetLockOwner: TThreadID;
begin
  {$IFDEF MSWINDOWS}
  Result := InterlockedCompareExchange(integer(FLockOwner), 0, 0);
  {$ELSE}
  Result := TInterlocked.Read(FLockOwner);
  {$ENDIF ~MSWINDOWS}
end;

Issue: On Windows, this casts TThreadID to integer. If TThreadID is 64-bit on 64-bit platforms, this truncates the thread ID and can cause incorrect owner detection, leading to:

• Multiple threads thinking they own the write lock
• Deadlocks or data corruption

Recommendation: Use platform-appropriate atomic operations for the full TThreadID width:

function TLightweightMREWEx.GetLockOwner: TThreadID;
begin
  Result := TThreadID(TInterlocked.Read(NativeInt(FLockOwner)));
end;

Similar issue exists in SetLockOwner (line 1675).

---

2. Potential Race Condition in BeginWrite

Location: OtlSync.pas:1690-1700

procedure TLightweightMREWEx.BeginWrite;
begin
  if GetLockOwner = TThread.Current.ThreadID then
    FWriteLockCount.Increment
  else begin
    FRWLock.BeginWrite;
    SetLockOwner(TThread.Current.ThreadID);
    FWriteLockCount.Value := 1;
  end;
end;

Issue: There's a small window between acquiring FRWLock.BeginWrite() and setting the lock owner where another thread could theoretically check GetLockOwner and get 0. However, this is mitigated by the fact that other threads would be blocked in FRWLock.BeginWrite().

Status: Acceptable but worth documenting the assumption that FRWLock.BeginWrite() provides the necessary memory barrier.

---

3. Exception Safety in EndWrite

Location: OtlSync.pas:1709-1717

procedure TLightweightMREWEx.EndWrite;
begin
  if GetLockOwner <> TThread.Current.ThreadID then
    raise Exception.Create('Not an owner');

  if FWriteLockCount.Decrement = 0 then begin
    SetLockOwner(0);
    FRWLock.EndWrite;
  end;
end;

Good: The ownership check prevents misuse.

Minor concern: If FWriteLockCount.Decrement somehow becomes negative due to incorrect use, the lock owner would never be cleared. Consider adding a debug assertion:

Assert(FWriteLockCount.Value >= 0, 'Write lock count is negative');

---

4. Breaking Change in Locked<T> Lock Implementation

Location: OtlSync.pas:1795-1806

The Acquire method now uses BeginWrite on Delphi 11+ instead of critical section:

procedure Locked<T>.Acquire;
begin
  {$IFDEF OTL_HasLightweightMREW}
  FLock.BeginWrite;
  {$ELSE ~OTL_HasLightweightMREW}
  FLock.Acquire;
  {$ENDIF ~OTL_HasLightweightMREW}
end;

Impact: This is a behavioral change:

• Before: Used critical section (non-recursive on older Delphi, potentially recursive on newer)
• After: Uses SRW write lock, which is now nestable thanks to TLightweightMREWEx

Recommendation: Update documentation to clarify that Acquire/Release (and Enter/Leave) now support nested calls on Delphi 11+, and that they acquire exclusive write access.

---

5. Debug Instrumentation Quality 👍

Location: Various in Locked<T>

The addition of FLockCount debug tracking is excellent:

{$IFDEF DEBUG}
FLockCount: IOmniCounter;
{$ENDIF DEBUG}

This helps catch lock misuse during development. Well done!

---

6. API Clarity: Enter vs Acquire

Location: OtlSync.pas:1880-1884

function Locked<T>.Enter: T;
begin
  Acquire;
  Result := FValue;
end;

Question: What's the intended distinction between Enter/Leave and Acquire/Release?

• Both acquire write locks
• Enter returns the value directly, which is convenient but creates ambiguity

Recommendation: Add XML documentation explaining when to use each pair:

• Acquire/Release: Explicit manual locking (backwards compatible)
• Enter/Leave: Convenient alternative that returns value
• BeginRead/EndRead: Read-only access (new, Delphi 11+)
• BeginWrite/EndWrite: Write access (new, Delphi 11+)

---

7. Missing Test Coverage ⚠️

Issue: I found no existing tests for Locked<T> in the test suite, and this PR doesn't add tests for the new MREW functionality.

Recommendation: Add tests covering:

1. Nested write locks: Verify BeginWrite can be called multiple times by the same thread
2. Read/write contention: Multiple readers vs single writer
3. TryBeginRead/TryBeginWrite: Timeout and non-blocking behavior
4. Cross-thread ownership: Verify EndWrite raises exception if called by non-owner
5. Backwards compatibility: Ensure old Acquire/Release pattern still works

---

8. Documentation Improvements Needed

Location: OtlSync.pas:43-56

The history section is updated, but consider adding:

1. Usage examples for the new MREW methods
2. Performance characteristics: When to use read vs write locks
3. Migration guide: How existing code behaves differently on Delphi 11+

---

Performance Considerations

Positive:

• SRW locks are generally faster than critical sections
• Read locks allow concurrent readers (significant performance win for read-heavy scenarios)
• The nested write lock implementation is efficient (no lock re-acquisition)

Concerns:

• The atomic operations in GetLockOwner/SetLockOwner add overhead on every write lock operation
• Consider profiling nested lock scenarios vs. single-lock scenarios

---

Security Concerns

No security issues identified. The ownership checks in EndWrite prevent misuse.

---

Recommendations Summary

Must Fix:

1. Fix GetLockOwner/SetLockOwner for 64-bit thread IDs (critical bug)
2. Add test coverage for new MREW functionality

Should Address:

3. Document API usage patterns (when to use which lock methods)
4. Clarify behavioral change in Acquire/Release for Delphi 11+
5. Add usage examples to documentation

Nice to Have:

6. Add debug assertions for lock count validity
7. Performance benchmarks comparing old vs new implementation

---

Overall Assessment

This is a well-implemented feature that brings valuable read/write locking capabilities to Locked<T>. The code is clean and follows existing patterns. However, there's a critical bug with thread ID handling on 64-bit platforms that must be addressed before merging.

The lack of test coverage is concerning given the complexity of lock implementations. I strongly recommend adding comprehensive tests before merging.

Recommendation: Request changes for the thread ID bug fix and test coverage.

[claude]

Code Review: PR #208 - Extends Locked with MREW capabilities (Delphi 11+)

Summary

This PR introduces TLightweightMREWEx, an extension to Delphi 11's TLightweightMREW that adds support for nested write locks, and integrates it into the Locked<T> record to provide more granular read/write synchronization. Overall, this is a well-structured enhancement with good attention to platform compatibility.

Code Quality & Best Practices

Strengths

1. Good conditional compilation: Proper use of OTL_HasLightweightMREW to maintain backward compatibility with pre-Delphi 11 versions
2. Comprehensive documentation: Excellent version history comments with links to blog posts explaining the design rationale
3. Consistent naming: Follows existing OTL naming conventions
4. Interface abstraction: ILightweightMREWEx provides good abstraction for the lock mechanism
5. Inline methods: Appropriate use of inline for performance-critical lock operations

Code Quality Concerns

1. Critical Race Condition in TLightweightMREWEx.BeginWrite (OtlSync.pas:1698-1707)

if GetLockOwner = TThread.Current.ThreadID then
  FWriteLockCount.Increment
else begin
  FRWLock.BeginWrite;
  SetLockOwner(TThread.Current.ThreadID);
  FWriteLockCount.Value := 1;
end;

Issue: There's a race condition between checking GetLockOwner and calling FRWLock.BeginWrite. If thread A checks the owner (false), then thread B acquires and releases the lock, then thread A continues and calls BeginWrite, the lock count will be incorrect.

Impact: Low probability but could cause subtle deadlocks or incorrect lock states in high-contention scenarios.

Recommendation: The current implementation appears correct upon closer inspection because FRWLock.BeginWrite will block until the lock is acquired, and then the owner is set atomically. However, consider adding a comment explaining this behavior to clarify the logic for future maintainers.

2. Assert in Production Code (OtlSync.pas:1720)

Assert(FWriteLockCount.Value >= 0, 'Write lock count is negative');

Issue: Assertions are typically disabled in release builds, but this checks for a critical invariant violation.

Recommendation: Consider using a runtime check with an exception:

if FWriteLockCount.Value < 0 then
  raise Exception.Create('Write lock count is negative');

3. Missing Validation in EndWrite

The EndWrite method at OtlSync.pas:1715-1725 checks ownership but doesn't validate that the lock was actually acquired (i.e., FWriteLockCount.Value > 0 before decrementing).

Recommendation: Add validation:

if FWriteLockCount.Value <= 0 then
  raise Exception.Create('Attempting to release write lock that was not acquired');

4. Inconsistent Comment Style

OtlSync.pas:482 has a comment // alias for Release but Release calls Leave, not the other way around. The comment is misleading.

Recommendation: Update to: // releases Write SRW lock on Delphi 11+, critical section lock on previous versions

Potential Bugs

1. Lock Count Assertion Issue (OtlSync.pas:1720)

Assert(FWriteLockCount.Value >= 0, 'Write lock count is negative');

This assertion checks >= 0 but should check > 0 because we're about to decrement. If the value is 0, decrementing will make it -1, which is the exact condition we're trying to prevent.

Fix: Change to:

Assert(FWriteLockCount.Value > 0, 'Write lock count is not positive');

2. Potential Memory Ordering Issue

In SetLockOwner and GetLockOwner (OtlSync.pas:1664-1683), on Windows, InterlockedCompareExchange and InterlockedExchange are used, which provide full memory barriers. However, on non-Windows platforms, TInterlocked.Read and TInterlocked.Exchange are used. Verify that these provide the same memory ordering guarantees.

3. BeginWrite/EndWrite Asymmetry Risk

If an exception occurs between FRWLock.BeginWrite() and SetLockOwner() in any of the BeginWrite methods, the lock will be held but the owner won't be set, leading to a deadlock. While unlikely with just two assignments, consider using try-finally or ensuring atomicity.

Performance Considerations

Positive

1. Reduced Overhead: Using SRW locks (TLightweightMREW) instead of critical sections for Locked<T> should provide better performance under contention
2. Nested Lock Optimization: Checking thread ownership before acquiring the lock avoids unnecessary kernel transitions for nested writes

Concerns

1. Atomic Operations Overhead: GetLockOwner uses InterlockedCompareExchange even for reads, which is more expensive than necessary. Consider using a relaxed atomic read or memory barrier pattern if the platform supports it.
2. Debug Lock Counting: The FLockCount tracking in DEBUG builds adds overhead to every lock/unlock operation. Ensure this is acceptable for debug performance.

Security Concerns

1. Exception Safety

The EndWrite method (OtlSync.pas:1718) raises an exception for incorrect ownership. While appropriate, ensure all calling code properly handles these exceptions or uses try-finally blocks to prevent resource leaks.

2. TThreadID Size Assumption

{$IFDEF DEBUG}
Assert(SizeOf(FLockOwner) = 4, 'TThreadID is no longer an integer');
{$ENDIF DEBUG}

This assertion is only in DEBUG mode. If TThreadID changes size in a release build, the interlocked operations will silently corrupt memory.

Recommendation: Make this a compile-time check or a runtime check in all builds.

Test Coverage

Missing Tests

The PR doesn't include unit tests for the new functionality. The existing TestOtlSync1.pas has comprehensive tests for TOmniMREW but no tests for:

1. TLightweightMREWEx nested write locks: Test multiple nested BeginWrite/EndWrite calls
2. TLightweightMREWEx ownership validation: Test that EndWrite from wrong thread raises exception
3. Locked<T>.BeginRead/EndRead: Test concurrent read access
4. Locked<T>.BeginWrite/EndWrite: Test exclusive write access
5. Locked<T>.Enter/Leave: Test basic usage patterns
6. Mixed read/write patterns: Test upgrading from read to write (should fail) and vice versa
7. Exception safety: Test behavior when exceptions occur during locked operations

Recommendations

Add test cases covering:

procedure TestLockedBeginReadWrite; // Basic read/write functionality
procedure TestLockedNestedWrite;     // Nested write locks
procedure TestLockedConcurrentRead;  // Multiple readers
procedure TestLockedExclusiveWrite;  // Exclusive writer
procedure TestLockedOwnership;       // Cross-thread ownership violation
procedure TestLockedEnterLeave;      // Enter/Leave methods

Additional Recommendations

1. Documentation

Consider adding XML documentation comments for the new public methods, especially explaining:

• The difference between Acquire/Release and BeginWrite/EndWrite
• The difference between Enter/Leave and BeginWrite/EndWrite
• When to use read vs. write locks
• That read locks cannot be upgraded to write locks

2. Example Usage

Add examples in comments showing proper usage patterns:

// Example: Read access
var data := locked.BeginRead;
try
  // Use data for read-only operations
finally
  locked.EndRead;
end;

// Example: Write access
var data := locked.BeginWrite;
try
  // Modify data
finally
  locked.EndWrite;
end;

3. API Consistency

The Enter method returns T, but Acquire doesn't. Consider making them consistent or clearly documenting why they differ.

Conclusion

This is a solid enhancement that brings valuable MREW functionality to Locked<T>. The code is well-structured and maintains backward compatibility. However, there are a few bugs that should be addressed (particularly the assertion in EndWrite) and test coverage should be added before merging.

Recommendation: Request changes for the bugs mentioned, especially:

1. Fix the assertion condition in EndWrite (issue files in the snapshot have wrong names #1 in Potential Bugs)
2. Add unit tests for the new functionality
3. Consider runtime validation instead of assertions for critical invariants

After these issues are addressed, this PR will be ready to merge.

---

Review completed with Claude Code

[claude]

Pull Request Review: Extends Locked with MREW capabilities (Delphi 11+)

Thank you for this comprehensive enhancement to the OmniThreadLibrary synchronization primitives. This is a well-structured implementation that brings significant improvements to the Locked<T> generic and introduces the TLightweightMREWEx extension.

Strengths

Code Quality & Design

• Clean conditional compilation: The use of OTL_HasLightweightMREW conditional define is well-organized and properly scoped to Delphi 11+
• Consistent API design: The new methods follow existing naming conventions and patterns in the codebase
• Good documentation: Version history is comprehensive and includes a blog post reference for additional context
• Interface abstraction: The ILightweightMREWEx interface with TLightweightMREWExImpl wrapper provides clean separation and enables reference-counted lifetime management

Functionality

• Nested write lock support: The implementation correctly handles reentrant write locks, a significant improvement over the base TLightweightMREW
• Backward compatibility: Existing code continues to work with the new implementation
• Debug support: The FLockCount tracking in DEBUG mode is a valuable debugging aid

---

Issues & Recommendations

1. CRITICAL: Thread Safety Issue in TLightweightMREWEx.BeginWrite

Location: OtlSync.pas:1704-1709

procedure TLightweightMREWEx.BeginWrite;
begin
  if GetLockOwner = TThread.Current.ThreadID then
    // We are already an owner so no need for locking.
    FWriteLockCount.Increment
  else begin

Issue: There's a race condition between checking GetLockOwner and incrementing FWriteLockCount. If two threads call BeginWrite simultaneously when no lock is held:

1. Thread A reads GetLockOwner (returns 0)
2. Thread B reads GetLockOwner (returns 0)
3. Thread A calls FRWLock.BeginWrite and sets owner
4. Thread B also calls FRWLock.BeginWrite (blocks)
5. Thread A completes and releases lock
6. Thread B acquires lock and sets owner

This scenario works, but consider this edge case:

1. Thread A holds the lock
2. Thread B starts checking GetLockOwner
3. Thread A releases the lock (sets owner to 0) between Thread B's check and the else block
4. Thread B incorrectly enters the else path

Recommendation: The current implementation may be acceptable given that TLightweightMREW.BeginWrite will provide the necessary synchronization, but adding a comment explaining this subtle ordering would improve code clarity.

2. Potential Issue: Missing Validation in TLightweightMREWEx.EndWrite

Location: OtlSync.pas:1719-1727

procedure TLightweightMREWEx.EndWrite;
begin
  if GetLockOwner <> TThread.Current.ThreadID then
    raise Exception.Create('Not an owner');

  if FWriteLockCount.Value <= 0 then
    raise Exception.Create('Attempting to release write lock that was not acquired');

Issue: The exception types are generic Exception rather than more specific exception classes.

Recommendation: Consider defining custom exception classes (e.g., ESyncError, ELockOwnershipError) or at least use EInvalidOperation for better exception handling by callers.

3. Performance: Unnecessary Interface Indirection in Locked<T>

Location: OtlSync.pas:459

FLock: {$IFDEF OTL_HasLightweightMREW}ILightweightMREWEx{$ELSE}TOmniCS{$ENDIF};

Issue: Using an interface adds heap allocation and reference counting overhead for every Locked<T> instance. Since TLightweightMREWEx is a record, it could be used directly.

Recommendation: Consider changing FLock to be a TLightweightMREWEx record directly instead of wrapping it in an interface. This would eliminate heap allocation and improve performance. The interface wrapper could still be provided separately if needed for other use cases.

Example:

FLock: {$IFDEF OTL_HasLightweightMREW}TLightweightMREWEx{$ELSE}TOmniCS{$ENDIF};

This would require removing the TLightweightMREWExImpl wrapper usage in Locked<T>.

4. API Design: Inconsistent Locking Semantics

Location: OtlSync.pas:483-485

procedure Acquire; inline;   // acquires Write SRW lock on Delphi 11+, critical section lock on previous versions
procedure Release; inline;   // releases Write SRW lock on Delphi 11+, critical section lock on previous versions
function  Enter: T; inline;  // acquires Write SRW lock on Delphi 11+, critical section lock on previous versions

Issue: On Delphi 11+, Locked<T> now has both exclusive write-mode methods (Acquire/Release, Enter/Leave, BeginWrite/EndWrite) and reader methods (BeginRead/EndRead). However, the old Acquire/Release always acquire in write mode, which may be overkill for read-only operations.

Impact: Code that was using Acquire/Release for read-only access will continue to work but won't benefit from the MREW optimization.

Recommendation: Consider adding migration guidance or examples in documentation showing when to use BeginRead vs BeginWrite. The current implementation is correct, but documentation could help users optimize their code.

5. Code Clarity: AssertLocked Limitation

Location: OtlSync.pas:1817-1824

procedure Locked<T>.AssertLocked;
begin
  // This is just a debugging helper. It catches most bad cases of accessing
  // Locked<T>.Value while Locked<T> is not locked. It may fail (not detect a problem)
  // in multithreading code where one thread may lock the Locked<T> and
  // then another thread tries to access Locked<T>.Value.
  {$IFDEF DEBUG}
  Assert(FLockCount.Value > 0, 'Locked<T> is not locked!');
  {$ENDIF DEBUG}
end;

Issue: The DEBUG counter doesn't track which thread holds the lock, so the assertion can't detect cross-thread violations in read-lock scenarios (where multiple threads can hold read locks simultaneously).

Recommendation: This is acceptable as documented, but consider noting in comments that this limitation is inherent to supporting both read and write locks. For more rigorous checking, you'd need to track thread IDs, which has performance implications.

6. Minor: Magic Number in Size Assertion

Location: OtlSync.pas:1665

{$IFDEF DEBUG}
Assert(SizeOf(FLockOwner) = 4, 'TThreadID is no longer an integer');
{$ENDIF DEBUG}

Issue: The assertion assumes TThreadID is 4 bytes, which may not hold on all platforms or future Delphi versions.

Recommendation: Use SizeOf(Integer) instead of the magic number 4, or document which platforms this is intended for.

---

Security Considerations

✅ No security issues identified. The locking primitives correctly:

• Prevent race conditions through proper use of atomic operations
• Validate lock ownership before releasing
• Guard against mismatched lock/unlock calls

---

Test Coverage

⚠️ No tests included in this PR

Observations:

• The codebase has existing unit tests in /unittests/TestOtlSync1.pas for TOmniMREW
• No tests are added for the new TLightweightMREWEx functionality
• No tests for the new Locked<T> methods (BeginRead/EndRead/BeginWrite/EndWrite)

Recommendation: Add unit tests covering:

1. Nested write lock acquisition and release
2. Concurrent read locks with write lock attempts
3. Read-to-write lock upgrade scenarios (if supported)
4. Error conditions (e.g., releasing without acquiring, wrong thread releases)
5. Thread safety stress testing similar to existing TestMREWRead and TestMREWWrite tests

Example test structure:

procedure TestOtlSync.TestLightweightMREWExNestedWrite;
procedure TestOtlSync.TestLightweightMREWExConcurrentReads;
procedure TestOtlSync.TestLockedTReadWrite;

---

Performance Considerations

1. Interface overhead: As mentioned above, the interface indirection in Locked<T> adds heap allocation. Consider benchmarking vs. direct record usage.

2. Memory alignment: TLightweightMREWEx contains a TOmniAlignedInt32 which ensures proper alignment for atomic operations—this is good.

3. Lock contention: The nested write lock support is well-implemented with minimal overhead in the reentrant case.

---

Minor Issues

1. Typo in comment (OtlSync.pas:55):

   ///       - Locked<T>.Access/Release now implement locking with a
   ///         SRW lock (when available) in 'write' access mode. This is funcionally

   "funcionally" → "functionally"

2. Inconsistent method declarations (OtlSync.pas:404-410):
   Some methods have {$IFEND} on the same line, others on separate lines. Consider standardizing formatting.

---

Summary

This is a solid enhancement that brings important functionality to OmniThreadLibrary. The implementation is generally well-thought-out with good backward compatibility. The main areas for improvement are:

1. Add unit tests for the new functionality
2. Consider removing interface indirection in Locked<T> for better performance
3. Use specific exception types instead of generic Exception
4. Add documentation/examples for migration from Acquire/Release to BeginRead/BeginWrite

The code is production-ready but would benefit significantly from comprehensive test coverage before merging.

---

Overall Assessment: ✅ Approve with suggestions

The implementation is correct and adds valuable functionality. The suggestions above are primarily about optimization and testing rather than correctness issues.

[bito-code-review]

Bito Review Skipped - No Changes Detected

Bito didn't review this pull request because we did not detect any changes in the pull request to review.