Can Lock abstraction be simplified by removing waitForLock overload?

[TimLovellSmith]

I've been reading 'Lock' and its lone implementation 'SlimReadWriteLock'.

There is something scary to me about the idea of a Lock class API with operations like 'ForReading', 'ForWriting', 'ForReadingUpgradable' where it will return you a holder of the lock, which I think implies that you own the lock and will thereby release it by Disposing the holder, but that only optionally waits to acquire the lock first (since it has an overload with a (bool waitForLock) signature.

I guess the intention here was that we're going to be able to use this Lock class to write some very optimal code where we don't wait for the lock if we know we already acquired it, or something clever like that right?

Well you may or may not be surprised to learn I could find no such extremely clever code anywhere in Castle.Core... in fact it seems that waitForLock is ALWAYS true. So I was scared for nothing.

So really this issue is just to point out this seems like a great opportunity for some YAGNI-aligned code simplification...

[jonorossi]

... its lone implementation 'SlimReadWriteLock'.

Today we only have a single implementation, .NET 2.0 and Silverlight don't have ReaderWriterLockSlim so this class hid another implementation using Monitor on .NET 2.0 and Silverlight when we used to support those runtimes.

... but that only optionally waits to acquire the lock first (since it has an overload with a (bool waitForLock) signature.

I guess the intention here was that we're going to be able to use this Lock class to write some very optimal code where we don't wait for the lock if we know we already acquired it, or something clever like that right?

Well you may or may not be surprised to learn I could find no such extremely clever code anywhere in Castle.Core... in fact it seems that waitForLock is ALWAYS true. So I was scared for nothing.

With a quick search on GitHub I see one use of ForReading(false) in Windsor, but it doesn't look like a great use case. Yes, it looks like this might have been a little YAGNI. I didn't check any of the other old projects like MonoRail or ActiveRecord.

So really this issue is just to point out this seems like a great opportunity for some YAGNI-aligned code simplification...

We could probably make the API clearer by changing the ForReading overload that takes a waitForLock parameter, and add a TryForReading or something method. However this would be a breaking change.

[stakx]

Today we only have a single implementation, .NET 2.0 and Silverlight don't have ReaderWriterLockSlim so this class hid another implementation using Monitor on .NET 2.0 and Silverlight when we used to support those runtimes.

We could probably make the API clearer by changing the ForReading overload that takes a waitForLock parameter, and add a TryForReading or something method. However this would be a breaking change.

Before breaking changes are considered, I'd like to ask whether it is even worth investing any more effort in an abstraction that no longer serves a purpose other than to give the BCL's ReaderWriterLockSlim a different API?

Why not rewrite code in Core and Windsor to use ReaderWriterLockSlim directly, then mark Lock et al. as [Obsolete]?

[jonorossi]

Before breaking changes are considered, I'd like to ask whether it is even worth investing any more effort in an abstraction that no longer serves a purpose other than to give the BCL's ReaderWriterLockSlim a different API?

Why not rewrite code in Core and Windsor to use ReaderWriterLockSlim directly, then mark Lock et al. as [Obsolete]?

Yer, I agree we should definitely consider changing the code to use RWLS directly as it'll also remove all the allocations of the "disposable holder" objects.

Once we have something rocking in Core I can move from the bottom up. And start changing Windsor :)

[stakx]

@TimLovellSmith - I'll take the liberty of closing this issue. I've just merged the above PR that deprecates Lock, so there's no more point investing any further efforts in improving these types. I hope that's fine with you, otherwise please speak up. 😃

[stakx]

Once we have something rocking in Core I can move from the bottom up. And start changing Windsor :)

@Fir3pho3nixx - Ready, set, go! 🏎 🏁

Cool, let me get the ASPNET Core stuff out first.

If you can keep tagging the issue with significant events, that would be super dooper awesome.