Weak reference is dangerous

[vfbb 285]

Weak reference in interfaces is not thread safe and it is dangerous and should be avoided in high availability systems, this is my conclusion after some tests.

 

It is known that the weak attribute should always be avoided due to loss of performance, giving preference to the unsafe attribute whenever we can guarantee the existence of the interface when using the unsafe reference. On the other hand, Weak should be a powerful resource in programming, serving as a guarantee way to know if an interface still exists or not, because we create a strong reference through the weak reference and check if the strong reference is nil or not and then use it, the strong referenced interface.

 

It turns out that in the delphi's interfaces internal system, the ARC part is thread safe, but the object's Weak list is not. After the destruction of an interface, the Weak reference list is insecurely traversed setting nil, opening loopholes to use the Weak reference value in some part of the code after the interface is destroyed and before that Weak reference is set to nil. Although this is not clear in System.pas, we can prove it by testing.

 

Attached is a small stress test project that confirms this issue.

 

In view of this problem, what is the best alternative for the weak reference?

TestWeak.zip

Edited December 10, 2020 by vfbb

[loki5100 7]

Ouuch, this is quite serious and this could probably explain my bug: https://stackoverflow.com/questions/64829275/android-memory-corruption
I just open a report on quality central: https://quality.embarcadero.com/browse/RSP-31858

[FPiette 382]

@vfbb I had a quick look at your sample and I see a lot of threads accessing common data without synchronization. Are you sure that the issue doesn't come from this concurrent access?

[loki5100 7]

@vfbb yes FPiette is right, your code is false 😞

you must do Tmonitor.Enter before modifying a variable that is accessed by several threads ! it's not a bug

[Dalija Prasnikar 1396]

Assignment operation in Delphi are not thread safe. If you have one thread writing and other threads reading, such access must be protected or you will get crashes or buggy behavior. 

 

You can safely write and read 32bit aligned simple types, in terms that such code will not cause any crashing, but such code will also not work as intended, unless you don't care about actual values stored.

 

So, weak references in Delphi are not thread safe. Also even strong interface references in Delphi are not thread safe. So there is no bug here, just bad test case.

 

Excerpt from my book Delphi Event-based and Asynchronous Programming (Part 5. Thread Safety) 

 

If you have multiple threads accessing - reading and writing - the same

reference, one thread can easily interfere with the other. Let's say we have a

shared interface reference `Data`, pointing to a previously created object, one

thread that sets that reference to nil, and another thread that tries to take

another strong reference from that one with an assignment to another variable

`Tmp`. The first thread will execute the `_IntfClear` function that will result in

object destruction. The second thread, trying to grab a strong reference

preventing object destruction, will execute the `_IntfCopy` function: 

 

var

  Data, Tmp: IInterface;

 

  Data := nil; // Thread 1 -> _IntfClear

 

  Tmp := Data; // Thread 2 -> _IntfCopy

 

If the `Source._AddRef` line from the `_IntfCopy` function executes before the

 

call to `IInterface(P)._Release` manages to decrease the reference count to zero

and subsequently calls the object's destructor, all is good. The second thread

will successfully capture another strong reference to the object instance.

However, the first thread can interrupt the second thread at the wrong moment,

just after the `Source <> nil` check was successfully passed, but before

`Source._AddRef` had the chance to increment the object's reference count. In

that case, the first thread will happily destroy the object instance while the

second thread will happily grab a strong reference to an already nuked object, and

you can forget about happily ever after.

 

Note: Besides interface references to objects, the reference counting mechanism

is also used for other types like strings and dynamic arrays. Assignment of

those types is also not thread-safe.

[vfbb 285]

44 minutes ago, Dalija Prasnikar said:

So, weak references in Delphi are not thread safe. Also even strong interface references in Delphi are not thread safe. So there is no bug here, just bad test case. 

Yes! the weak reference is not thread safe. But if you cannot make a strong reference through the weak reference to check if the strong reference is null, as it is not safe, there is no benefit in having it in Delphi, nothing will differentiate you from Unsafe, except for the poor performance.

 

1 hour ago, FPiette said:

@vfbb I had a quick look at your sample and I see a lot of threads accessing common data without synchronization. Are you sure that the issue doesn't come from this concurrent access?

The stress test was really confusing, but it's thread-safe, the only global variables that threads change are booleans.

