2

For the concurrency programs I have been writing in C#, my locks/synchronization tend to follow this pattern:

try
{
    Monitor.Enter(locker);

    // critical region
}
finally
{
    Monitor.Exit(locker);
}

This is a classic pattern I've discovered in one of Paul Deitel's C# books years ago, and have been following ever since. This has worked very well for many applications I've done that required concurrency. However, I recently had a discussion with another developer about concurrency and they asked me... why are you doing it that way when you could be using "scoped locks".

To this point I'll admit I had no idea what they were. I started doing some digging to figure out whether this was something that might be helpful to writing my applications. In my search I discovered this RAII (resource acquisition is initialization) pattern, which to my understanding allows for scoped locks. I guess that this pattern is predominantly used in C++ but there are some pseudo-implementations out there in C# such as the following (taken from Resource Acquisition is Initialization in C#):

using System;

namespace RAII
{
    public class DisposableDelegate : IDisposable
    {
        private Action dispose;

        public DisposableDelegate(Action dispose)
        {
            if (dispose == null)
            {
                throw new ArgumentNullException("dispose");
            }

            this.dispose = dispose;
        }

        public void Dispose()
        {
            if (this.dispose != null)
            {
                Action d = this.dispose;
                this.dispose = null;
                d();
            }
        }
    }

    class Program
    {
        static void Main(string[] args)
        {
            Console.Out.WriteLine("Some resource allocated here.");

            using (new DisposableDelegate(() => Console.Out.WriteLine("Resource deallocated here.")))
            {
                Console.Out.WriteLine("Resource used here.");

                throw new InvalidOperationException("Test for resource leaks.");
            }
        }
    }
}

I am just trying to understand how this pattern produces synchronization in C#... Is the synchronization inherent to something having to do with resource allocation?

Also, isn't it kind of abusing the language to force a user to use a constructor w/using in order for this to work? What happens if something is cancelled and an exception is thrown in a constructor? I don't see where this is that much better than good old Enter/Exit pattern.

Glorfindel
  • 3,137
  • 6
  • 25
  • 33
