Alternative to lock for async operations

Question

Hello i have the following problem:

I have a class Pool that contains a list of Connection-s.The Connection is a wrapper over a socket.I somehow need to create the socket ,ConnectAsync-it ,wrap it into a Connection and return it to the caller.The problem is that i need this this collection to be thread-safe.
Specifically i need the collection to be thread safe when i create a new Connection or when a Connection calls Pool-s Free method.

What alternative to the lock do i have? I have seen so far SemaphoreSlim but i do not understand it.

Pool

   internal partial class Pool    {
            public static Pool MakePool(UserSettings settings)
            {
                return new Pool(settings);
            }

            private List<Connection> liveConnections;

            private readonly object @lock = new object();

            public readonly UserSettings settings;

            public async  Task<Connection> ConnectAsync()
            {
                Connection readyConnection;
                lock(@lock)
                {
                    if (this.liveConnections == null)
                    {
                        this.liveConnections = new List<Connection>(this.settings.MIN);
                    }
                    readyConnection = this.liveConnections.FirstOrDefault(x => !x.IsUsed);
                    if (readyConnection == null)
                    {

                        readyConnection = await CreateConnectionAsync(settings);
                        this.liveConnections.Add(readyConnection);

                    }

                    return readyConnection;

                }
            }
            private async Task<Connection> CreateConnectionAsync(UserSettings settings)
            {
                //Socket initialization
                Socket socket = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
                IPAddress address=IPAddress.Parse(settings.HOST_NAME);
                int port = settings.PORT;
                IPEndPoint point = new IPEndPoint(address, port);
                await socket.ConnectAsync(point);


                ConnectionSettings conSettings = new ConnectionSettings
                {
                    pool = this,
                    ConnectionID = GenerateID(),
                    socket = socket,
                };
                Connection con= Connection.CreateConnection(conSettings);
                return con;

            }
            //this gets called by the connection !!
            internal void Free(string ID)
            {
                lock (@lock)
                {
                    Connection con=this.liveConnections.Find(x => x.ID == ID);
                    con.IsUsed = false;
                }
            }
            private static string GenerateID()=>Guid.NewGuid().ToString();

            private Pool(UserSettings settings)
            {
                this.settings = settings;
            }

        }

Connection

public class Connection :IDisposable
    {
        private PhysicalConnection rawConnection;

        internal  static Connection CreateConnection(ConnectionSettings settings)
        {
            Connection con = new Connection(settings);

            return new Connection(settings);
        }

        public readonly string ID;
        private readonly Pool parentPool;
        public bool IsUsed { get; internal set; }

        public void Dispose()
        {
            this.parentPool.Free(this.ID);
        }



        private Connection(ConnectionSettings settings)
        {
            this.ID = settings.ConnectionID;
            this.parentPool = settings.pool;
            this.rawConnection = new PhysicalConnection(settings.socket);

        }
    }

ConnectionSettings

class ConnectionSettings
    {
        public Pool pool;
        public string ConnectionID;
        public Socket socket;
    }

As you can see the Pool is sent in the Connection constructor so that the Connection can notify the Pool when it is disposed !

If you want a thread-safe collection, why don't you use one that the framework provides like `System.Collections.Concurrent.ConcurrentBag`? — Camilo Terevinto, Jul 20 '18 at 11:47
@BercoviciAdrian Then what should happen to your collection while you're awaiting? Should it stay locked? Or you can unlock it at while awaiting? — rs232, Jul 20 '18 at 11:54
@rs232 Yes it should stay locked.I want the operation of `opening a socket`,`wrapping it in a Connection` and `add Connection to collection` to be atomical. — Bercovici Adrian, Jul 20 '18 at 11:57
@BercoviciAdrian then it seems that you overcomplicate the task by using await. You want to lock the collection, then start an asyncronous operation (probably on another thread) which might take indefinite time and invoke indefinite number of other operations, and to unlock your collection only after that operation completes. This is a very dangerous behaviour as it is so easy to introduce nearly-impossible-to-identify race conditions. The code should be synchronous IMO: lock the collection, push in an item, unlock the collection, enjoy a working code. — rs232, Jul 20 '18 at 12:03
I think you are right.I wanted to do first `ConnectAsync` the `Socket` before wrapping it and adding it to the collection...but i suppose the `Connect` will do just fine. — Bercovici Adrian, Jul 20 '18 at 12:07
Anyway if i use `await ConnectAsync` or `Connect` it is the same thing.The code will behave synchronously nonetheless.The only thing that differs is that in the latter case i get a state machine . — Bercovici Adrian, Jul 20 '18 at 12:10

