-1

I am in a situation where cleaning up unmanaged resources is a critical section. To solve this, I changed this...

void SomeMethod()
{
    //work
    using (var doc = SpreadsheetDocument.Open(results.FileName, true))
    {
        //use doc.
    }
}

to this...

public static readonly object Locker = new object();
void SomeMethod()
{
    //work
    {//scoping doc
        var doc = SpreadsheetDocument.Open(results.FileName, true);
        try
        {
             //use doc
             //At some point wrapping a critical section via lock(Locker) 
        }
        finally
        {
            lock (Locker)
            {
                if (doc != null) ((IDisposable)doc).Dispose();
            }
        }
    }
}

Which, I believe, is an ugly and brittle solution. So, I changed it to the following...

public static readonly object Locker = new object();
void SomeMethod()
{
    //work
    CustomUsingWithLocker(SpreadsheetDocument.Open(results.FileName, true), Locker, doc =>
    {
        //use doc
        //At some point wrapping a critical section via lock(Locker) 
    });
}

public static void CustomUsingWithLocker<T>(T resource, object locker, Action<T> body)
    where T : class, IDisposable 
{
    try
    {
        body(resource);
    }
    finally
    {
        lock (locker)
        {
            if (resource != null) resource.Dispose();
        }
    }
}

Is this custom solution robust? Can I improve on it? Is it guarantied to release any unmanaged resources like the built in Using statement?

Onosa
  • 1,275
  • 1
  • 12
  • 28
  • Why are you protecting doc with a lock when it is not a shared object? – Polyfun Jun 20 '13 at 15:08
  • Inside of my customUserWithLocker, doc is used to create an excel document. If a different doc is disposing during this time, a timeout exception is thrown. By locking the excel document creation and locking the dispose call, the timeout expectation never occurs. I believe file streams are shared between docs. – Onosa Jun 20 '13 at 17:18
  • It is a horrible idea to use Office Interop from ASP.NET or another server technology. These APIs were written for use in a desktop application, for automating Office (a suite of desktop applications). Server applications are different in many ways that make it a very, very bad idea to use Office Interop in them. It's also unsupported by Microsoft, and may violate your Office license. See [Considerations for server-side Automation of Office](http://support.microsoft.com/kb/257757) – John Saunders Jun 20 '13 at 18:24

1 Answers1

0

The premise here seems to be that Dispose is called multiple times from multiple threads. If that's the case, change that premise. Even if doc is passed to...somewhere in which it's used in another thread, that other place ins't responsible for disposing it, and shouldn't be disposing it. Find where it's being disposed besides the end of the using and change it to not do that.

If Dispose isn't being called elsewhere, and you simply have a non-thread safe Dispose method that shouldn't be accessed from multiple threads (even though your code currently is smart enough to not do that) and you're just trying to apply good defensive programming, I think it's safe to say that this isn't needed here. It's a valid constraint of this method, in this particular context, to say that only one thread should be responsible for disposing of the object.

Servy
  • 202,030
  • 26
  • 332
  • 449
  • I believe `Dispose` is only being called once, in the `finally` section. If one doc is in a critical section (specifically, when it creates an excel document), and another doc is disposing, an exception` is thrown. I do not get an exception when I wrap the critical section and the dispose by a common mutex of some sort. So, I believe you are right in that the `Dispose` is not thread safe. I think the doc is sharing file streams (poorly). The overarching process is wrapped by a `SemaphoreSlim(2)` and creating a document occurs near disposing, so that's why they conflict so often. – Onosa Jun 20 '13 at 17:30