I have Many Clients where each client is having multiple simultaneous Tunnel(Data Fetching Middleware).
I have to Manage all the clients with live Tunnels of each of the Client.
My Tunnel Class is having many Properties and Functions, I am showing only useful properties As :
public class Tunnel : IEquatable<Tunnel>
{
public Guid UID { get; private set; }
public Tunnel(){
this.UID = Guid.NewGuid(); ;
}
public Tunnel(Guid uid_)
{
this.UID = uid_;
}
public bool Equals(Tunnel other)
{
return other != null && other.UID == this.UID;
}
public override bool Equals(object obj)
{
var other = obj as Tunnel;
return other != null && other.UID == this.UID;
}
public override int GetHashCode()
{
return this.UID.GetHashCode();
}
public static bool operator ==(Tunnel tunnel1, Tunnel tunnel2)
{
if (((object)tunnel1) == null || ((object)tunnel2) == null)
{ return Object.Equals(tunnel1, tunnel2); }
return tunnel1.Equals(tunnel2);
}
public static bool operator !=(Tunnel tunnel1, Tunnel tunnel2)
{
if (((object)tunnel1) == null || ((object)tunnel2) == null)
{ return !Object.Equals(tunnel1, tunnel2); }
return !(tunnel1.Equals(tunnel2));
}
// 10 Other Properties
}
I have ClientConnections class which manages all the clients with their LiveTunnels As :
public class ClientsConnections
{
internal readonly ConcurrentDictionary<Object, Dictionary<Guid, Tunnel>> ClientsSessions;
public ClientsConnections(){
this.ClientsSessions = new ConcurrentDictionary<object, Dictionary<Guid, Tunnel>>();
}
public Tunnel AddOrUpdateClientTunnel(Object ClientID, Tunnel tnl)
{
if (tnl.ClientID == null) { tnl.ClientID = ClientID; }
this.ClientsSessions.AddOrUpdate(ClientID, new Dictionary<Guid, Tunnel>() { { tnl.UID, tnl } }, (oldkey, liveTunnels) =>
{
lock (liveTunnels)
{
if (liveTunnels.ContainsKey(tnl.UID))
{
liveTunnels[tnl.UID] = tnl;
}
else
{
liveTunnels.Add(tnl.UID, tnl);
}
}
return liveTunnels;
});
return tnl;
}
public bool RemoveClientTunnel(Object ClientID, Tunnel tnl)
{
Boolean anyRemoved = false;
// When There is No Tunnel i.e. current tunnel is the last in ClientSessions, Remove Entire Key Value from Concurrent Dictionary
if(this.ClientsSessions.ContainsKey(ClientID))
{
Dictionary<Guid, Tunnel> removedTunls;
Dictionary<Guid, Tunnel> liveTunls = this.ClientsSessions[ClientID];
lock (liveTunls)
{
if (liveTunls.ContainsKey(tnl.UID))
{
liveTunls.Remove(tnl.UID);
if(!anyRemoved){ anyRemoved = true;}
}
}
if (liveTunls.Count == 0)
{
//No Tunnels for this ClientID, Remove this client from Concurrent Dictionary
this.ClientsSessions.TryRemove(ClientID, out removedTunls);
if (removedTunls.Count != 0)
{
// Oops There were some Livetunnels, add them back
AddOrUpdateClientTunnelRestore(removedTunls);
}
}
}
return anyRemoved;
}
public bool AddOrUpdateClientTunnelRestore( Dictionary<Guid, Tunnel> tnltoAdd)
{
bool anyAdded = false;
Object ClientId = tnltoAdd[tnltoAdd.Keys.First()].ClientID;
this.ClientsSessions.AddOrUpdate(ClientId, tnltoAdd, (oldkey, liveTunnels) =>
{
lock (liveTunnels)
{
foreach (Tunnel tmpTnl in liveTunnels.Values)
{
if (!liveTunnels.ContainsKey(tmpTnl.UID))
{
liveTunnels.Add(tmpTnl.UID, tmpTnl);
if (!anyAdded) { anyAdded = true; }
}
}
}
return liveTunnels;
});
return anyAdded;
}
}
When there is no LiveTunnel of a client the entire client must be removed from the ConcurrentDictionary.
Is there any better way to do so, Specially in function RemoveClientTunnel ?
For the current Scenario, there are Around 10,000 Clients and each client have at least 2 to 4 LiveTunnels, on an average 8 to 10 LiveTunnels per Client.
Frequency : There are some time duration where the client connection frequencies are high. Eg, At 9:30 Am all clients starts to connect, around 12 Pm Clients starts disconnecting (30-50%), around 2 PM Clients Re-connects, 5PM Clients Starts disconnecting.
A high traffic starts from 9:30Am. The frequency of Tunnel : each clients holds the Tunnel at least for 1-2 Sec. minimum. If we count the minimum time duration a tunnel holds is minimum 1-2 Sec. There is No Max Time limit of a Tunnel Duration. A Client can hold any number of tunnels for a very long time duration (18 Hours )
CodePudding user response：
Well, your RemoveClientTunnel is fundamentally broken, so it's hard to find a worse way to do it really, unless we get into grading how much more worse it can get.
Specifically, you're using the ContainsKey followed by this[] anti-pattern in a multi-threaded environment, which is a race condition if two different threads are trying to remove sessions. At the very least, the lock in that function should be moved up to above the ContainsKey, or even better use composite functions that handle atomicity for you, such as TryGet or TryUpdate.
In other words, either you handle atomicity yourself (such as with your lock, or even better something more granular that doesn't require you to run kernel code every single loop, like ReaderWriterLockSlim), or you rely completely on the ConcurrentDictionary implementation, but then you have to actually use its primitives, not do this mish-mash of code that drops atomicity randomly inbetween calls. On a long enough timeline, you'll see null reference exceptions on the lock (liveTunls) line, because the indexer couldn't find the item that ContainsKey claims is there.
CodePudding user response：
Your usage scenario: "lots of clients with a few tunnels per client", indicates that using immutable dictionaries as values of the ConcurrentDictionary is a viable option. It's neither a highly performant nor a memory-efficient option, but it offers simplicity and easily provable correctness in return. Essentially by making the values of the ConcurrentDictionary immutable, you free yourself from most of the synchronization-related responsibility. The ConcurrentDictionary will provide the basic synchronization by itself, and you'll only have to resolve some edge cases by spinning, not by locking. Here is how you could do it:
public class ClientsConnections
{
private readonly ConcurrentDictionary<object,
ImmutableDictionary<Guid, Tunnel>> _sessions = new();
public Tunnel AddOrUpdateClientTunnel(object clientID, Tunnel tunnel)
{
if (tunnel.ClientID == null) tunnel.ClientID = clientID;
_sessions.AddOrUpdate(clientID,
_ => ImmutableDictionary.Create<Guid, Tunnel>().Add(tunnel.UID, tunnel),
(_, existing) => existing.SetItem(tunnel.UID, tunnel));
return tunnel;
}
public bool RemoveClientTunnel(object clientID, Tunnel tunnel)
{
while (true)
{
if (!_sessions.TryGetValue(clientID, out var existing))
return false; // clientID was not found
var updated = existing.Remove(tunnel.UID);
if (updated == existing) return false; // tnl.UID was not found
if (updated.IsEmpty)
{
if (_sessions.TryRemove(KeyValuePair.Create(clientID, existing)))
return true; // Client successfully removed
}
else
{
if (this._sessions.TryUpdate(clientID, updated, existing))
return true; // Client successfully updated
}
// We lost the race to either TryRemove or TryUpdate. Try again.
}
}
}
As an added bonus you would be able to get a snapshot of all the client connections at a specific point in time, by invoking the ConcurrentDictionary.ToArray method. The immutable collections have snapshot semantics. You can enumerate them at your own leisure, without having to worry about concurrent updates invalidating the enumeration.
If you want to get fancy you could encapsulate the TryGetValue TryRemove/TryUpdate spinning logic in an extension method TryUpdateOrRemove for the ConcurrentDictionary class, and use it like this:
public bool RemoveClientTunnel(object clientID, Tunnel tunnel)
{
return _sessions.TryUpdateOrRemove(clientID,
(_, existing) => existing.Remove(tunnel.UID));
}
The TryUpdateOrRemove method is here.