IMHO: IDistributedLock should be public

[nerumo]

IMHO: The IDistributedLock should be public. Because:

• The caller of the lock doesn't have to know what implementation of lock it's using
• Unit Testing: if it already contains an interface, I don't have to abstract the DistributedLock any further.
• The interface is no internal implementation detail, indeed it is the interface of the lock api

Btw, thanks for the good work in this project

[madelson]

Hi @nerumo. I'm glad you like the project and thank you for your thoughts. I agree that having an interface would make testing a bit easier. There are a few reasons why thus far I've kept it internal:

• Lock instances are not interchangeable. SQL and System locks have different semantics as far as scope. Even within the realm of SQL locks, the way the lock is created (connection/transaction vs. connection string) determines whether a single lock instance can be used concurrently across different threads.

• Any change to an interface is a breaking change. Since I try to maintain backwards compatibility, I am always nervous about making that kind of API commitment unless I think it will add a lot of value for consumers. One workaround for this would be to replace the interface with an abstract base class.

• The reason I created the interface internally was so that I could share key common pieces of logic between lock implementations using extension methods. However, I like the fact that the actual public API isn't exposed this way because it is more discoverable when the methods actually belong to the class. From a testing perspective extensions are also frustrating because they cannot be mocked. Again, an abstract class with virtual base implementations would help here.

Definitely something to consider for a future release.

[nerumo]

Tnx for the explanation. I see that the IDistributedLock interface indeed is an internal "implementation detail".

The locks aren't interchangable

Ok, fair enough. How a about an additional interface for each kind of lock?

One workaround for this would be to replace the interface with an abstract base class.

The way mocking frameworks (moq, fakeiteasy etc.) work is, that they work (by far) best with interfaces. Every method that isn't an interface method or isn't virtual, can't be mocked with these frameworks. That's why I'd prefer an interface to work with.

Any change to an interface is a breaking change.

If we wouldn't need to refactor it to an abstract base class and use additional interfaces, the change would be very safe and according to semver, this kind of change would be a minor change:

MINOR version when you add functionality in a backwards-compatible manner

What do you think? Would adding new and separate interfaces for each lock address both of our concerns?

[madelson]

@nerumo just to clarify on the interface compatibility point, my proposal is to follow the "abstract base classes as versionable interface" pattern seen in DbCommand, HttpContextBase, etc (e. g. see https://haacked.com/archive/2008/02/21/versioning-issues-with-abstract-base-classes-and-interfaces.aspx/). While as you say we could add additional interfaces over time, this gets muddy from an API perspective.

I haven't used FakeItEasy but I have use Moq extensively and it works really nicely with abstract base classes set up like interfaces (no logic or state in the ABC at all, just abstract/virtual methods). Does FakeItEasy only support interfaces?

[nerumo]

Thank you for the article link, someone could finally show me a downside of interfaces :). I'm no expert in library/framework design, but aren't many arguments in that article obsolete through the introduction of nuget and it's workflow? If you do an update of this lib, all applications must and will be recompiled anyway (since this lib won't be installed and referenced through the GAC or so). And then it would be terrible not to break compilation and throw a NotImplementedException on runtime. But there are still some valid points and I don't want to take that argue about that into this project :)

FakeItEasy works the same way as Moq, just more of the same ;). But working with interfaces is slightly different:

• you can't mock everything, e.g. the base class constructor. So I have to mock all the parameters to that constructor too
• When working with mocked classes I struggle to figure out which method is now valid or able to be mocked. Of course, if everything of that class is virtual then I don't have to worry, but it's not enforced. So if just a single method/property/event isn't virtual, there are several drawbacks like strict mode which isn't working anymore and also the fact that you then have a mixed mode between real implementation and mocking code. (and this is even happening in the .net Framework).

So long story short: I get your point and I don't mind mocking a an ABC, but for now I just work with my own interface to abstract the DistributedLock.

Still looking forward for updates on this lib, thanks again.

[benmccallum]

I find myself in need to mock this too. I'll share whatever wrapper/s I create here in the meantime.

[benmccallum]

    public interface ISqlDistributedReaderWriterLockFactory
    {
        ISqlDistributedReaderWriterLock Create(string lockName, string connectionString);
    }

    public class SqlDistributedReaderWriterLockFactory : ISqlDistributedReaderWriterLockFactory
    {
        public ISqlDistributedReaderWriterLock Create(string lockName, string connectionString)
            => new SqlDistributedReaderWriterLock(lockName, connectionString);
    }

    public interface ISqlDistributedReaderWriterLock
    {
        Task<IDisposable> TryAcquireWriteLockAsync(TimeSpan timeout = default, CancellationToken cancellationToken = default);
    }

    public class SqlDistributedReaderWriterLock : ISqlDistributedReaderWriterLock
    {
        private readonly Medallion.Threading.Sql.SqlDistributedReaderWriterLock _internalLock;

        public SqlDistributedReaderWriterLock(string lockName, string connectionString)
        {
            _internalLock = new Medallion.Threading.Sql.SqlDistributedReaderWriterLock(lockName, connectionString);
        }

        public Task<IDisposable> TryAcquireWriteLockAsync(TimeSpan timeout = default, CancellationToken cancellationToken = default)
            => _internalLock.TryAcquireWriteLockAsync(timeout, cancellationToken);
    }

Autofac setup:

builder.RegisterType<SqlDistributedReaderWriterLockFactory>().As<ISqlDistributedReaderWriterLockFactory>().SingleInstance();

[madelson]

@benmccallum just curious: is your goal with having an interface to abstract away which type of distributed lock you use or is it purely for mocking in unit tests? In other words, would having various methods be virtual be equally useful?

[benmccallum]

@madelson, in my case it's to facilitate mocking in unit tests. Having the SqlDistributedReaderWriterLock methods be virtual would def help me (as I could just wrap that as Mock<SqlDistributedReaderWriterLock> instead of needing that extra interface ISqlDistributedReaderWriterLock and wrapper concrete class MyNs.SqlDistributedReaderWriterLock).

[cravecode]

IMO, it should not be the responsibility of this library to expose an interface for the sake of abstraction. If this library did expose an interface that you relied on, you'd be defeating the purpose by creating a dependency on this whole library for anything downstream of your initial implementation.

I like @benmccallum's implementation