Correctly lock a variable inside static method

Question

I'm using SignalR to notify clients about some changes. My hub (but it's not fundamental to know it's a SignalR hub) is defined as

public class NotificationHub : Hub
{
    private static readonly IHubContext HubContext = GlobalHost.ConnectionManager.GetHubContext<NotificationHub>();

    private static readonly IDictionary<int, string> UserConnectionMapping = new Dictionary<int, string>();

    private static readonly ILog Log = LogManager.GetLogger(System.Reflection.MethodBase.GetCurrentMethod().DeclaringType);

    private const string UserId = "UserId";

    private readonly object userConnectionMappingLock = new object();



    public static void Static_UpdateStatus(int userId, string message)
    {
      lock(userConnectionMappingLock ) //This causes troubles
      {
         if (UserConnectionMapping.ContainsKey(userId))
        {
            var connectionId = UserConnectionMapping[userId];

            HubContext.Clients.Client(connectionId).updateNotifications(message);
        }
     }
 }
 public override Task OnConnected()
    {
        if (Context.QueryString[UserId] == null) return base.OnConnected();

        var userId = int.Parse(Context.QueryString[UserId]);
        Log.Debug($"User {userId} connected on SignalR with Connection Id {Context.ConnectionId}");

        lock (userConnectionMappingLock)
        {
            UserConnectionMapping[userId] = Context.ConnectionId;
        }

        return base.OnConnected();
    }

Since it's static the method (and it can't be otherwise since I need to access from external classes), should I declare the lock static as well? Consider that there will be only 1 instance of NotifyHub. Thanks

You can't use a non-static member from a static method. The compiler will prevent you. Make it static. — Baldrick, Jun 13 '18 at 08:35
That's not the correct way to do it, you should exctrat the ConnectionMapping outside the hub in a singleton expose Add and Remove methods to handle connected and disconnected users, and to access the bub oustide the hubpipeline, have a look here https://stackoverflow.com/questions/15128125/how-to-use-signalr-hub-instance-outside-of-the-hubpipleline which points to this link https://learn.microsoft.com/en-us/aspnet/signalr/overview/guide-to-the-api/hubs-api-guide-server#callfromoutsidehub if it's not clear let me know I can give you sample code. — lyz, Jun 13 '18 at 08:59
@lyz thanks for your point... an example would be really appreciated, why it's not the correct way to do? I don't either like the facto to have a static Hubcontext here — advapi, Jun 13 '18 at 09:05

score 1 · Answer 1 · answered Jun 13 '18 at 09:23

The thing you're locking and the thing being protected should really have the same lifetime and scope; at the moment UserConnectionMapping is static, and userConnectionMappingLock is per-instance, which is a recipe for disaster.

Frankly, static dictionaries are always a little dangerous, but they can be used safely; options:

make userConnectionMappingLock match the scope of the dictionary - add static to userConnectionMappingLock (or remove static from UserConnectionMapping, see 3)
lose userConnectionMappingLock completely (just throw it away), and lock on the dictionary itself (UserConnectionMapping)
make UserConnectionMapping not be static - so you have a dictionary per hub instance (assuming the lifetime of NotificationHub works for that)
use ConcurrentDictrionary<int,string>

I'd probably lean towards the last one - you're less likely to shoot your foot off with it. Although I think a case could be made for applying 3 in addition to any of 1/2/4.

Correctly lock a variable inside static method

1 Answers1