Snoop
  • 2,718
  • 5
  • 24
  • 52
  • 5
    Just to understand your question better, why are you not using the `lock` statement? https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/lock-statement – BgrWorker Jun 18 '18 at 13:35
  • @BgrWorker I did not know (until you pointed that out) that a `lock` releases automatically, even if an exception is thrown. – Snoop Jun 18 '18 at 13:40
  • Your disposable isn't doing any synchronization here. If you add the `Monitor.Enter` and `Monitor.Exit` (or something similar) to the disposable's ctor and dispose (being careful of double disposes) then you can let the `using` deal with exception handling and the such. It's maybe a little more robust than manual `try`/`finally` since there's less to forget. – Telastyn Jun 18 '18 at 13:57
  • @Telastyn have you actually seen anything like what I am describing here, or are you just adding your comment based on what you see in front of you? Or in other words, am I completely missing the point/implementation of scoped locks in C#? – Snoop Jun 18 '18 at 14:04
  • 3
    Sure, I've seen disposables used for this sort of thing. It's a common approach to "this must be called when you're done!" problems. – Telastyn Jun 18 '18 at 14:36
  • Isn't the *whole point* of the `IDisposable` machinery to call the "this must be called when you're done!" things when you are done, exactly *because* finializers are not destructors? – Caleth Jun 18 '18 at 14:49
  • @Caleth is this addressed at me? Based on my understanding I would agree, that is the point of `IDisposable`. Not sure about the *finalizers are not destructors* part... but I do know that with the `IDisposable` machinery, Windows will (at some point) GC the resources in question. – Snoop Jun 18 '18 at 14:58
  • @Caleth: The "whole point" of the `IDisposable` interface (and its counterpart `using` statement in the C# language) is to [*clean up unmanaged resources.*](https://msdn.microsoft.com/en-us/library/system.idisposable(v=vs.110).aspx) From time to time, it is used to provide deterministic completion of a process, such as closing a file or terminating a connection. It is also occasionally abused to do things like [close HTML tags](https://lostechies.com/jimmybogard/2008/01/16/abusing-using-statements/). – Robert Harvey Jun 18 '18 at 15:07
  • @Snoop in C++ has deterministic destruction. The language guarantees *exactly when* destructors will run, and classes should rely on that for correctness. C# does not have such determinism, outside `IDisposable`. I would classify (amongst more "normal" resources) "The enabled state of UI elements that were disabled when this action started" as "an unmanaged resource", that RAII is appropriate for. – Caleth Jun 18 '18 at 15:10
  • @Caleth: To be clear, Microsoft defines an "unmanaged resource" as "not subject to garbage collection." – Robert Harvey Jun 18 '18 at 15:12
  • @RobertHarvey My concept of implementing `IDisposable` was... throw every field into the `Dispose` method that already implements `IDisposable`. Is that completely wrong? – Snoop Jun 18 '18 at 15:14
  • @Snoop: It's not wrong, but you have to be careful. For example, people routinely avoid disposing Entity Framework data contexts, because you can rip the rug out from under an `IQueryable`. As my links demonstrate, Microsoft doesn't even always follow their own rules, so it's not as simple as summarily calling `Dispose()` on everything that implements `IDisposable`. – Robert Harvey Jun 18 '18 at 15:20
  • @RobertHarvey Thanks, that's good advice. Will be more careful on how I am implementing `IDisposable`. – Snoop Jun 18 '18 at 15:23
  • have a look at this: https://www.youtube.com/watch?v=2yXtZ8x7TXw locks are not the only (or even the best) solution to concurrency. – ctrl-alt-delor Jun 18 '18 at 18:17
  • Try to avoid holding more than one lock at a time ("scoped locks"). This is a classic solution to a classic problem, but it is far simpler to rework the design so you only need to hold one lock at a time, by retrieving the data you need in local variables, letting go of one lock, then opening another. Scoped locks cause no end of problems. – Frank Hileman Jun 19 '18 at 00:00
  • @FrankHileman what do you mean *scoped locks cause no end of problems*? Are you saying that... scoped locks *cause* problems? Just a bit confused there. – Snoop Jun 19 '18 at 12:05
  • I could have phrased that better. Nothing wrong with using a lock statement or they way you do it, equivalently. What I meant was nested locks. They are very difficult to get right, tend to regress over time (someone breaks them), and almost never needed: with a little work, you can break the lock scopes up so they are no longer nested. – Frank Hileman Jun 19 '18 at 23:52
  • @FrankHileman Okay, thanks for clearing that up. Not to be too difficult, but where exactly does the nested lock come into play here? Just not seeing it, but then again I'm obviously searching for a better understanding :) – Snoop Jun 20 '18 at 15:02
  • Nested locks are a classic solution to the problem of multiple locks open at once. Look up lock hierarchy for more information. You would most often encounter this when implementing multi-threaded servers. – Frank Hileman Jun 20 '18 at 17:53

2 Answers2

2

Your question looks a bit confusing to me, since both code snippets are not equivalent, but I guess what you really meant is the following: why should someone use something like

 Monitor.Enter(locker);
 using (new DisposableDelegate(() => Monitor.Exit(locker))
 {
     // critical region
 }

instead of the try/finally block from your question?

That is indeed a sensible question, because both ways are semantically equivalent, and there is IMHO no benefit from reinventing the wheel by such an DisposableDelegate class.

Of course, when you follow the link from the comment below the question you linked to, you find a better implementation of the "disposable delegate": the ResourceProtector, which lets your write code like

 using (new ResourceProtector(()=> Monitor.Enter(locker), () => Monitor.Exit(locker))
 {
     // critical region
 }

This has indeed some advantage over try/finally, since it groups the corresponding allocation/deallocation statements together. So the code becomes less error prone, especially when it is used in context of multiple resource allocations, other statements before and after the allocation, or when it is evolved.

Doc Brown
  • 199,015
  • 33
  • 367
  • 565
0

I am just trying to understand how this pattern produces synchronization in C#... Is the synchronization inherent to something having to do with resource allocation?

The end brace (}) of the using block is an implicit call to Dispose.

The resource is acquired at the = (initialisation), and released at the end of that scope. The name RAII comes from C++, where scopes demarcate the lifetime of objects declared within them, and code in destructors runs however the scope is left (early return, exception thrown, control flows off the bottom)

Also, isn't it kind of abusing the language to force a user to use a constructor w/using in order for this to work?

It depends on what you mean by "abusing". This is C#'s only way of deterministically cleaning up

What happens if something is cancelled and an exception is thrown in a constructor?

The same thing that happens if an exception is thrown in the code before a try. Hopefully whatever that is, is also safe. I find (modern) C++ better in this regard, as it is easy to write a class that cleans up properly from a half-formed instance throwing.

I don't see where this is that much better than good old Enter/Exit pattern.

It's rather easy to define a warning for

CA0000 You created an IDisposable (MyClass myObj) and there is a code path that fails to Dispose it

rather than (hundreds of variations of)

CA0000 You called myObj.SomeSetup() and there is a code path that doesn't reach myObj.SomeTeardown()

Especially as there isn't a mechanism for an author of MyClass to indicate to the compiler that MyClass.SomeSetup must be paired with MyClass.Teardown

In the specific case of synchronising on an object, the lock statement is more sensible, but you could write something like

class ScopedLock : IDisposable
{
    private object obj;
    public ScopedLock(object obj)
    {
        this.obj = obj;
        if (this.obj != null) { Monitor.Enter(obj); }
    }
    public void Dispose()
    {
        if (this.obj != null) { Monitor.Exit(obj); }
        this.obj = null;
    }
}
Caleth
  • 10,519
  • 2
  • 23
  • 35