2

Background

I am working on implementing a derived class of a database accessor that should only allow one thread at a time access to the database. But this question should be applicable to any form of single thread access with the Disposable pattern.

In Stephen Cleary's blog he shows how to implement a mutex with an IDisposable patern, this is almost the idea.

On MSDN's website they show how to use the IDisposable pattern in a derived class.

In this answer about best practices of SQlite db in android it is proposed to open the SQLite db, and never close it, but use a type of delegate to maintain concurrency; this I do not like, but it gave me the idea of what follows.

The program

The base class which throws exception if more than one thread tries to access it:

class BaseClass : IDisposable

The derived class which implements the lock:

class DerivedClass : BaseClass
{
     private static Semaphore _mutex = new Semaphore(1, 5);

     public DerivedClass
     {
         _mutex.WaitOne();
     }

     protected override void Dispose(bool disposing)
     {
         if (disposed)
             return;

Question

Where should the _mutex.Release() be called? Before or after the base.Dispose()?

i.e. option 1:

 _mutex.Release();
 disposed = true;
 base.Dispose(disposing);

or option 2:

 disposed = true;
 base.Dispose(disposing);
 _mutex.Release();

MSDN says to free any unmanaged objects before calling base.Dispose, which makes it feel that the release should also happen before, like option 1. And it's MSDN, I'm sure they know the best.

But, option 1 opens the door for a thread to access the base class before it is Disposed (at least it looks like it). Which makes me feel that option 2 is the correct implementation.

I have tried both implementations, and both work; but I am unsure which one will keep on working.

Can you please also explain why the one or the other.

Community
  • 1
  • 1
Barnstokkr
  • 2,904
  • 1
  • 19
  • 34
  • Do you want the mutex to be taken throughout the class'es lifetime? – i3arnon Jan 27 '15 at 08:32
  • Also, where does it say to call `base.Dispose` at the end? – i3arnon Jan 27 '15 at 08:36
  • @I3arnon I don't quite get what you mean. But the mutex should always exist through the lifetime of the app to allow multiple instances of DerivedClass to be created and always allow only one thread access. The class is always used in the `using` context. – Barnstokkr Jan 27 '15 at 08:38
  • @I3arnon if you look at the implementation on MSDN, it looks as if the Dispose structure wants you to dispose last. – Barnstokkr Jan 27 '15 at 08:41
  • I mean the you enter the mutex (which isn't a mutex, it's a semaphore) when you create an instance and leave it when dispose of it. which mean you're in the mutex throughout the instance's lifetime. – i3arnon Jan 27 '15 at 08:43
  • Why are you creating an semaphore and calling its `WaitOne` inside the class constructor? what purpose does that serve? – Yuval Itzchakov Jan 27 '15 at 08:46
  • @Yuval Itzchakov to block the creating thread if another is already using it. If you have a better solution I will really want to hear it, I personally don't like it very much. – Barnstokkr Jan 27 '15 at 08:49
  • 1
    Would you want your application to hang on an instance creation such as `var d = new Derived`? You should take it out of the constructor and use it only when invoking one of the `Derived` classes methods. – Yuval Itzchakov Jan 27 '15 at 08:51
  • @Yuval Itzchakov I'll think about that, but I'm not sure if it will work, but it's worth investigating. Thank you. – Barnstokkr Jan 27 '15 at 09:10

3 Answers3

2

You should go with the second option of first disposing the base class and then releasing the mutex.

If you don't you're leaving the door open for concurrency, even if it's a very small one.

There's no rule about the order in which the different Dispose method should execute. In the MSDN article, they do call base.Dispose at the end, but it's just one of the options.

i3arnon
  • 113,022
  • 33
  • 324
  • 344
2

I beg to differ slightly from l3arnon's answer.
Although there is no rule; it is always advisable that you dispose classes in the reverse order of instantiation, by which you would be maintaining the integrity of the Dispose method of the derived classes.
Anyway in your code you are not using a named Mutex and hence the scope of the Mutex is within the scope of the DerivedClass, even otherwise the BaseClass should be independent of the child classes or commands from child instances in this case the base.Dispose() call. I'd always recommend the below pattern from MSDN

protected override void Dispose(bool isDisposing){
 if(isDisposing && !IsDisposed){
  //Release all resources and mutex here
  IsDisposed = true;
 }
 base.Dispose(isDisposing);
}
Vignesh.N
  • 2,618
  • 2
  • 25
  • 33
  • This is interesting, don't you think that this will introduce concurrency issues? – Barnstokkr Jan 27 '15 at 10:39
  • 1
    it will not; but then again you shouldn't be Diposing resources when you are not sure if resources are still held/used from other threads. It that is the case then the problem is not with the Disposing but how different threads are accessing the resources. Developer needs to make sure all operations from all threads are completed before you can Dispose the instance. – Vignesh.N Jan 27 '15 at 11:28
2

I only post because I so violently disagree with the upvoted answer :) Threading racing bugs are the nastiest bugs you can ever deal with. Exceedingly hard to debug, Murphy's law dictates that they only ever occur when you are not debugging your app and happen randomly only once a month. You can never test your app sufficiently to be 100% sure that you don't have such a bug, you need all the help you can get to detect them.

Disposing an object while another thread is still using it is such a mishap, it is not uncommon. It is therefore imperative that you make it as likely as possible that you get an exception when this happens. Super-duper important to dispose the Mutex first, that maximizes the odds for getting an ODE. Now you know you have a bug.

Not the only reason, in general you have little knowledge of the base class' Dispose() method. It may throw. You don't really want to "leak" the mutex when that happens, that just causes more trouble when code beyond your control is unwise enough to catch such an exception.

Two pretty decent reasons to follow the Microsoft guidance. Every Disposable(bool) override in the .NET Framework calls the base.Dispose() method last.

Hans Passant
  • 922,412
  • 146
  • 1,693
  • 2,536
  • Quoting: `"Disposing an object while another thread is still using it "` -this should probably be common sense, but it's not; now everything makes sense! This explains perfectly the "why"! – Barnstokkr Jan 27 '15 at 11:54
  • @Barnstokkr Just to clarify: No one mentioned disposing an object being used by another thread. The mutex here is static (which I don't recommend, but that's irrelevant to the question). `base.Dispose` may throw (although it shouldn't) just like any other method and the solution is a `try-finally` block. If the point of this class is to limit concurrency on its base class (again, not what I'd recommend) this misses it. – i3arnon Jan 27 '15 at 12:21