Levon Ravel
Levon Ravel

Reputation: 100

Multiple threads access a return method is it thread safe with a lock?

I am trying to access a method from multiple threads, I am unsure if this is thread safe. If it is not, what would be the most efficient way to make it safe?

I took into account everyone's insights my final product can be shown below, I have also opted to call recycle on the packet itself.

My packet pool class

using System.Collections.Generic;

namespace Networking
{
    public class PacketPool
    {
        private Queue<Packet> pool = new Queue<Packet>();
        private readonly object instance = new object();

        public Packet CreatePacket(string method)
        {
            lock (instance)
                return pool.Count == 0 ? new Packet() { Pool = this } : 
                       pool.Dequeue();
        }

        public void Recycle(Packet packet)
        {
            lock(instance)
               pool.Enqueue(packet);
        }
    }
}

The Packet Class

using System;
using System.Net;

namespace Networking
{
    public class Packet 
    {
        public Protocol Proto = Protocol.Sequenced;
        public PacketFlags Flag = PacketFlags.None;
        public Fragment Fragmented = Fragment.NotFragged;
        public SendType SendType = SendType.Raw;
        public EndPoint RemoteEp = new IPEndPoint(IPAddress.Any, 0);
        public byte[] Buffer = new byte[512];
        public int Count = 0;
        public int Ptr = 8;
        public int Bits { get; set; } = 0;
        public ushort Id = 0;
        public ushort Lead = 0;
        public PacketPool Pool;

        public void Recycle()
        {
            Bits = 0;

            if (Buffer.Length > 512)
                Array.Resize(ref Buffer, 512);

            Count = 0;
            Flag = PacketFlags.None;
            Fragmented = Fragment.NotFragged;
            Proto = Protocol.Sequenced;
            Ptr = 8;
            Lead = 0;
            SendType = SendType.Raw;
            Pool.Recycle(this);
        }
    }
}

Hoping that the solution above makes life easier.

Upvotes: 2

Views: 181

Answers (2)

Jesse de Wit
Jesse de Wit

Reputation: 4177

The code you've shown looks threadsafe, as long as the packets themselves are not modified on seperate threads. You could also consider making the PacketPool class threadsafe perhaps, to save some precious locking time.

You could simplify it by replacing Queue with ConcurrentQueue.

Note that you are modifying the Packet sent into the Recycle method. I'd recommend to make the data objects immutable, to avoid unexpected behaviour.

public class PacketPool
{
    public ConcurrentQueue<Packet> pool = new ConcurrentQueue<Packet>(2000);

    public Packet CreatePacket(string method)
    {
        if (pool.TryDequeue(out Packet packet))
        {
            return packet;
        }

        return new Packet();
    }

    public void Recycle(Packet packet)
    {
        packet.Bits = 0;

        if (packet.Buffer.Length > 512)
            Array.Resize(ref packet.Buffer, 512);

        packet.Count = 0;
        packet.Flag = PacketFlags.None;
        packet.Fragmented = Fragment.NotFragged;
        packet.Proto = Protocol.Sequenced;
        packet.Ptr = 8;
        packet.Lead = 0;
        packet.SendType = SendType.Raw;
        pool.Enqueue(packet);
    }
}

Upvotes: 4

Julian
Julian

Reputation: 36780

If the Packet isn't shared across threads, then this is thread safe thanks to the locks.

Be careful with the public field pool. You could edit also that on thread unsafe.

Please note that a dedicated lock object is recommended, otherwise there could be a deadlock - e.g another process locks the public property Pool. Recommended to make the lock object private and use it only in that class (so one level up)

When you synchronize thread access to a shared resource, lock on a dedicated object instance (for example, private readonly object balanceLock = new object();) or another instance that is unlikely to be used as a lock object by unrelated parts of the code. Avoid using the same lock object instance for different shared resources, as it might result in deadlock or lock contention.

https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/lock-statement

Upvotes: 1

Related Questions