Bugfix Race Condition in Notifier List

AuraTrader/AuraTrader.dproj

@@ -4,7 +4,7 @@
         <ProjectVersion>20.3</ProjectVersion>
         <FrameworkType>FMX</FrameworkType>
         <Base>True</Base>
-        <Config Condition="'$(Config)'==''">Debug</Config>
+        <Config Condition="'$(Config)'==''">Release</Config>
         <Platform Condition="'$(Platform)'==''">Win64</Platform>
         <ProjectName Condition="'$(ProjectName)'==''">AuraTrader</ProjectName>
         <TargetedPlatforms>3</TargetedPlatforms>

Src/Myc.Core.Notifier.pas

@@ -7,43 +7,53 @@ uses
     System.SyncObjs;
 
 type
-    // Low-level implementation for thread-safe multicast events.
+    // A thread-safe, low-level list for managing multicast event sinks of type IInterface.
-    // Implemented as a list referencing IInterface instances, with a locking mechanism for thread safety.
+    // It uses a doubly linked list and a spinlock for synchronization.
-    // Utilizes minimal memory.
+    // `Advise` registers a sink and returns a tag for constant-time removal via `Unadvise`.
-    // `Advise` adds an interface and returns a tag for constant-time removal by `Unadvise`.
-    // `UnadviseAll` removes all registered interfaces.
-    // After `Finalize` is called, the list is cleared, and no new interfaces can be added (closed state).
     TMycNotifyList<T: IInterface> = record
         type
-            TTag = Pointer; // Opaque tag used to identify a registered receiver for unsubscription.
+            // Opaque tag used to identify a registered receiver for unsubscription.
+            TTag = Pointer;
 
-            PItem = ^TItem; // Pointer to an internal list item.
+            // Pointer to an internal list item.
-            TItem = record // Internal structure for storing a receiver and list linkage.
+            PItem = ^TItem;
+            // Internal structure for storing a receiver and list linkage.
+            TItem = record
                 Next, Prev: PItem; // Pointers to the next and previous items in the doubly linked list.
                 Receiver: T; // The registered interface instance (the event sink).
             end;
 
     strict private
-        // Bit 0: Lock state (0 = locked, 1 = unlocked).
+        // Head of the linked list. The pointer value itself is repurposed for a spinlock.
-        // Bit 1: Finalized state (0 = not finalized, 1 = finalized).
+        // Bit 0 of the address stores the lock state (0 = locked, 1 = unlocked).
-        // Other bits (if not 0 and bit 0 is 1) can be a direct interface pointer if FList is nil.
+        FList: PItem;
-        FList: PItem; // Head of the linked list for additional receivers beyond the first one.
 
-        class function AllocItem: PItem; static; inline; // Allocates and initializes memory for a new TItem.
+        // Allocates memory for a new TItem.
-        class procedure FreeItem(Item: PItem); static; inline; // Frees memory previously allocated for a TItem.
+        class function AllocItem: PItem; static; inline;
+        // Frees memory previously allocated for a TItem.
+        class procedure FreeItem(Item: PItem); static; inline;
 
     public
-        procedure Create; // Initializes the notification list, preparing it for use.
+        // Initializes the list in an unlocked state.
-        procedure Destroy; // Cleans up all resources, including unadvising all receivers. Assumes no concurrent access.
+        procedure Create;
-        function Advise(const Receiver: T): TTag; // Registers a receiver interface and returns an opaque tag for later unsubscription.
+        // Safely clears all registered receivers and cleans up resources.
+        procedure Destroy;
+        // Registers a receiver interface and returns an opaque tag for later unsubscription.
+        function Advise(const Receiver: T): TTag;
+        // Unregisters a single receiver using its subscription tag.
         procedure Unadvise(Tag: TTag);
-        procedure UnadviseAll; // Unregisters all currently advised receivers.
+        // Unregisters all currently advised receivers.
-        procedure Lock; inline; // Acquires an exclusive lock for thread-safe operations on the list.
+        procedure UnadviseAll;
-        procedure Release; inline; // Releases the previously acquired exclusive lock.
+        // Acquires an exclusive lock for thread-safe operations on the list.
-        function IsLocked: Boolean; inline; // Checks if the list is currently locked by any thread.
+        procedure Lock; inline;
-        procedure Notify(
+        // Releases the previously acquired exclusive lock.
-            Func: TPredicate<T>
+        procedure Release; inline;
-        ); // Iterates through registered receivers and invokes the predicate; removes receiver if predicate returns false.
+        // Checks if the list is currently locked by any thread.
+        function IsLocked: Boolean; inline;
+        // Invokes a predicate for each registered receiver.
+        // If the predicate returns false, the receiver is detached from further notifications
+        // by setting its interface reference to nil. The list item itself is not freed here.
+        procedure Notify(Func: TPredicate<T>);
     end;
 
 implementation
@@ -55,10 +65,7 @@ end;
 
 procedure TMycNotifyList<T>.Destroy;
 begin
-    // Because refcounting is thread-safe, this will always be entered once after all references
+    Lock;
-    // to Self are dropped. No locking needed!
-    //    Assert(not IsLocked);
-    NativeUInt(FList) := NativeUInt(FList) and not 1;
     UnadviseAll;
 end;
 
@@ -173,7 +180,7 @@ end;
 procedure TMycNotifyList<T>.Release;
 begin
     Assert(IsLocked);
-    TInterlocked.Exchange(NativeUint(FList), NativeUint(FList) or 1);
+    TInterlocked.BitTestAndSet(NativeUint(FList), 0);
 end;
 
 procedure TMycNotifyList<T>.Unadvise(Tag: TTag);