Basically the test is to prove that this is not thread safe:

 

 

Another user found this problem and explained it in the comments of marco cantu's blog:
https://blog.marcocantu.com/blog/2016-april-weak-unsafe-interface-references.html

Edited December 10, 2020 by vfbb

[Dalija Prasnikar 1396]

You are mixing apples and oranges. 

 

Weak references work fine and have their purpose when used in single thread scenario. Their purpose is not achieving some magical thread safety, but breaking reference cycles.

 

I will say it again, assignment of any non trivial type in Delphi is not thread safe. Never has been. As soon as one thread is writing some data, all access to that data must be protected. There is no way around it.

[Anders Melander 1782]

38 minutes ago, vfbb said:

Yes! the weak reference is not thread safe. But if you cannot make a strong reference through the weak reference to check if the strong reference is null, as it is not safe, there is no benefit in having it in Delphi, nothing will differentiate you from Unsafe, except for the poor performance.

If the weak reference is not thread safe then the the weak-to-strong copy also isn't thread safe.

As far as I can tell there's no way that [weak] references can be implemented thread safe so I don't see a bug here.

[vfbb 285]

35 minutes ago, Anders Melander said:

If the weak reference is not thread safe then the the weak-to-strong copy also isn't thread safe.

As far as I can tell there's no way that [weak] references can be implemented thread safe so I don't see a bug here.

Yes Anders! There is no way to workaround this, the solution is redesign the code to doesn't depend or use weak attribute.

 

46 minutes ago, Dalija Prasnikar said:

Weak references work fine and have their purpose when used in single thread scenario. Their purpose is not achieving some magical thread safety, but breaking reference cycles.

dalija, that's why Unsafe exists.

 

 

if you cannot transform WeakToStrong reference safetly, then you should not use Weak, you need to redesign your code to not use Weak, or if you just need to avoid circular reference, use Unsafe.

 

Don’t use Weak, it is dangerous. This is the title and this is truly, once it is not thread safe.

[Anders Melander 1782]

2 minutes ago, vfbb said:

Don’t use Weak, it is dangerous. This is the title and this is truly, once it is not thread safe.

[weak] is not unsafe if you respect its limitations. Otherwise you might as well ague that strings are unsafe.

[vfbb 285]

2 minutes ago, Anders Melander said:

[weak] is not unsafe if you respect its limitations. Otherwise you might as well ague that strings are unsafe.

what is the scenario that does not fit to use Unsafe, that is really necessary Weak and that is really safe to use Weak? This scenario does not exist

[David Heffernan 2345]

6 minutes ago, Anders Melander said:

[weak] is not unsafe if you respect its limitations. Otherwise you might as well ague that strings are unsafe.

^^^ this

[Dalija Prasnikar 1396]

8 minutes ago, vfbb said:

Yes Anders! There is no way to workaround this, the solution is redesign the code to doesn't depend or use weak attribute.

 

dalija, that's why Unsafe exists.

 

No. While unsafe can also be used to break reference cycles, it cannot be used interchangeably with weak, meaning unsafe used in wrong place can lead to dangling pointers. 

 

8 minutes ago, vfbb said:

if you cannot transform WeakToStrong reference safetly, then you should not use Weak, you need to redesign your code to not use Weak, or if you just need to avoid circular reference, use Unsafe.

 

Don’t use Weak, it is dangerous. This is the title and this is truly, once it is not thread safe.

 

Again, using weak is perfectly fine. If you read my book example again, you will see that you cannot even safely take strong reference from strong reference if original has been written to.

 

So now what? Don't use interfaces at all because they are not thread safe. Don't use strings and dynamic arrays because they are not thread safe?

[Dalija Prasnikar 1396]

13 minutes ago, vfbb said:

what is the scenario that does not fit to use Unsafe, that is really necessary Weak and that is really safe to use Weak? This scenario does not exist

You use weak when you cannot guarantee that weak reference lifespan will be less or equal to original strong reference.

 

For instance, let's pretend that TComponent is truly reference counted class, and that same goes for its descendants TEdit and TMenu. Edit has PopupMenu property that can be set to independent TMenu instance. Currently destroying such TMenu instance would leave dangling pointer in Edit PopupMenu property, but we have notification mechanism that notifies Edit that Menu will be destroyed and that Edit should clear reference to the menu.

 

