How can I structure a try-catch-finally block to handle errors inside finally?

Question

I've got a problem with making calls to a third-party C++ dll which I've wrapped in a class using DllImport to access its functions.

The dll demands that before use a session is opened, which returns an integer handle used to refer to that session when performing operations. When finished, one must close the session using the same handle. So I did something like this:

public void DoWork(string input)
{
    int apiHandle = DllWrapper.StartSession();

    try
    {
        // do work using the apiHandle
    }
    catch(ApplicationException ex)
    {
        // log the error
    }
    finally
    {
        DllWrapper.CloseSession(apiHandle);
    }
}

The problem I have is that CloseSession() sometimes causes the Dll in question to throw an error when running threaded:

System.AggregateException: One or more errors occurred. ---> System.AccessViolationException: Attempted to read or write protected memory. This is often an indication that other memory is corrupt.

I'm not sure there's much I can do about stopping this error, since it seems to be arising from using the Dll in a threaded manner - it is supposed to be thread safe. But since my CloseSession() function does nothing except call that Dll's close function, there's not much wiggle room for me to "fix" anything.

The end result, however, is that the session doesn't close properly. So when the process tries again, which it's supposed to do, it encounters an open session and just keeps throwing new errors. That session absolutely has to be closed.

I'm at a loss as to how to design an error handling statement that's more robust any will ensure the session always closes?

Answer 1

I would change the wrapper to include disposal of the external resource and to also wrap the handle. I.e. instead of representing a session by a handle, you would represent it by a wrapper object.

Additionally, wrapping the calls to the DLL in lock-statements (as @Serge suggests), could prevent the multithreading issues completely. Note that the lock object is static, so that all DllWrappers are using the same lock object.

public class DllWrapper : IDisposable
{
     private static object _lockObject = new object();

     private int _apiHandle;
     private bool _isOpen;

     public void StartSession()
     {
         lock (_lockObject) {
             _apiHandle = ...; // TODO: open the session
         }
         _isOpen = true;
     }

     public void CloseSession()
     {
         const int MaxTries = 10;

         for (int i = 0; _isOpen && i < MaxTries; i++) {
             try {
                 lock (_lockObject) {
                     // TODO: close the session
                 }
                 _isOpen = false;
             } catch {
             }
         }
     }

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

Note that the methods are instance methods, now.

Now you can ensure the closing of the session with a using statement:

using (var session = new DllWrapper()) {
    try {
        session.StartSession();
        // TODO: work with the session
    } catch(ApplicationException ex) {
        // TODO: log the error
        // This is for exceptions not related to closing the session. If such exceptions
        // cannot occur, you can drop the try-catch completely.
    }       
} // Closes the session automatically by calling `Dispose()`.

You can improve naming by calling this class Session and the methods Open and Close. The user of this class does not need to know that it is a wrapper. This is just an implementation detail. Also, the naming of the methods is now symmetrical and there is no need to repeat the name Session.

By encapsulating all the session related stuff, including error handling, recovery from error situations and disposal of resources, you can considerably diminish the mess in your code. The Session class is now a high-level abstraction. The old DllWrapper was somewhere at mid distance between low-level and high-level.