explicit interface implementation of ICollection in Generic Classes

[MarkPhB]

Hi, :) I have a generally question that bother me. Let me explain it with an example, so that you understand the question. Let's say we have simple TcpSerer-class which a ConnectionList and looks like this

public class TcpServer {

        public List ConnectionList { get; }

        public void start () { }

        public void stop () { }

        private void client_Connected ( TcpClient client ) {
            ConnectionList.Add( client );
        }

        private void client_Disconnected ( TcpClient client ) {
            ConnectionList.Remove( client );
        }
    }

So, as you can see, this class is not thread-safe. Because an exception will thrown when i enumerate the ConnectionList while some Client connect or disconnect. Thats why i need an object to provide a possibility to lock the ConnectionList while i enumerate it. OK so far so good. :) Now i thought just take the SyncRoot-Property from the ICollection Interface implementation. But someone told me, i should not use that Object for synchronization with the list. Because the SyncRoot-Property was a mistake from Microsoft and decoy the developers to use it without thinking about synchronization. :^) But the same one told me, that there is no differenc between the object from the SyncRoot-Property and the object i could provide with a derivation of the List<T>. :doh: Thats why my questions: Q#1 - Is it true that the SyncRoot-Property was a mistake from Microsoft and shoundnt be used for synchronization. ? :confused: Q#2 - If Q#1 is true then - Why ? :confused: Q#3 - What its the recommended pattern to lock / synchronize any collection.:confused: Thanks in advance. P.S.: I asked this question already in the msdn-forum but it doesn't satisfy me. Link: http://forums.msdn.microsoft.com/en-US/netfxbcl/thread/3d760576-7cd2-44d5-936e-969ff5213648/[^]

[Simon P Stevens]

A#1) Yes, SyncRoot was a bit of a balls up from someone at MS. A#2) http://msdn.microsoft.com/en-us/magazine/cc188793.aspx[^]. Basically, IIRC it all boils down to the fact that your lock object should really be private. A#3) Create a private object and use that.

private Object _lockObject = new Object();
.
.
.
lock(_lockObject)
{
...
}

In theory, if your writing a class to wrap a ICollection object, and if that object is contained privately within your class, and you never expose it directly, then your probably OK to use the SyncRoot object, as it will be private. I'm not totally sure on this though.

Simon

[MarkPhB]

thank you for the good answer =) But in the case of the TcpServerClass....I have two possiblilities P#1 - I provide a lock-object with a collection to synchronize with it ( SyncRoot - ;)) or P#2 - I provide a lock-object from the class which wrap the collection ( TcpServer.SyncLock). But for P#2 i would have to do this everytime i use a collection whereat for the P#1 the lock-object would just come with the collection. :) I understand you when you say... lock object should really be private ...but in a development, you should know what you are doing. So, thats why it is not really necessary, i think :)

[Simon P Stevens]

The private lock object should be private to the TcpServer class. Like this:

public class TcpServer {

    **private Object \_lockObject = new Object()                <-- HERE**

    // ConnectionList should be private because 
    // if it's public there is nothing to stop people
    // from outside the class modifying it. You 
    // should instead expose public accessors that 
    // either provide a full deep copy of the list, 
    // or just return specific items from the list.
    **private** List ConnectionList { get; }

    public void start () { }

    public void stop () { }

    private void client\_Connected ( TcpClient client ) {
        **lock(\_lockObject)                                    <-- HERE
        {**
            ConnectionList.Add( client );
        **}**
    }

    private void client\_Disconnected ( TcpClient client ) {
        **lock(\_lockObject)                                    <-- HERE
        {**
            ConnectionList.Remove( client );
        **}**
    }
}

MarkPhB wrote:

I understand you when you say... lock object should really be private ...but in a development, you should know what you are doing. So, thats why it is not really necessary, i think [Smile]

It's not because of what you might do, it's because of what other people might do. If you make the lock object public, nasty horrible programmers who don't like you might write a program that reflects on your assembly, and takes a lock on your public lock object. This will cause your program to hang, or worse. Public lock objects are bad practice.

Simon

[MarkPhB]

Simon Stevens wrote:

nasty horrible programmers who don't like you might write a program that reflects on your assembly, and takes a lock on your public lock object

Well in that case you have no chance. If someone uses reflection then there is no way to protect it. With Reflection someone can do everything, unimportant whether the lockObject is private or public. ;)

Simon Stevens wrote:

Public lock objects are bad practice

OK, but how shall i lock the ConnectionList for an external enumeration ? ...or shall i copy the entire ConnectionList befor i enumerate it ? :^)

[Simon P Stevens]

MarkPhB wrote:

Well in that case you have no chance. If someone uses reflection then there is no way to protect it. With Reflection someone can do everything, unimportant whether the lockObject is private or public. [Wink]

Really. Dam it, how stupid. How can anything ever be secure if someone can just fully reflect all your private stuff and change anything they want. Have I missed the point here? [It seems your right: You have to pass the BindingFlags.NonPublic value! This seems rather dumb]

MarkPhB wrote:

OK, but how shall i lock the ConnectionList for an external enumeration ? ...or shall i copy the entire ConnectionList befor i enumerate it ? [Sniff]

Yes, copying the collectionlist before returning it would be the standard way. We have several classes that wrap collections, and do copies before returning them to maintain integrity.

Simon