This implementation is Thread-safe?

[vfbb 285]

I have a base data class that is used for multithreaded data classess in my project.
I made an implementation to reduce thousands of lines of code, which theoretically should work, but even with some successful tests, I am afraid to release a version to the end user without being sure.

function TipCustomData.GetSafeValue(var AField: string): string;
begin
  Lock; // Enter critical section
  try
    Result := AField;
  finally
    Unlock; // Leave critical section
  end;
end;

procedure TipCustomData.SetSafeValue(var AField: string; const AValue: string);
begin
  BeginUpdate; // Enter critical section
  try
    if AField <> AValue then
    begin
      AField := AValue;
      Change;
    end;
  finally
    EndUpdate; // Leave critical section triggering change event
  end;
end;

In the code above, the most intuitive in these cases would be to use the field pointer, but as "var" parameters internally it is already passed as a pointer (regardless of type), so both implementations should work in the same theoretical way (correct me if i'm wrong). Of course, I only access the parameters within the critical section.

The implementation of my multithreaded data class, descendant of this, would be for example like this:

type
  TipUser = class(TipCustomData)
  private
    FName: string;
    function GetName: string;
    procedure SetName(const AValue: string);
  public
    property Name: string read GetName write SetName;
  end;

function TipUser.GetName: string;
begin
  Result := GetSafeValue(FName);
end;

function TipUser.SetName(const AValue: string);
begin
  Result := SetSafeValue(FName, AValue);
end;

So the question is, is this cleaner implementation that could exist of a multithreaded data class, is it really thread-safe?

 

Sorry for my bad english. 😉

[Georgge Bakh 29]

If I got it right, you protecting with a critical section (the same one I hope) read access to fields as well as field modifications with change event call.

This may be or may be not enough depending on what your program do and what invariants you must ensure.

Additionally, with some logic in change event handler there is a risk of deadlock.

[Lars Fosdal 1793]

Never do locking on a per field basis - unless you have one critical section per field.

Otherwise, always lock the entire object.

As Georgge says: The risk of deadlock is real.  

 

Been there, done that.

 

if ipUser.TryLock
then try
  ipUser.Name := 'New name'
finally
  ipUser.Unlock;
end
else // do a sleep/retry?

 

 

 

[David Heffernan 2353]

55 minutes ago, Lars Fosdal said:

Never do locking on a per field basis - unless you have one critical section per field.

Otherwise, always lock the entire object

Not sure I agree with this sweeping statement. Can you provide a rationale for that statement?

 

I don't think that thread safety can be boiled down to rules like this. 

[Lars Fosdal 1793]

Naturally, it is a simplification and doesn't apply to every context, but we used to have lots of micro locking, and lots of lock failures.  

The problem was multiple threads competing over multiple objects over multiple fields from the same objects for read access. 

Locking per field behaved like a "zipper" between threads.  Think Allocation / Article / Saldo / Location - multiple allocations of the same article at the same location.

Object locking with wait/retry on contention solved that for us. You could argue that MRSW locking would be a better approach, but that is its own can of worms if you need to increase lock level.

 

A more important rule, is do not lock unless you really need to lock.

 

Analyze the way the code works. 

Straight assignments are atomic, unless they involve logic or reallocation.

Will there REALLY be other threads trying to access the same data at the same time? 

Do the data change so frequently and are they accessed so frequently that it is a real problem?

 

[Guest]

19 hours ago, vfbb said:

So the question is, is this cleaner implementation that could exist of a multithreaded data class, is it really thread-safe?

Is this is cleaner ?, yes, 

Are you doing it right ? no

 

You should lock with the same lock so it must be Lock..Unlock or BeginUpdate..EndUpdate for both reading and writing.

 

[Stefan Glienke 2019]

Using a CS for many-read-few-write scenarios might be slow but only you can know and measure.

[David Heffernan 2353]

2 hours ago, Lars Fosdal said:

Naturally, it is a simplification and doesn't apply to every context, but we used to have lots of micro locking, and lots of lock failures.  

That's because your design was broken, not that per field locks are inherently a problem. What matters is the algorithm, and having a deep understanding of the issues. Superficial rules like the one you gave do harm in my view. 

 

2 hours ago, Lars Fosdal said:

Straight assignments are atomic, unless they involve logic or reallocation.

Data needs to be aligned. That's really the key point. 

[Stefan Glienke 2019]

2 hours ago, Lars Fosdal said:

Straight assignments are atomic, unless they involve logic or reallocation.

No, they are not - apart from alignment as David pointed out it depends on the data type - string, interface or dynamic array assignments for example are far from being atomic.

Edited January 27, 2020 by Stefan Glienke

[Lars Fosdal 1793]

At what point is the instance pointer reference for the string, interface or dyn.array changed? 

 

When are field values in instances not aligned (unless overrides have been added)?

 

https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/lock-statement

Quote

Avoid using the same lock object instance for different shared resources, as it might result in deadlock or lock contention.

 

[David Heffernan 2353]

You can't deadlock with a single lock, so long as the lock is recursive. 

 

These blanket statements are not helpful. 

[Lars Fosdal 1793]

Here is another unhelpful blanket statement, based on real-world, empirical evidence.

Multiple threads, multiple objects, cross-thread cross-object access and micro locking per field using the same single lock per object = high risk of deadlock.

 

Sure, you can attribute it to bad design - but perfect designs for software that evolves over decades are really hard to do.

[David Heffernan 2353]

If you use the right techniques then it is perfectly possible to achieve write software that won't deadlock. 

[Stefan Glienke 2019]

1 hour ago, Lars Fosdal said:

At what point is the instance pointer reference for the string, interface or dyn.array changed? 

What? Assignment of these three types consist of multiple operations - just look into _UStrAsg, _IntfCopy or _DynArrayAsg - hence they are not atomic.

[vfbb 285]

5 hours ago, Kas Ob. said:

You should lock with the same lock so it must be Lock..Unlock or BeginUpdate..EndUpdate for both reading and writing.

Yes, the BeginUpdate call Lock internally, and the EndUpdate call Unlock. The same critical section. Just one critical section per object.

 

20 hours ago, Georgge Bakh said:

Additionally, with some logic in change event handler there is a risk of deadlock.

Yes, my Change event is completely asynchronous, similar to the fmx messaging system (subscribe / unsubscribe), but it uses X threads to send these messages from one thread to another.

 

2 hours ago, Lars Fosdal said:

Multiple threads, multiple objects, cross-thread cross-object access and micro locking per field using the same single lock per object = high risk of deadlock.

I don't like the term "big risk" or "low risk" of deadlock. A good design has to be done with the chance of zero deadlock, locking the object just to copy the data to the local var of the thread or else locking only to write local var of the thread in the object (without executing anything inside locking).

 

Example:

procedure TipThread.Execute
var
  LName: string;
  LId: Cardinal;
begin
  // Read copying to local var
  LUser.Lock;
  try
    LName := LUser.Name;
    LId := LUser.Id;
  finally
    LUser.Unlock;
  end;

  // Run your code...

  // Write from local var
  LUser.BeginUpdate;
  try
    LUser.Name := LName;
    LUser.Id := LId;
  finally
    LUser.EndUpdate;
  end;
end;

This will never cause a deadlock.

Edited January 27, 2020 by vfbb

[Dalija Prasnikar 1404]

4 hours ago, Lars Fosdal said:

At what point is the instance pointer reference for the string, interface or dyn.array changed? 

 

 

https://dalijap.blogspot.com/2017/12/is-it-thread-safe-assume-not.html

[Alex7691 7]

8 hours ago, Lars Fosdal said:

At what point is the instance pointer reference for the string, interface or dyn.array changed? 

 

When are field values in instances not aligned (unless overrides have been added)?

 

https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/lock-statement

 

Quote

Avoid using the same lock object instance for different shared resources, as it might result in deadlock or lock contention.

 

Contention yes, deadlock, no. Actually it is quite the contrary. Deadlocks only occur because more than one lock is used (or one which can't be called recursively, like Windows' SRWLock objects). This document is *really* wrong.

Edited January 27, 2020 by Alex7691

[Lars Fosdal 1793]

The deadlock on single lock instance for multiple shared instances (read; individual access locking to multiple fields of a class) is most likely to happens when multiple fields for multiple objects are accessed from multiple threads for the same objects. In our case, it was parallel processing of warehouse picks. F.x. Same article, same location, different picker, needing to update the stock saldo at the same time. It involves interaction between a relatively large number of object instances, such as object cache list retrieval and the objects themselves.

 

I could exemplify in detail, but then I'd only get "but you are doing it wrong" - which I was - and that is why I work hard to minimize the use of implicit locking per field, and rather lock the entire object when needed for as short a time as possible, and avoid locking as far as can be deemed safe.  This is not academically flawless code.  It is code developed by a multitude of people over a long time.  

[David Heffernan 2353]

@Lars Fosdal you can't have a deadlock with only a single recursive lock. I think that was mentioned earlier in the thread. 

[Lars Fosdal 1793]

My apologies for being unclear: One crit sec per object instance, with separate locking per object field. Return to previous description. 

[David Heffernan 2353]

30 minutes ago, Lars Fosdal said:

My apologies for being unclear: One crit sec per object instance, with separate locking per object field. Return to previous description. 

Deadlocks are perfectly possible in this scenario. 

 

The fact that you talk about the likelihood of deadlocks is frightening. I really don't get such a defeatist approach. 

[Anders Melander 1815]

19 minutes ago, David Heffernan said:

Deadlocks are perfectly possible in this scenario.

Indeed; Hierarchical lock schemes are notoriously difficult to get right and most people don't even realize that if you have nested locking then you need to have a hierarchy.

 

37 minutes ago, David Heffernan said:

The fact that you talk about the likelihood of deadlocks

... is pretty much a guarantee that one will happen sooner or later.

[Lars Fosdal 1793]

Defeatist? More like a realist. 

 

After getting rid of the per field micro locking and properly handling contention, the problem was greatly reduced.

Even so - carefully crafted sins appear from time to time.

 

Such is the life of dealing with legacy code. 

[David Heffernan 2353]

Lots of us deal with legacy code. After all, we are Delphi developers.