In imagined reference counting scenario, you could have popup menu marked as weak reference and you would not need any notification mechanism to clear that reference when PopupMenu instance is deleted because weak would zero out (clear) that reference when menu is destroyed. If you use unsafe instead of weak, then you would have dangling pointer in PopouMenu property. 

Edited December 10, 2020 by Dalija Prasnikar

[vfbb 285]

Right, I'm generalizing something that I shouldn't because I only work on multithreaded systems with high availability, and in this scenario, I really won't be able to use Weak.

 

But at least this limitation should be documented, since crashes generated by this limitation are difficult to diagnose. @loki5100 This can be your problem too, once it generate a generic crash like yours.

Edited December 10, 2020 by vfbb

[Dalija Prasnikar 1396]

15 minutes ago, vfbb said:

But at least this limitation should be documented, since crashes generated by this limitation are difficult to diagnose.

Well, there is really nothing to document because nothing is really thread safe in Delphi. Unless you want every single documentation page having "This is not thread safe"  (I am exaggerating a bit, but this is generally true in 99.99%)

[zed 14]

@vfbb How about using a lock? Try this fix:

diff --git a/Unit1.pas b/Unit1.pas
index e0d57a2..a27eef0 100644
--- a/Unit1.pas
+++ b/Unit1.pas
@@ -6,7 +6,7 @@ interface
 uses
   System.SysUtils, System.Types, System.UITypes, System.Classes, System.Variants,
   FMX.Types, FMX.Controls, FMX.Forms, FMX.Graphics, FMX.Dialogs, System.Threading,
-  FMX.Controls.Presentation, FMX.StdCtrls;
+  FMX.Controls.Presentation, FMX.StdCtrls, System.SyncObjs;
 
 type
   ITest = interface
@@ -38,6 +38,7 @@ type
     FThreadsPool: TArray<TThreadPool>;
     FTest: ITest;
     FTestWeak: TArray<TWeakOfTest>;
+    FLock: TCriticalSection;
   public
     { Public declarations }
   end;
@@ -54,6 +55,7 @@ implementation
 
 procedure TForm1.FormCreate(Sender: TObject);
 begin
+  FLock := TCriticalSection.Create;
   FTestControlThreadPool := TThreadPool.Create;
   FTestControlTask := TTask.Run(
     procedure()
@@ -85,17 +87,32 @@ begin
           begin
             while (not FDestroying) and (not FCanceled) do
             begin
-              LTest := Form1.FTestWeak[(Length(Form1.FTestWeak) div 2)-1].FTestWeak; // Use a weak reference that is in the middle of the list of weak references
+              FLock.Acquire;
+              try
+                LTest := Form1.FTestWeak[(Length(Form1.FTestWeak) div 2)-1].FTestWeak; // Use a weak reference that is in the middle of the list of weak references
+              finally
+                FLock.Release;
+              end;
               if Assigned(LTest) then // Test if the strong reference is valid, that is, theoretically verify that the object has not yet been destroyed
               begin
                 LTest.Test; // Just a useless call, because if the reference still exists and the object has been destroyed, this will give an exception
-                LTest := nil; // Removing the strong reference
+                FLock.Acquire;
+                try
+                  LTest := nil; // Removing the strong reference
+                finally
+                  FLock.Release;
+                end;
               end;
             end;
           end, Form1.FThreadsPool[I]);
         end;
         Sleep((Random(20) + 3) * 500); // Wait all tasks run many AddRef / Release simulating a stress test
-        Form1.FTest := nil; // Remove the strong ref to force the object destruction (Note: we don't know what task will destroy, will be the last that remove the strong ref)
+        FLock.Acquire;
+        try
+          Form1.FTest := nil; // Remove the strong ref to force the object destruction (Note: we don't know what task will destroy, will be the last that remove the strong ref)
+        finally
+          FLock.Release;
+        end;
         Sleep((Random(10) + 1) * 200); // Wait some seconds to ensure the object destruction
 
         // Cancel the test, check if have error, free all objects and get ready to repeat the next test loop
@@ -137,6 +154,7 @@ begin
   FTestControlTask.Wait;
   FTestControlTask := nil;
   FTestControlThreadPool.Free;
+  FLock.Free;
 end;
 
 { TTest }

To protect your data you need to use a lock every time you assign value to the LTest and Form1.FTest.

Edited December 10, 2020 by zed