Comments (9)
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();
from distributedlock.
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.
from distributedlock.
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?
from distributedlock.
@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?
from distributedlock.
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.
from distributedlock.
I find myself in need to mock this too. I'll share whatever wrapper/s I create here in the meantime.
from distributedlock.
@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?
from distributedlock.
@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
).
from distributedlock.
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
from distributedlock.
Related Issues (20)
- Increase in lost lock after upgrading to .NET 7, EF Core 7 and Microsoft.Data.SqlClient 5.0.1 HOT 21
- Add custom data to AzureBlobLeaseDistributedLock HOT 10
- Redis: RedisScript (LuaScript) execution error - CultureInfo problem HOT 7
- DistributedLock.Azure: Remove MaxNonInfiniteLeaseDuration HOT 2
- Use Distributed lock to manage multiple calls to the same Azure function HOT 2
- Postgres distributed lock using PgBouncer connection pooler HOT 8
- About Distribute Locking System using Redis HOT 2
- RobiniaDocs API Explorer HOT 8
- Postgres - Multiple threads successfully acquire the same lock HOT 3
- Concurrency with Redis Semaphore HOT 2
- Error acquiring lock on PostgreSQL when using Npgsql 8.0 HOT 5
- PostgreSQL — Exception while using "AcquireAsync" or "TryAcquireAsync" multiple times HOT 3
- DistributedLock.Azure is updated but not released to NuGet HOT 1
- Document Support for Key Prefixes in RedisDistributedLock HOT 3
- Adopt central package management HOT 2
- Add build checks for AOT compat HOT 2
- Update from vulnerable versions of SQL server packages (dependabot PRs)
- Consider updating Azure.Storage.Blobs to latest once linked issue is resolved
- Rename CopyPackageToPublishDirectory.targets to Directory.Build.targets
- Using UseTransaction for SqlServer locks requires explicitly disabling UseMultiplexing
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from distributedlock.