Transaction scoped DistributedLock is released prematurely

[adzhiljano]

Hey,

Thank you for this great library!

When using a SqlDistributedLock from DistributedLock.SqlServer, scoped to a transaction, it seems that the lock is released before the transaction is commited/rollbacked (sp_releaseapplock executed explicitly before transaction commit)

Wasn't expecting that and thought that this is by design, but then I looked through the code and found the following:

DistributedLock/src/DistributedLock.Core/Internal/Data/DedicatedConnectionOrTransactionDbDistributedLock.cs

Lines 218 to 219 in 3da3eaf

	var canSkipExplicitRelease =
	this._transactionScoped && (!this.Connection.IsExernallyOwned || !this.Connection.CanExecuteQueries);

If we're in the transaction scoped scenario and the transaction hasn't been commited (connection is externally owned and still open) we'll have
true && (!true || !true) -> true && (false || false) -> true && false -> false

I'm not entirely sure of the meaning behind those properties, but shouldn't it match the comment above this piece of code and be like:

var canSkipExplicitRelease =
                        this._transactionScoped || (!this.Connection.IsExernallyOwned || !this.Connection.CanExecuteQueries);

Thanks!

[madelson]

Hmm the comment in the source on that line says:

// For transaction-scoped locks, we can sometimes skip the explicit release step. This comes up when either
// (a) We own the connection and therefore the transaction. In this case we're about to dispose the transaction and release that way
// (b) The transaction is dead (e. g. completed or rolled back) in which case the lock has already been released

This seems in line with what the code is doing. Cases (a) and (b) only apply to transaction-scoped locks, hence this._transactionScoped &&. Then case a is !this.Connection.IsExernallyOwned and case b is !this.Connection.CanExecuteQueries.

The one case we don't consider is when releasing a connection-scoped lock and the connection is dead, but that's not a supported pattern in the library currently; we'd rather fail hard so that you can figure out that you killed the connection and lost the lock early.

What am I missing?