6
\$\begingroup\$

Take a look at following code:

/*abstract*/ class Capturable
{
    object _lock = new object();
    public bool IsCaptured { get; private set; }

    public bool TryCapture()
    {
        if (!IsCaptured)
            lock (_lock)
                if (!IsCaptured)
                    return IsCaptured = true;
        return false;
    }

    public void Release()
    {
        lock (_lock)
            IsCaptured = false;
    }
}

The idea is to inherit from Capturable when I need an object with exclusive access. Object will be used from different places (not necessarily different threads). If this object is needed, then the caller should first call TryCapture() (so theoretically there could be many calls when object is occupied, therefore double check locking is used to quickly return false).

Expected scenarios of usage:

  1. periodically do something (e.g. by timer);
  2. get exclusive access for a duration (e.g. if there is a long operation during which nobody should change state of object);

Both cases are covered with something like:

var obj = Manager.Get<SomeType>();
...
if(obj.TryCapture())
    try
    {
        ...
        obj.SomeMethod();
        ...
    }
    finally
    {
        obj.Release();
    }

The questions:

  • Is this a good approach to get exclusive access? Code-wise and architecturally? E.g. I can use Monitor.TryEnter instead of double check locking, but not sure how to return IsCaptured then.

  • Is there a possibility to avoid if+TryCapture+try+finally everywhere? Similar to how clean lock looks like (it covers Monitor calls) or how nice using is to ensure Dispose() call. The problem here comes when multiple of such object has to be captured at the beginning of method, then it will looks like dozens of nested if/try/finally, which I don't like.

\$\endgroup\$
  • \$\begingroup\$ Hello! Please don't make changes to the original post once it has been reviewed, as that invalidates the current answers. Please see our meta side on performing iterative reviews for more information! It looks like your edit could have just been a comment on the reviewers post. \$\endgroup\$ – syb0rg Jun 16 '16 at 12:47
  • \$\begingroup\$ @syb0rg, I am new to this site. Is it bad to add edits (at the end of question without modifying existing part) to clarify? Clarification was big (I miss it when post original question) to fit as comment (as well as it contain code snippet). \$\endgroup\$ – Sinatr Jun 16 '16 at 12:50
  • \$\begingroup\$ The edit read to me like it was targeted at the reviewer's post to this question. If you abstract it more, I'd say it might be an okay edit, but I think a comment on that users review would be better. \$\endgroup\$ – syb0rg Jun 16 '16 at 12:53
6
\$\begingroup\$

What happens if this is called by something that hasn't captured the object?

public void Release()
{
    lock (_lock)
        IsCaptured = false;
}

Honestly, I don't see what this is giving you that you don't get from:

Monitor.TryEnter(lockObj)

and

Monitor.Exit(lockObj);

