Making a List thread safe

[chris128]

Bonjour

I am using WCF to make a chat application - I dont think the fact I'm using WCF is relevant for this particular question but just thought I would mention it in case there is something special about the way WCF does threading that I dont know about.

So I have my WCF service that runs on a server and a WPF app that acts as the WCF client - each time a new client signs in or out it updates the server WCF service to let it know that it has changed status. The server then updates a list to either add or remove the user, so this list basically represents who is online at any one time. The list is declared in the core server class like so:

Private Shared List(Of ChatUser)

So, I have a method in my server side that is called whenever a user needs to be added to this list and originally I thought I could have some issues because while the server might be looping through this list to find out who is online, another user might have signed out and the list would therefore be modified while the server was looping through it which would cause an exception. So I added the following to the start of the method that removes a user from the list:

SyncLock New Object

and I havent had any problems... but I'm still not totally convinced that this will completely solve the issue. So would creating a Property give me a bit of extra safety? Assuming I always used the property to access the list in my code.
Like this:

Private Shared Property CurrentClientList() As List(Of ChatUser)
        Get
            SyncLock New Object
                Return _CurrentClientList
            End SyncLock
        End Get
        Set(ByVal value As List(Of ChatUser))
            SyncLock New Object
                _CurrentClientList = value
            End SyncLock
        End Set
End Property

Also, am I actually using SyncLock correctly? I mean should I be referring to a shared object that all of the threads can access rather than just doing New Object each time or does it not make a difference?

Thanks
Chris

[Jenner]

You should be using a shared object because by making new, different objects for get and set, you could have threads getting data at the same time another thread is setting the data. Basically, anywhere that you want one and only one thread at that moment inside the block reading or messing with that variable, you'd use Synclock to that common object.

[chris128]

Ah I see thanks, but other than that its a reasonable idea?

[stanav]

No, you're using it wrong... The lock object needs to be global to ensure that all the methods that entering the synclock block use the same lock object. Right now, your lock object is only a local object, and thus it won't help at all.

Edit: I'm too slow...

[Jenner]

Oh yea, the idea is sound. Using Monitor would also work, and that also works like SyncLock.

[chris128]

OK cool, think I've got the hang of it now but I have a problem - one of the MVPs on the WCF forums told me a while ago that you shouldn't call a WCF service from within a SyncLock block but I am finding that I really need to do that in a lot of places. Does anyone know why he might have suggested this? The only thing I can think of is that maybe if the call to the WCF service (in this case its a client callback) throws an exception then the lock could somehow get messed up... but I cant quite understand how.

[jmcilhinney]

First up, is it really appropriate to have a setter for a List property? Maybe but almost universally the answer would be no. Generally speaking you want callers to be able to add to and remove from the List but you don't actually want callers to be able to replace the List object with another. Normally a property that exposes a List should look like this:

Dim _currentClientList As New List(Of ChatUser)
 
Public ReadOnly Property CurrentClientList()
    Get
        Return Me._currentClientList
    End Get
End Property

or, with lazy loading:

Dim _currentClientList As List(Of ChatUser)
 
Public ReadOnly Property CurrentClientList()
    Get
        If Me._currentClientList Is Nothing Then
            Me._currentClientList = New List(Of ChatUser)
        End If
 
        Return Me._currentClientList
    End Get
End Property

Think about it. Where in the Framework can you actually assign a List object to a property rather than get the existing List from the property and then Add or Remove on it?

Having made that change, synchronising getting the List is pointless. It's actually adding and removing items that needs to be synchronised. Also, the List class implements the ICollection interface and therefore provides its own object for locking. It's an explicit implementation though, so a cast is required, e.g.

SyncLock DirectCast(myList, ICollection).SyncRoot
    'Access myList here.
End SyncLock

[chris128]

I did always wonder about that Setter for List properties but I assumed that got triggered when you called the Add method (or basically any time the list was modified).

I dont agree with your statement about syncing the reading of the list being pointless though - consider the following scenario:
Thread A is looping through the list reading from it, while it is half way through this loop thread B removes an item from the list. Thread A will now throw an exception stating that the collection has been modified. Unless I'm missing something the only way around this is to Sync the reading of the list as well?

[jmcilhinney]

I dont agree with your statement about syncing the reading of the list being pointless though

I didn't say that syncing reading the List was pointless. I said syncing getting the List was pointless. Enumerating, adding to and removing from the List must absolutely be synced. Just keep in mind, though, that reading the list from multiple threads simultaneously is no problem at all. For this reason you shouldn't really put reads in a SyncLock block. Ideally you would use a ReaderWriterLock, which allows multiple readers and no writers, or else one writer and no readers.

[chris128]

Ah I see, thanks for the clarification.
Due to that guy on the MS WCF forums telling me not to make an outbound WCF call from within a synclock I have changed my service so that whenever any code wants to read from the list it calls a function which returns a copy of the list - this function is synclock'ed so that takes care of any threading issues when enumerating or reading from the list. Then I have a method which removes items from the list and a method which adds items to the list, which are both synclock'ed with the same object that the enumerating method is locked with.
Obviously the only problem with this is that because the enumerating function returns a copy of the list, the caller could be working on a slightly out of date list if an item is removed from the list just after it receives the copy but a bit of defensive coding should stop that from causing any problems... although I'm certainly open to better suggestions if you have any