Thread protecting a list of objects

Question

I've been working with some multi-threaded applications, and part of this requires thread-protecting objects. I have individual object thread protection down by using the following method:

type
  TMyClass = class(TObject)
  private
    FLock: TRTLCriticalSection;
    FSomeString: String;
    procedure Lock;
    procedure Unlock;
    function GetSomeString: String;
    procedure SetSomeString(Value: String);
  public
    constructor Create;
    destructor Destroy; override;
    property SomeString: String read GetSomeString write SetSomeString;
  end;

implementation

constructor TMyClass.Create;
begin
  InitializeCriticalSection(FLock);
  Lock;
  try
    //Initialize some stuff
  finally
    Unlock;
  end;
end;

destructor TMyClass.Destroy;
begin
  Lock;
  try
    //Finalize some stuff
  finally
    Unlock;
  end;
  DeleteCriticalSection(FLock);
  inherited Destroy;
end;

procedure TMyClass.Lock;
begin
  EnterCriticalSection(FLock);
end;

procedure TMyClass.Unlock;
begin
  LeaveCriticalSection(FLock);
end;

function TMyClass.GetSomeString: String;
begin
  Result:= '';
  Lock;
  try
    Result:= FSomeString;
  finally
    Unlock;
  end;
end;

procedure TMyClass.SetSomeString(Value: String);
begin
  Lock;
  try
    FSomeString:= Value;
  finally
    Unlock;
  end;
end;

However, when I implement a list of objects, I can't figure out how to safely protect each object. I create my object lists like this:

type
  TMyClass = class;
  TMyClasses = class;

  TMyClass = class(TObject)
  private
    FOwner: TMyClasses;
  public
    constructor Create(AOwner: TMyClasses);
    destructor Destroy; override;
  end;

  TMyClasses = class(TObject)
  private
    FItems: TList;
    function GetMyItem(Index: Integer): TMyItem;
  public
    constructor Create;
    destructor Destroy; override;
    procedure Clear;
    function Count: Integer;
    property Items[Index: Integer]: TMyClass read GetMyItem; default;
  end;

implementation

{ TMyClass }

constructor TMyClass.Create(AOwner: TMyClasses);
begin
  FOwner:= AOwner;
  FOwner.FItems.Add(Self);
  //Initialize some stuff...
end;

destructor TMyClass.Destroy;
begin
  //Uninitialize some stuff...
  inherited Destroy;
end;

{ TMyClasses }

constructor TMyClasses.Create;
begin
  FItems:= TList.Create;
end;

destructor TMyClasses.Free;
begin
  Clear;
  FItems.Free;
  inherited Destroy;
end;

procedure TMyClasses.Clear;
begin
  while FItems.Count > 0 do begin
    TMyClass(FItems[0]).Free;
    FItems.Delete(0);
  end;
end;

function TMyClasses.Count: Integer;
begin
  Result:= FItems.Count;
end;

function TMyClasses.GetMyItem(Index: Integer): TMyClass;
begin
  Result:= TMyClass(FItems[Index]);
end;

There are two ways I see doing this, and both ways I don't trust. One way would be to implement a critical section lock in the list object (TMyClasses) and each object within would share this lock (by calling FOwner.Lock; and FOwner.Unlock;. But then two different threads wouldn't even be able to work with two different objects from this list at one time, and would defeat the purpose of multithreading. The second way would be to put another critical section in each individual object of their own, but too many of these is also dangerous, right? How can I protect the list and every object in the list together?

Answer 1

You cannot realistically expect to use the same approach in your list class as you use in the simple class that serializes access to a single object.

For example, your list class has, like so many before it, a Count property, and an indexed Items[] property. I'm going to presume that your threading model allows the list to mutate. Now, suppose you want to write code like this:

for i := 0 to List.Count-1 do
  List[i].Frob;

Suppose that another thread were to mutate the list whilst this loop was running. Well, that would clearly lead to runtime failures. So, we can conclude that the loop above would need to be wrapped with a lock. Which means that thread-safety aspects of the list must be exposed externally. You cannot keep it all internal with the current design.

If you wish to keep the lock internal to the class you'll have to remove the Count and Items[] properties. You could have your list looking like this (with some parts removed):

type
  TThreadsafeList<T> = class
  private
    FList: TList<T>;
    procedure Lock;
    procedure Unlock
  public
    procedure Walk(const Visit: TProc<T>);
  end;
....
procedure TThreadsafeList<T>.Walk(const Visit: TProc<T>);
var
  Item: T;
begin
  Lock;
  try
    for Item in FList do
      Visit(Item);
  finally
    Unlock;
  end;
end;

And now you can replace the loop above with this:

ThreadsafeList.Walk(
  procedure(Item: TMyItemClass)
  begin
    Item.Frob;
  end
);

It's not difficult to extend this concept to allow for your Walk method to support deletion of certain items, as determined by the Visit procedure.

But as you say, quite what you can do with such a list is moot. Shared data is the bane of multi-threading. I suggest you find a way to solve your problem that gives each thread its own private copy of all data that it needs. At which point you need no synchronisation and it's all good.

One final point. There is no single concept of thread safety. What is meant by thread safety varies from context to context. Eric Lippert said it best: What is this thing you call "thread safe"? So anytime you ask a question like this, you should give plenty of detail on your particular use case and threading model.