Unless you want to release the capture from a different thread, in which case it seems like you're more likely to run into the 'Released by wrong thing` problem.

Why do you need to know if it's captured? Try to capture the lock and if it fails, you know that it's captured... What purpose does the property really serve?

The problem here comes when multiple of such object has to be captured at the beginning of method, then it will looks like dozens of nested if/try/finally

If you're going to be capturing groups of objects then you're going to need to be very careful about the order that you capture those objects in order to prevent deadlocks where one method performs:

Capture A, B, C

And another method performs

Capture C, A

One of the things you've indicated is that you don't like the idea of the nested try/finally blocks to ensure that all captured items are released. You could consider writing some utility functions to help make this cleaner. Something like this:

static bool TryCaptureAll(params Capturable[] capturables)
{
    List<Capturable> capturedItems = new List<Capturable>();

    foreach(var capturable in capturables)
    {
        if(!capturable.TryCapture())
        {
            // Failed to capture one of the items, release the ones
            // we did capture and return failure.
            foreach(var captured in capturedItems)
            {
                captured.Release();
            }
            return false;
        }

        capturedItems.Add(capturable);
    }

    return true;
}
static void ReleaseAll(params Capturable[] capturables)
{
    foreach (var capturable in capturables)
    {
        capturable.Release();
    }
}

Would allow you to lock and release groups of objects like this:

if (CaptureList.TryCaptureAll(a, b, c))
{
    try
    {
        a.SomeMethod();
        b.SomeMethod();
        c.SomeMethod();
    }
    finally
    {
        CaptureList.ReleaseAll(a, b, c);
    }
}
\$\endgroup\$
  • \$\begingroup\$ See edit. Regarding Monitor.TryEnter - checking bool is faster and want to avoid any overhead when object is captured and something else want to capture it. I see it like if something else would check if(!obj.IsCaptured), this check is simply inside method (which I think is convenient). As for group - order doesn't matter. Really. A, B and C are individual objects, it doesn't matter which one you capture first or release first. I only need guarantee what nothing else attempt to access object members unless TryCapture return true for them. \$\endgroup\$ – Sinatr Jun 16 '16 at 12:48
  • \$\begingroup\$ Regarding first part: currently I assume what the caller is responsible to call TryCapture() first before using any other member of object (including Release()). I can check if(!IsCaptured) throw new InvalidOperationException() at the beginning of each method (including Release()), but this will not ensure what it was same caller (e.g. same thread). ideally this require kind of authorization, where TryCapture() returns token, which then has to be passed to other members (to check if this token was used to capture). \$\endgroup\$ – Sinatr Jun 16 '16 at 12:56
  • \$\begingroup\$ @Sinatr is releasing from a different thread to the thread that performed the capture an actual requirement? It's a lot more straightforward if it has to be the same thread, since you can use Monitor. If it's not a requirement, do your threads have that much work to do if they fail to capture an object that there will be a noticeable performance difference between Monitor and checking the Capture property? \$\endgroup\$ – forsvarir Jun 16 '16 at 13:08
  • \$\begingroup\$ Take a look. Replacing double check locking doesn't change a thing, but you can see problem 2 more clearly there. P.S.: my experience with this site is poor, it needs tools to simplify code-conversations, putting code in comments is awful, without code is too much blablabla.. =( \$\endgroup\$ – Sinatr Jun 16 '16 at 13:54
  • \$\begingroup\$ @Sinatr I've added a suggestion around your nested try/finally issue. \$\endgroup\$ – forsvarir Jun 16 '16 at 15:50
2
\$\begingroup\$

Let’s define some executable example. First: resources to protect. They are not thread safe:

class ResourceA
{
    public void A() { Console.WriteLine("A!"); }
}
class ResourceB
{
    public void B() { Console.WriteLine("B!"); }
}
class ResourceC
{
    public void C() { Console.WriteLine("C!"); }
}

Resource locator to get access to resources (you manager class). Please also see here.

class ResourceLocator
{
    public T Get<T>() => Instances
        .OfType<T>()
        .FirstOrDefault();

    IEnumerable<object> Instances = new object[] 
    {
        new ResourceA(), new ResourceB(), new ResourceC()
    };
}

Now let's implement protected thread safe façade, which unifies order of locking (like A -> B -> C) to get rid of dead locks:

class ABCService
{
    public ABCService(ResourceLocator resources)
    {
        Resources = resources;
    }

    // Try-Parse pattern, operation here can be skipped if
    // resources are busy: 
    // (https://msdn.microsoft.com/en-us/library/ms229009(v=vs.110).aspx)
    public bool TryAB()
    {
        var a = Resources.Get<ResourceA>();
        var b = Resources.Get<ResourceB>();
        using (Capture capture = new MultiCapture(a, b))
        {
            if (capture)
            {
                a.A(); b.B(); a.A();
            }

            return capture;
        }
    }

    // Required operation, Exception will be thrown 
    // if resources are busy, so operation can not be completed
    public void BC()
    {
        var b = Resources.Get<ResourceB>();
        var c = Resources.Get<ResourceC>();
        using (new RequiredCapture(b, c))
        {
                c.C(); b.B(); 
        }
    }

    // Required operation, takes all three resources
    // Please note the order of resource capturing - 
    // it should be the same
    public void ABC()
    {
        var a = Resources.Get<ResourceA>();
        var b = Resources.Get<ResourceB>();
        var c = Resources.Get<ResourceC>();
        using (new RequiredCapture(a, b, c))
        {
            c.C(); b.B(); a.A();
        }
    }

    ResourceLocator Resources { get; }
}

Now let's define an active (threaded) component. You could find it useful to pass CancellationToken to resource actions to support long running processes termination:

class Actor : IDisposable
{
    public Actor(ResourceLocator resources)
    {
        var service = new ABCService(resources);
        Cts = new CancellationTokenSource();
        Token = Cts.Token;
        Task.Run(async () =>
        {
            while (!Token.IsCancellationRequested)
            {
                await Task.Delay(50);
                if (!service.TryAB())
                    Console.WriteLine("AB was busy.");
            }
        });
        Task.Run(async () =>
        {
            while (!Token.IsCancellationRequested)
            {
                await Task.Delay(100);
                try
                {
                    service.BC();
                }
                catch
                {
                    Console.WriteLine("BC was busy.");
                }                    
            }
        });
    }

    public void Dispose()
    {
        Cts.Cancel();
        Cts.Dispose();
    }

    CancellationTokenSource Cts { get; }
    CancellationToken Token { get; }
}

Let’s run:

class Program
{
    static void Main(string[] args)
    {
        var resources = new ResourceLocator();

        // we can safely have more than one actor
        var a1 = new Actor(resources); 
        var a2 = new Actor(resources);

        Console.ReadLine();
    }
}

Library code:

public abstract class Capture : IDisposable
{
    public static implicit operator bool(Capture capture) =>
        capture.Active;

    protected abstract bool Active { get; }
    public abstract void Dispose();
}

public class TargetCapture : Capture
{
    public TargetCapture(object target)
    {
        if (Monitor.TryEnter(target))
            Target = target;
    }

    protected override bool Active => Target != null;

    public override void Dispose()
    {
        if (!Active)
            return;

        Monitor.Exit(Target);
        Target = null;
    }

    object Target { get; set; }
}

public class MultiCapture : Capture
{
    public MultiCapture(params object[] targets)
    {
        Captures = targets
            .Select(t => new TargetCapture(t))
            .ToArray();

        if (!Active)
            Dispose();
    }

    protected override bool Active => 
        Captures.All(c => c);

    public override void Dispose()
    {
        foreach (var capture in Captures)
            capture.Dispose();
    }

    IEnumerable<Capture> Captures { get; }
}

public class RequiredCapture : MultiCapture
{
    public RequiredCapture(params object[] targets)
        : base(targets)
    {
        if (!Active)
            throw new InvalidOperationException();
    }
}

UPDATE Just in case if you never block a sequence of resources:

class ResourceLocator
{
    public void Lock<T>(Action<T> handler)
    {
        var obj = Get<T>();
        using (new RequiredCapture(obj))
            handler(obj);
    }

    public bool TryLock<T>(Action<T> handler)
    {
        var obj = Get<T>();
        using (var capture = new TargetCapture(obj))
        {
            handler(obj);
            return capture;
        }
    }

    T Get<T>() => Instances
        .OfType<T>()
        .FirstOrDefault();

    IEnumerable<object> Instances = new object[] 
    {
        new ResourceA(), new ResourceB(), new ResourceC()
    };
}

Now:

class Program
{
    static void Main(string[] args)
    {
        var resources = new ResourceLocator();
        Task.Run(() => 
        {
            while (true)
                resources.Lock<ResourceA>(a => a.A());                                
        });
        Task.Run(() => 
        {
            while (true)
            {
                resources.Lock<ResourceA>(a => a.A());                                
                resources.Lock<ResourceB>(b => b.A());                                
            }
        });

        Console.ReadLine();
    }
}
\$\endgroup\$
  • \$\begingroup\$ That was a lot of effort, thanks. Something I don't understand: in ABCService.TryAB you return MultiCapture instance from within using block (so it will be disposed for the caller), right? I don't know why deadlocking is a concern. My problem is not locating resources or synchronize access to combination of them (imagine you have 20 objects, that would be 20! combinations, order doesn't matter, will you create so many methods or what???). I don't want to group them. They are independent. Lock-free. Objects. I simply want to lock access to one at a time. Then to another. Nothing more. \$\endgroup\$ – Sinatr Jun 20 '16 at 7:19
  • \$\begingroup\$ 1) Actually, TryAB returns no more than true/false value to let caller know if lock was successful. Capture class is truthy/falsy to help with such kind of scenarios. 2) Just in case if you never lock a sequence of resources – see an update above. \$\endgroup\$ – Dmitry Nogin Jun 21 '16 at 7:11
2
\$\begingroup\$

Double Check Locking is not recommended. Specially in this case where the time spend inside the lock is minimal.

public bool TryCapture()
{
    if (!IsCaptured)
        lock (_lock)
            if (!IsCaptured)
                return IsCaptured = true;
    return false;
}

Keep it simple..

public bool TryCapture()
{
    lock (_lock)
        if (!IsCaptured)
            return IsCaptured = true;
    return false;
}

I like this approach in one of the answers the most. It allows the derived class to decide the locking pattern, so you can take advantage Monitor.TryEnter, rather than being held hostage of the lock as designed in TryCapture.

public abstract class Capture : IDisposable
{
    public static implicit operator bool(Capture capture) =>
        capture.Active;

    protected abstract bool Active { get; }
    public abstract void Dispose();
}

If the order of consumers that require exclusive access is important and needs to be fair, you'd need an active object that uses a queue to schedule access to the resource.

\$\endgroup\$
  • \$\begingroup\$ I am not convinced by the article. Author refers to Jon Skeet, but Jon is talking about singletons. But somehow both of you are right, I just looked at production code and there is exactly your version of TryCapture method. Probably mine had some issues, can't remember now it was 3 years ago ;) And I like your idea to use manager, it could be something like PriorityQueue used by wpf Dispatcher instead of simple lock, otherwise some thread are simply have no chance to capture busy resource. \$\endgroup\$ – Sinatr Jul 15 at 7:36
1
\$\begingroup\$

Should your object know if it is captured? Probably not. So we can factor out that responsibility.

Let’s define:

public abstract class Capture : IDisposable
{
    public static implicit operator bool(Capture capture) =>
        capture.Active;

    protected abstract bool Active { get; }
    public abstract void Dispose();
}

And

public class TargetCapture : Capture
{
    public TargetCapture(object target)
    {
        if (Monitor.TryEnter(target))
            Target = target;
    }

    protected override bool Active => Target != null;

    public override void Dispose()
    {
        if (!Active)
            return;

        Monitor.Exit(Target);
        Target = null;
    }

    object Target { get; set; }
}

And

public class MultiCapture : Capture
{
    public MultiCapture(params object[] targets)
    {
        Captures = from target in targets
                   select new TargetCapture(target);

        if (!Active)
            Dispose();
    }

    protected override bool Active => 
        Captures.All(c => c);

    public override void Dispose()
    {
        foreach (var capture in Captures)
            capture.Dispose();
    }

    IEnumerable<Capture> Captures { get; }
}

Demo:

    static void Main(string[] args)
    {
        var x = ...
        using (var capture = new TargetCapture(x))
            if (capture)
            {
                // ...
            }

        var y = ...
        var z = ...
        using (var capture = new MultiCapture(x, y, z))
            if(capture)
            {
                // ...
            }
    }

UPDATE

I would also define this:

public class RequiredCapture : MultiCapture
{
    public RequiredCapture(params object[] targets)
        : base(targets)
    {
        if (!Active)
            throw new InvalidOperationException();
    }
}

So it would be very easy to use:

        var r = ...
        using (new RequiredCapture(r))
        {

        }

The demo you requested:

class ProtectedSystem
{
    public int AccessCount { get; private set; }

    // throws if access is blocked
    public void IncrementAccess()
    {
        using (new RequiredCapture(this))
            AccessCount = AccessCount + 1;
    }

    // allowed to skip operation
    public bool TryIncrementAccess()
    {
        using (var capture = new MultiCapture(this))
        {
            if (capture)
                AccessCount = AccessCount + 1;

            return capture;
        }
    }

    // test method, please note consuming looks like 
    // an obvious defect in the design, as it opend 
    // to racing conditions
    public bool IsFreeAtTheMoment()
    {
        using (var capture = new MultiCapture(this))
            return capture;
    }
}
\$\endgroup\$
  • \$\begingroup\$ How another thread can check whenever x is captured? I can think of workarounds (storing instance of Capture in public static variable), but the idea of removing responsibility and making object unaware defeat this requirement. Object itself should implement Capturable or at least there should be some kind of layer (good idea? IDisposable isn't the right one though) through which object is accessed (and this layer provide capturing). Anyhow idea is interesting, thanks for insights. \$\endgroup\$ – Sinatr Jun 17 '16 at 7:17
  • \$\begingroup\$ @Sinatr By allowing to check current status, you are opening a lot of possibilities for racing conditions to come. It is usually implemented as an atomic operation - try to capture by another thread with immediate dispose of the capture if the capture itself is not necessary. \$\endgroup\$ – Dmitry Nogin Jun 17 '16 at 7:22
  • \$\begingroup\$ @Sinatr Please also see an update above. \$\endgroup\$ – Dmitry Nogin Jun 17 '16 at 7:30
  • \$\begingroup\$ Can you show an example how this solution ensure exclusive access to the object from different methods (e.g. multiple threads)? Do it with just one object using TargetCapture please. I am not able to follow your edit: I understand you see some problem with MultiCapture, but I do not understand which one. \$\endgroup\$ – Sinatr Jun 17 '16 at 7:35
  • \$\begingroup\$ @Sinatr Sure, here it is. \$\endgroup\$ – Dmitry Nogin Jun 17 '16 at 8:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.