current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

I have a lock class, that handels a database lock. It's not important how.
It is used in the beginning of a large operation, here is how it is used today:

public void LargeOperation()
{
    try
    {           
        MyLock.DoLock("SomeId");
        // Do lots of stuff that might thorw exceptions
    }
    finally
    {
        MyLock.ReleaseLock("SomeId");
    }
}

I would like to use it like this instead:

public void LargeOperation()
{
  using(new MyLock("someId"))
  {
    // Do lots of stuff that might thorw exceptions
  }
}

this works fine, but im not sure wether it is a bad idea. I have implemented IDisposable on my class MyLock but it doesn't dispose.. instead it releases the lock. Here is how the class look:

public class MyLock : IDisposable
{
    private string _id;
    public MyLock(string id)
    {
       DoLock(id);
    }

    public static void DoLock(string id)
    {
      //..code omitted
    }

    public static void ReleaseLock(string id)
    {
      //..code omitted
    }

    public void Dispose()
    {
      ReleaseLock(_id);
    }
}

Is this abuse of the IDisposable interface?

share|improve this question

2 Answers 2

up vote 4 down vote accepted

The IDisposable was primarily added in order to be able to release acquired unmanaged resources in a deterministic fashion. However it also provides a nice way of releasing other resources in a deterministic fashion as you have found.

I would not say that this is an abuse of the disposable pattern: You acquire a lock which is a resource and you need to release it at a given point in time. For me this is good enough especially since you said it's some kind of db lock which I assume lives outside of the application (that is unmanaged for me). It provides you with native support through using to make sure it's released in case the executing code throws an exception.

However I don't particularly like your two public static methods on there. Now you all of a sudden have two interfaces into the class. If you want a factory like interface then make the DoLock method a proper factory method returning the lock which was acquired (in which case the ctor should become private):

public class MyLock : IDisposable
{
    private string _id;

    private MyLock(string id)
    {
        _id = id;
        DoLock();
    }

    public static MyLock Acquire(string id)
    {
        return new MyLock(id);
    }

    private void DoLock()
    {
        // get the lock
    }

    public void Release()
    {
        // release it
    }

    public void Dispose()
    {
        Release();
    }
}

While you could do without a public Release method having Dispose calling Release is semantically useful in case you can't use using and need to release the lock explicitly. In this case the code will be clearer when you call Release rather than Dispose. The .NET stream classes do a similar thing (where Dispose calls Close).

Some things to consider:

  • Add an "object is disposed" flag so you don't try to release the same lock twice.
  • Add a finalizer to try and release the lock in case it was not properly released. Add a Debug.Assert at least for that case. Note that adding a finalizer will make all MyLock objects go through the finalizer queue which will add overhead for the GC. You could wrap the finalizer into a #ifdef DEBUG to only do that in debug builds - depends how many of these locks you create (although I guess if you create lots of them you have another problem altogether).
share|improve this answer
    
This was great review :) thanks! I will implement the Acquire as you suggested. –  Jens Kloster Oct 28 '13 at 8:08
1  
I think that you want to have the finalizer in release builds too. If you log the fact that the finalizer was called, you're not actually hiding the bug and you're making the application behave better. –  svick Oct 28 '13 at 12:06
    
You should never throw an ObjectDisposedException when Dispose is called a second time. See msdn.microsoft.com/en-us/library/… –  Bort Oct 28 '13 at 12:21
    
@Bort: Good point. Although it probably only matters when you also implement a finalizer. –  ChrisWue Oct 28 '13 at 19:13
up vote 0 down vote

Following the guidelines for implementing the IDisposable interface i would say it depends on how you have implemented the MyLock class. If your Lock class uses unmanaged resources it is correct to implement the interface. But i would not implement the IDisposable interface to make coding easier.

Maybe you should use a CodeSnippets in Visual Studio to insert the necessary code to use your Lock class.

share|improve this answer
    
thanks for the review. You have a valid point (+1), however it is not the typing (of code) that my problem. I think it is easier to read the buisness code when locks (like mine) fill as little as possible. –  Jens Kloster Oct 28 '13 at 8:11

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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