Passing a lock to another thread

[jhudsoncedaron]

ReaderWriterLock implements IDisposable
ReaderWriterLockSlim implements IDisposable

I got myself into a place where this is massively inconvenient. So I wrote an implementation of a reader writer lock in terms of the managed locking primitives. It didn't work.

The reason it doesn't work may be understood as follows:

var obj = new System.Threading.Lock();
obj.Enter();
var th = new Thread(() => {
	obj.Exit();
});
th.Start();
th.Join();

As a base check, I did the following:

var obj = new System.Threading.Lock();
obj.Enter();
var tb = new Thread(() => {
	obj.Enter();
	System.Threading.Thread.Sleep(2000);
	obj.Exit();
});
tb.Start();
var th = new Thread(() => {
	System.Threading.Thread.Sleep(1000);
        var z = new System.Threading.Lock();
        z.Enter();
	var field = obj.GetType().GetField("_owningThreadId", System.Reflection.BindingFlags.Instance | System.Reflection.BindingFlags.NonPublic);
        field.SetValue(obj, field.GetValue(z));
	obj.Exit();
});
th.Start();
tb.Join();
th.Join();

That code ran through. There's no reason a priori why the method cannot function; but it won't function because the caller checks.

You're probably thinking this doesn't work anyway because the lock is recursive. In my case I can prove the second call to Enter isn't recursive.

Note that just knocking the check out is a bad idea. We would have to have more like a constructor argument to say "make a non-recursive lock that doesn't check".

[jhudsoncedaron]

Stupid alternate solution:

Lock.TransferOwnership(Thread? newOwner);

The lock must be locked.
Either the caller must own the lock, or no thread must own the lock.
If the argument is not null, the new thread becomes the owner.
If the argument is null, no thread owns the lock. Any thread may now call TransferOwnership. A thread must take ownership with TransferOwnership before it can be unlocked.

There is no reason this method would need to check if it's being called by two threads at once.

[Clockwork-Muse]

I got myself into a place where this is massively inconvenient.

.... like what?

Stupid alternate solution:

This is a recipe for deadlocks, because you could be sending the lock to a thread about to exit/crash/whatever, and now the lock is "lost" (and under no circumstances would a thread be able to "steal" a lock from whichever one already owns it, because that would be a recipe for an even larger disaster).

It's somewhat unusual to work directly with Thread in modern code, in preference for using Task and async (for a variety of reasons).

In any case, lock handoffs between threads like you're suggesting are incredibly suspicious, implying that you're probably doing something wrong.

[jhudsoncedaron]

I got myself into a place where this is massively inconvenient.

.... like what?

Right now, I'm looking at a case where the object that uses it doesn't actually have any form of lifetime tracking because it is entered by every single web service endpoint. It's an effective singleton but actually isn't one because editing appsettings.json causes it to be recycled to reload its configuration (editing the configuration on the fly would not work for a host of reasons involving caches and background worker threads). Technically it implements IAsyncDispose to tear down the background worker threads; but there's zilch all to stop a web service call from entering it a few instructions later. The only thing that actually releases the caches is the GC itself.

And we now have a thing that can get hit from one of its endpoints that can't be protected from being entering after dispose starts because that's how MVC.NET works, but has to have a ReaderWriter style lock to be torn down.

You would think I could make the ReaderWriterLock a singleton. Unfortunately this is not true. It's a doubleton. The giant object is a library, and it's loaded into non-web-service processes, and one of them has to talk to two databases at once. Oof. (And the unit test harness creates 8 of them in parallel because it can.)

Stupid alternate solution:

In any case, lock handoffs between threads like you're suggesting are incredibly suspicious, implying that you're probably doing something wrong.

The object is to build two new lock abstractions, ReaderWriterLock and a SymmetricReaderWriterLock (don't know what this is? Don't worry. Nobody does until they see the use case appear.) from managed lock primitives and so don't require IDisposable. The guts within look suspicious, but only have to be written once and can be studied in all directions. An actual implementation of these is about 20 to 50 lines. The primitive inside a ReaderWriterLock does indeed look like handing off the ownership of a lock from one thread to another, but doesn't get stuck.

This is the real base form:

lock (lock1)
{
    if (Interlocked.Add(ref readers, 1) == 1) TakeLock2();
}

if (Interlocked.Add(ref readers, -1) == 1) ReleaseLock2();

But TakeLock2() and ReleaseLock2() also have direct callers that don't go through the counter.

If you think about what the contents of TakeLock2() and ReleaseLock2() must be, you will see the problem.

Unfortunately my proof of not-blowing-up because the lock primitive thinks it's a recursive lock is specific to the actual surrounding code not to being a ReaderWriter lock.

[bencyoung-Fignum]

Just because an object implements IDisposable doesn't mean you have to use it in a Dispose pattern? You could just use it as you want and only dispose it if you know it's finished with...

[jhudsoncedaron]

@bencyoung-Fignum : The code path to actually get rid of it depends on the GC to finish the job. We know by the time Dispose is called, no new references to the object can be obtained; however existing threads could be about to call it. So the only thing that can actually release the object is the GC. Attempting to get rid of that is a whole-codebase transform if it can even be done at all.

Since this is true of the main object, this is also true of the reader-writer lock within it.

[bencyoung-Fignum]

But you could just never dispose it? If there's only a couple in the system it shouldn't matter. However I have to say the underlying architecture is sounding pretty dangerous and trying to contort locking to match may not work out well!

[jhudsoncedaron]

@bencyoung-Fignum : It's a direct consequence of the code in ASP.NET that causes it to pick up changes in appsettings.json while running; otherwise none of this would be a real issue and the dispose would only fire in the test harness.