Is it completely safe to use pattern of ReaderWriterLockSlim.EnterXXX() with consequent try-finally clause

Question

MSDN Documentation and many examples of using ReaderWriterLockSlim class recommends using the following pattern:

cacheLock.EnterWriteLock();
try
{
    //Do something
}
finally
{
    cacheLock.ExitWriteLock();
}

But I'm curious if it's completely safe. Is it possible that some exception will happen after lock is acquired, but before the try statement so that lock is stuck in the locked state? The most obvious candidate is ThreadAbortException. I understand that probability of this situation is extreemely small, but the consequences are extreemely bad - so I think it worth thinking about it. I don't believe compiler understands this pattern and prevents processor from interrupting thread before try statement.

If there is theoretical possibility that this code is unsafe, and is there ways to make it safer?

If the documentation is right, then 'ReaderWriterLockSlim' is thread safe and this code sample is as thread safe as it gets. You probably ought to worry more the disposal of `cahceLock`. — Jodrell, Jul 02 '14 at 14:12
The interaction between this pattern with `Monitor.Enter` and `ThreadAbortionException`, is surprisingly subtle. See [Subtleties of C# IL codegen](http://blogs.msdn.com/b/ericlippert/archive/2007/08/17/subtleties-of-c-il-codegen.aspx) on Eric Lippert's blog. In C# 4 the `lock` pattern generates different code to avoid this. — CodesInChaos, Jul 02 '14 at 16:12

score 8 · Answer 1 · answered Jul 02 '14 at 14:15

There are only three ways this could theoretically fail that I can think of:

ThreadAbortException you mentioned already. This is fairly easy to handle correctly: just make sure you never call Thread.Abort(). You almost certainly don't need to; there are almost always better ways of achieving the desired result.

Only if you really, really, really need to call it, and the thread you're aborting is the one with a risk of keeping a lock open, place your entire block of code (from Enter to Exit) in a try...finally clause where the try-block is empty. Thread.Abort() will throw the ThreadAbortException only when the current finally handler finishes.
StackOverflowException is another possibility. It may happen during the call to ExitWriteLock. This is also fairly easy: when a stack overflow happens, the process is terminated. You cannot catch or handle this. Since the process is terminated, no other thread in your process will be keeping any lock open.
OutOfMemoryException might theoretically be thrown during the call to ExitWriteLock. Unlike StackOverflowException, this one is theoretically catchable. If you don't catch it, again the process will be terminated, and no other thread in your process will be keeping any lock open. If you do catch it, however, you cannot hope to handle this correctly, and chances are, your other threads will soon start throwing this exception as well.

In short, I wouldn't worry about it.

Placing everything in the finally block is great idea. I completely forgot about this possibility. I know that calling Abort() is bad practice, but if you develop a reusable library, you can't control it. — Sasha, Jul 02 '14 at 14:24

score 7 · Accepted Answer · edited Jul 02 '14 at 14:47

7

It can be an issue and apparently is in some high-load scenarios. This article goes into it further, but it basically boils down to using an empty try block coupled with a finally to acquire & release the lock:

var lockIsHeld = false;
try {
   try {
   }
   finally {
      rwl.EnterReadLock();
      lockIsHeld = true;
   }

   // Do work here
}
finally {
   if (lockIsHeld) {
      rwl.ExitReadLock();
   }
}

This approach guarantees that your lock is always acquired and release as finally blocks are guaranteed to run even in the case of a ThreadAbortException.

Other exceptions are detailed in @hvd's post.

Personally I wouldn't worry about it unless you actually see this issue in the wild...

edited Jul 02 '14 at 14:47

Sasha

8,537
4
49
76

answered Jul 02 '14 at 14:21

Dean Ward

4,793
1
29
36

2

Using a delegate parameter you can encapsulate this pattern in a method so the caller looks like `rwl.UsingReadLock(() => {...})` – CodesInChaos Jul 02 '14 at 16:15
Interesting. My understanding is that `try` is not translated to a CPU instruction - it simply marks the start of a region in code to which a `catch`/`finally` applies and therefore an exception could not happen between the first `try` and the second `try` (they would both point to the `rwl.EnterReadLock()` instruction). The `lockIsHeld` variable should then be unnecessary. – EM0 Jun 19 '18 at 10:03
1

OK, maybe it is: apparently the compiler can generate a no-op instruction between the two `try` statements? https://blogs.msdn.microsoft.com/ericlippert/2009/03/06/locks-and-exceptions-do-not-mix/ – EM0 Jun 19 '18 at 10:07

Is it completely safe to use pattern of ReaderWriterLockSlim.EnterXXX() with consequent try-finally clause

2 Answers2

Linked