Hi,

I already posted this question already some months ago. But there was no answer/comment to it. So I
come up with it again:

Spring4D collections are not thread safe.

I used Classes.TInterfaceList a lot until now and want to replace
these with TList.  

So my question is, what is the best way to add similar thread saftey as there is in TInterfaceList?

cu Christian

Christian Kaufmann wrote:
> I already posted this question already some months ago. But there was no answer/comment to it. So I come up with it again:

Probably because I just occasionally read here :)

> Spring4D collections are not thread safe.

That is by design. Implementing them thread-safe would cause performance impact even for single threaded usage (as it is the case with TInterfaceList).

> I used Classes.TInterfaceList a lot until now and want to replace these with TList.

That is not the proper replacement if you need thread safety. TInterfaceList is just a wrapper around a TThreadList which provides LockList and UnlockList.
The Spring4D collections are replacement for those from Generics.Collections. However there are no special thread-safe versions of it.

> So my question is, what is the best way to add similar thread saftey as there is in TInterfaceList?

Here is the most simple example:

{code}type
  IThreadSafeList = interface
    ['{048C55BA-3ADD-4DFE-B54F-7E93CD86A9F8}']
    function Add(const item: T): Integer;
  end;

  TThreadSafeList = class(TList, IThreadSafeList)
    function Add(const item: T): Integer; override;
  end;

function TThreadSafeList.Add(const item: T): Integer;
begin
  MonitorEnter(Self);
  try
    Result := inherited Add(item);
  finally
    MonitorExit(Self);
  end;
end;{code}

Also a word about IReadOnlyList that you may find in Spring.Collections.
Though it just allows read access but does not ensure that the underlying list is not changed from another place. Keep that in mind.

>
>IMyCustomer = interface
>   function Orders: IReadOnlyOrders; // Returns a read-only copy of orders
>   procedure AddOrder(const Order : IOrder);
>   procedure DeleteOrder(...);
>   // etc
>end;
>?

My ideas go in this direction. I plan to use a pattern where I always create a copy of the list when
adding / deleting an item:

procedure TMyCustomer.AddOrder(const AOrder: IOrder);
begin
  FOrders := TMyOrders.CreateAdd(FOrders, AOrder);
end;

Thanks for your comments. One main problem will be to provide this information to other programmers
in our team. It's always hard to put all this info in documentation and since programmers are lazy,
they just start to use / copy something without fully understand it. 

So I try to create our library as safe as possible, with many standard patterns. But user requests
tend to be "exceptions" all the time and this causes misuse of existing functionality by
programmers.

cu Christian

> {quote:title=Christian Kaufmann wrote:}{quote}
> >Why are you directly exposing the collections, though? 
> 
> 
> IOrders = interface IList;
> 
> IMyCustomer = interface
>   function Orders: IOrders;
> end;
> 
> Ok, maybe I should use Spring.Collections interfaces only internally / local and protect them with a
> lock there.

Yes, I think so.  Why not something like:

IReadOnlyOrders = IReadOnlyList; // No sure of the read-only name, but you get the idea

IMyCustomer = interface
   function Orders: IReadOnlyOrders; // Returns a read-only copy of orders
   procedure AddOrder(const Order : IOrder);
   procedure DeleteOrder(...);
   // etc
end;
?

As it stands, by exposing the internal Orders, you let any code do random stuff to the internals - sort, remove items (whereas your Delete method might want to fire an event or notification), add things (ditto), etc.  You're exposing the internals externally.

In addition, as you've found, you make it really hard to be threadsafe.  How do you safely add something to Orders?  I suppose you could lock on Orders through a TMonitor or something, but it's messy, and easy to forget to to, and you only need to forget it once to have a bad bug.  Much better to have the synchronization inbuilt into the class that implements IMyCustomer, and provide access methods, so AddOrder, DeleteOrder etc are threadsafe.  Then, if you must enumerate, operate over a read-only version
.  (Actually, another possibility is to enumerate via passing an anonymous method into a ForEach method in IMyCustomer, which is called for each item. Again, the customer class manages safety / locking / etc.)

