Expose a method to force a lock to be released.

[madelson]

Expose a method to force a lock to be released.

[madelson]

Hi Matthew, Thanks for reporting and thanks for your interest in the library. The idea of locks not releasing when Dispose returns without error is indeed concerning. A “force kill” seems difficult to implement because the lock is “held” by the underlying sql connection or transaction rather than by application state within the library. To do this id need to add global tracking of all the created connections and transactions in a way that did not leak memory or violate thread safety (underlying connections/transactions are not thread safe). One way you could implement this yourself would be to use the constructors that take in a connection/transaction. If you are managing the connection/transaction you can force kill. Perhaps a better solution would be to use SQL itself. There is a system view that lets you see all of the held locks I believe; you could use this to find the holding SPID and kill it with the SQL kill command. I would really like to get to the bottom of the issue. A few steps: - which locking strategy are you using? There are different supported approaches. If you are using the default you could try another approach like transaction and see if that makes a difference - can you post the code you use to call into the library? - do you have logs that would let you know whether Dispose ever fails? - are you using Azure? What version of sql server?

…

On Nov 1, 2019, at 3:50 AM, Matthew Katinsky ***@***.***> wrote:  We have a scalable application that makes tens of thousands - perhaps hundreds of thousands - of calls to DistributedLock every hour from dozens of processes running in parallel. For the most part it has been robust, but we have encountered a couple of instances where a lock is acquired and never released. When this happens, there is a cascading effect and several other processes hang or fail because they can never get the lock to that resource. We've run this app for months, and the problem has only happened twice so far. So far, we have no clear way to intentionally replicate the problem. We will continue to investigate, but in the meantime, as a workaround, would it be possible for you to expose a method that would force all locks on a given path resource to be released? I realize this is a dangerous tool that would have to be used carefully. However, we rarely hold a lock for more than a couple of seconds, so it would only be used in a case where a process could not access a lock after an hour or so. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[phoeniceus]

Thanks. I will look into some of those options.

Locking strategy and code:

  var sqlDistributedLockProvider = new DistributedLockProvider(_appSettings.SqlLockConnectionString, _log);
  using (_sqlDistributedLockProvider.Lock(path))
  {
    // Do something with path
  }

  public class DistributedLockProvider
  {
    private string _connectionString;
    private ILogger _log;

    public DistributedLockProvider(string connectionString, ILogger log)
    {
      _connectionString = connectionString ?? throw new ArgumentNullException(nameof(connectionString));
      _log = log;
    }

    public IDisposable Lock(string path)
    {
      var pathString = SqlDistributedLock.GetSafeLockName(path);
      var fileLock = new SqlDistributedLock(pathString, _connectionString, SqlDistributedLockConnectionStrategy.OptimisticConnectionMultiplexing);

      /*
       * We ask for locks so often, that we end up using up all the DB connections 
       * available in the pool. The next request throws an exception after the 100 second timeout.
       * So, instead of calling Acquire(), which holds the connection until the lock is 
       * received, we will use TryAcquire(), which releases the connection if the lock is 
       * not available. This should reduce the load on the number of connections required. 
       */
      IDisposable result = null;
      while (result == null)
      {
        try
        {
          result = fileLock.TryAcquire(timeout: new TimeSpan(0, 0, 10));
          if (result == null)
          {
            _log?.Log(LogLevel.Debug, $"Could not acquire a lock for {pathString} within ten seconds. Will wait and try again.");
          }
        }
        catch (Exception e)
        {
          // This usually indicates that we could not get a connection within the default 100 second timeout.
          _log?.Log(LogLevel.Warning, e, $"Could not acquire a lock for {pathString}. Will wait and try again.");
        }
        if (result == null)
        {
          Task.Delay(1000).Wait();
        }
      }
      return result;
    }
  }

All of our code is wrapped in extensive error trapping and logging, so if DIspose() failed there is a strong chance we'd see an exception message. But I cannot promise 100%.

We are not using Azure. I believe we are using SQL Server 2016.

[madelson]

Thanks! This is helpful. Some thoughts: Good to know that you are using connection multiplexing. This has some additional complexity and it’s possible there could be a rarely-hit race condition there that is causing the issue. That’s the first thing i want to look into in the distributed lock source. One thing that might help with the investigation is to see if we can reproduce via a unit test that replicates a usage profile similar to what your app is doing when problems occur. If you have a sense of what that looks like (e. G. Number of different lock paths, rough amount of concurrency, how long locks are held, etc) then that could help in creating such a test. Another thing I’d want to know is what types of exception messages get logged in the catch all block. It’s possible that this is swallowing something that could indicate internal state corruption of the multiplexer. Given that you are currently multiplexing, I would consider trying out the default strategy and modifying your connection string to set a larger max pool size so that you don’t exhaust the pool. Hopefully that can provide a workaround while we figure out the underlying problem.

…

On Nov 2, 2019, at 2:51 AM, Matthew Katinsky ***@***.***> wrote:  Thanks. I will look into some of those options. Locking strategy and code: var sqlDistributedLockProvider = new DistributedLockProvider(_appSettings.SqlLockConnectionString, _log); using (_sqlDistributedLockProvider.Lock(path)) { // Do something with path } public class DistributedLockProvider { private string _connectionString; private ILogger _log; public DistributedLockProvider(string connectionString, ILogger log) { _connectionString = connectionString ?? throw new ArgumentNullException(nameof(connectionString)); _log = log; } public IDisposable Lock(string path) { var pathString = SqlDistributedLock.GetSafeLockName(path); var fileLock = new SqlDistributedLock(pathString, _connectionString, SqlDistributedLockConnectionStrategy.OptimisticConnectionMultiplexing); /* * We ask for locks so often, that we end up using up all the DB connections * available in the pool. The next request throws an exception after the 100 second timeout. * So, instead of calling Acquire(), which holds the connection until the lock is * received, we will use TryAcquire(), which releases the connection if the lock is * not available. This should reduce the load on the number of connections required. */ IDisposable result = null; while (result == null) { try { result = fileLock.TryAcquire(timeout: new TimeSpan(0, 0, 10)); if (result == null) { _log?.Log(LogLevel.Debug, $"Could not acquire a lock for {pathString} within ten seconds. Will wait and try again."); } } catch (Exception e) { // This usually indicates that we could not get a connection within the default 100 second timeout. _log?.Log(LogLevel.Warning, e, $"Could not acquire a lock for {pathString}. Will wait and try again."); } if (result == null) { Task.Delay(1000).Wait(); } } return result; } } All of our code is wrapped in extension error trapping and logging, so if DIspose() failed there is a strong chance we'd see an exception message. But I cannot promise 100%. We are not using Azure. I believe we are using SQL Server 2016. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

[madelson]

@phoeniceus any update? I've been trying to reproduce this locally to no avail.

[madelson]

Closing due to inactivity/inability to reproduce.

@phoeniceus if you're still encountering this issue with the current version of the library please feel free to reopen and continue the discussion.