rs232 · Accepted Answer · 2018-07-20T12:23:20.997

1

It looks like you don't even need to keep your call to CreateConnectionAsync inside the lock:

public async  Task<Connection> ConnectAsync()
{
   Connection readyConnection;
   lock(@lock)
   {
       if (this.liveConnections == null)
       {
           this.liveConnections = new List<Connection>(this.settings.MIN);
       }
       readyConnection = this.liveConnections.FirstOrDefault(x => !x.IsUsed);
   }
   if (readyConnection == null)
   {
       readyConnection = await CreateConnectionAsync(settings);
       lock(@lock)
       {
            this.liveConnections.Add(readyConnection);
       }
   }
   return readyConnection;
}

Your CreateConnectionAsync does not use liveConnections collection at all. At the point you've done with searching through the collection, so it can be unlocked while your new connection asyncronously tries to connect to its endpoint.

edited Jul 20 '18 at 12:23

answered Jul 20 '18 at 12:13

rs232

1,248
8
16

When i create a new `Connection` i must add it to the `liveConnections` list or else what would be the point of `this.liveConnections.FirstOrDefault(x=>!x.IsUsed)`.I must store it in the list to be able to check it at a later time.I updated my code so that you add the new `Connection` to the collection. – Bercovici Adrian Jul 20 '18 at 12:17
1

Oh, sorry, I missed that part from the answer! Now updated. – rs232 Jul 20 '18 at 12:23
So you must call `lock` twice to be able to do it.I supposed it is way too costly. – Bercovici Adrian Jul 20 '18 at 12:26
What function does your first lock actually do? – Andrey Jul 20 '18 at 12:28
Your locks get very short and distributed in time. This is good. Then, lock is effectively a critical section, which is not that heavy comparing to kernel sync objects. – rs232 Jul 20 '18 at 12:29
@Andrey it effectively prevents: a) creating two different collections, b) modifying the collection while iterating over it inside FirstOrDefault. – rs232 Jul 20 '18 at 12:31
1) It is better to create the list in the constructor then do it lazily or use `System.Lazy` 2) FirstOrDefault does not modify the collection 3) Instead of thread-safing the List better use ConcurrentList instead of locks – Andrey Jul 20 '18 at 12:38
1. That's a matter of preference. 2. It does not, but without lock the collection might be modified from outside WHILE executing FirstOrDefault. 3. What is ConcurrentList? – rs232 Jul 20 '18 at 12:44
1. No, it is not matter of preference. Fewer locks is better. Lazy initialisation should be used only if there are reasons to do it, eager is the default. 2, 3: https://learn.microsoft.com/en-us/dotnet/api/system.collections.concurrent.blockingcollection-1.getconsumingenumerable?view=netframework-4.7.2 – Andrey Jul 20 '18 at 12:55
1. Your suggestion does not reduce the amount of locks since we still need to iterate over the collection. Again, while I agree that we totally CAN move the collection initialization to the class constructor or anywhere else, in reality it is NOT IMPORTANT. 2,3. This seems to be a pretty random link to msdn, it does not contribute to the answer at all. – rs232 Jul 20 '18 at 13:13
@rs232 there is class BlockingCollection that basically does blocking for you and FirstOrDefault gonna be thread safe in combination with Add. I am pointing to the fact that there is number of ways to avoid locking entirely. You can use classical indexed `for` with List, because the only modification is appending. – Andrey Jul 20 '18 at 13:49

score 0 · Answer 2 · answered Jul 20 '18 at 13:11

0

You can use a pair of ConcurrentBags, one for used and one for unused connections. Initialise them in the constructor and I think you won't need any locks at all.

answered Jul 20 '18 at 13:11

Andrey

59,039
12
119
163

But using 2 datastructures i still have to keep them synchronized.Whenever i push inside one i have to pop from the other. – Bercovici Adrian Jul 20 '18 at 13:18
1

@BercoviciAdrian that push+pop shouldn't be an atomic operation as long as you pop before pushing. – rs232 Jul 20 '18 at 13:22
1

@BercoviciAdrian not really. it gonna be like this `x = c1.pop(); c2.push(x);` both pop and push individually are atomic and since only 1 thread is in the possession of x then it is safe. You usually should care only about thread safety of shared resources. – Andrey Jul 20 '18 at 13:45

Alternative to lock for async operations

2 Answers2