Cheers,

David

>Why are you directly exposing the collections, though? 


IOrders = interface IList;

IMyCustomer = interface
  function Orders: IOrders;
end;

Ok, maybe I should use Spring.Collections interfaces only internally / local and protect them with a
lock there.

cu Christian

> {quote:title=Christian Kaufmann wrote:}{quote}
> The question for me remains: Should I write "wrapper" classes arround Spring4D to add such
> functionality or should I create subclasses of the Spring4D implementations.
> 
> Maybe I should ask/discuss this in the Spring4D Google group.

Asking there is probably a good idea.

Why are you directly exposing the collections, though? Collections are normally an implementation detail hidden inside a class that exposes, say, Add or Delete methods. You don't usually want other classes to be able to mess around inside a private collection - you provide the API which happens to be backed by a S4D or other collection.

>Try returning a read-only copy and enumerating over that.  Ie, a method in a (say) list that returns a read-only list; the code that needs to enumerate enumerates over that temporary read-only copy.  
Yes, I thought of this as the best solution as well.

The question for me remains: Should I write "wrapper" classes arround Spring4D to add such
functionality or should I create subclasses of the Spring4D implementations.

Maybe I should ask/discuss this in the Spring4D Google group.

cu Christian

> {quote:title=Christian Kaufmann wrote:}{quote}
> >
> >What kind of thread safety do you want?
> 
> I mean safety when accessing from different threads. TInterfaceList is safe regarding Add/Delete
> operations, but the enumerator is not safe.
> 
> The rules I work with are:
> 1) keep data private for each thread
> 2) expose "constants" only.
> 3) shared editable data objects have a TCriticalSection
> 
> So for the third case I'll have to create wrappers or extend the Spring collections I think.

Enumerating is tricky, since you have to ensure that nothing is written to the collection while another thread is enumerating over it. And if the other thread is slow, that can block things badly.  It's very non-granular too, because while locking would typically lock for the shortest amount of time necessary (eg when adding an object, or reading an object) locking while enumerating locks for the time of enumerating the entire thing.

Try returning a read-only copy and enumerating over that.  Ie, a method in a (say) list that returns a read-only list; the code that needs to enumerate enumerates over that temporary read-only copy.  Creating it should be fairly cheap and certainly better than locking for the duration of an enumeration.  It also follows the paradigm of thread safety through immutability: one common approach to thread safety is to avoid locks etc by never mutating state.  Using a read-only copy is a light version of this.



Cheers,

David

>
>What kind of thread safety do you want?

I mean safety when accessing from different threads. TInterfaceList is safe regarding Add/Delete
operations, but the enumerator is not safe.

The rules I work with are:
1) keep data private for each thread
2) expose "constants" only.
3) shared editable data objects have a TCriticalSection

So for the third case I'll have to create wrappers or extend the Spring collections I think.

cu Christian

> {quote:title=Christian Kaufmann wrote:}{quote}
> Hi,
> 
> I already posted this question already some months ago. But there was no answer/comment to it. So I
> come up with it again:
> 
> Spring4D collections are not thread safe.
> 
> I used Classes.TInterfaceList a lot until now and want to replace
> these with TList.  
> 
> So my question is, what is the best way to add similar thread saftey as there is in TInterfaceList?

What kind of thread safety do you want?

S4D classes are threadsafe in that you can use them in other threads just fine, but like most classes aren't designed for use from multiple threads at once.  Are you asking about thread safety when accessing them from multiple threads, eg to add to a collection from several threads at once, or to read from several at once, etc?  If so, the "best" way depends on what you're trying to do.  In general, though, don't expose the collection directly but have a class / methods to control access, and lock so that
 only one thread can read or write at once.

Cheers,

David