C#: Refactoring an oversized try/catch/finally

Question

Recently I've come to discover that I've inherited one of the internal auxilliary programs used. I've made a few minor fixes and features to improve it in the past, but now I've been given a major feature that requires significant overhaul in order to implement.

This code has a recurring issue with overzealous try/catch patterns, catching all exceptions and writing them to log before quietly continuing. I've been working around these to avoid accidentally breaking anything, only narrowing them if I know all exceptions that could plausibly be thrown within the try block.

However, one occurrance includes a sizeably meaty finally block. It looks approximately like this:

void BigMethod() {
    bool stuff_found = false;
    bool lockTaken = false;
    // other locals that are referenced in finally block...
    try {
        Monitor.TryEnter(m_lock, 500, ref lockTaken);
        if (!lockTaken) {
            Logger.WriteLine("Failed to acquire lock");
            return;  // This used to be a throw-as-goto
        }

        if (m_queue.TryDequeue(out StuffInfo inf))
            return;
        stuff_found = true;
        // ...
        // 200 lines of BigMethod stuff, down from previously 350 lines
        // This includes creating some files and dirs, and several returns
        // that used to be throw-as-goto
    } catch (Exception _e) {
        Logger.WriteLine("Error{0}", _e);
        // This was clearly originally intended to be a catch-as-label but
        //   I'm unsure what I'd break if I were to remove it.
    } finally {
        if (stuff_found) {
            try {
                // Delete certain leftover files from outer try
            } catch (Exception e) {
                Logger.WriteLine("Error:{0}", e);
            }
            // PROBLEM: More stuff that could also plausibly throw,
            // including logging and sending an email of the failure
        }

        if (lockTaken)
            Monitor.Exit(m_lock);  // FIXME: This might get skipped
    }
}

The problem is that this finally block mixes together synchronization behavior (lock taking and releasing), file cleanup, error handling, and conclusion of the results of BigMethod, all at once.

The most immediate problem here is if any of the code at the PROBLEM: comment throws, the lock is never released.

Monitor.Exit needs to be called if lockTaken is true, no matter what. It also has to occur after the PROBLEM lines and activity, since this lock seems to prevent the creation and deletion of a file from multiple threads. (While I'm not 100% sure this method is called from multiple threads, it is a possibility.)

I can't just move the PROBLEM code into the try block, since it does some file cleanup after a failure, and moving it to try would mean that cleanup doesn't happen if something throws. Only thing I can think of to ensure the lock is consistently released is to wrap the contents of the finally in yet another try/finally, or the entire body of the method, and that just seems really icky.

How can I reasonably fix this (at the very least the uncertain Monitor.Exit) cleanly?

Answer 1

You could consider introducing a IDisposable-object to own whatever resource that needs cleanup, with a using statement or declaration to ensure cleanup. If each resource is in its own disposable object with a separate using, everything should be fine. Note that the lock could also be managed this way.

So you might have code looking something like:

using var myLock = AquireLock();
using var myResource1 = CreateResource1();
using var myResource2 = CreateResource2();
using var myResource3 = CreateResource3();

That guarantees that each resource is released, in the inverse order of creation.

Other types of refactoring, like moving code to separate methods or classes, might also be applicable. Especially if the logic is directly related to some resource you have a class for already.

In some cases it can be useful with a disposable object that takes an Action to run on dispose. Or to create a list of disposable objects that is disposed in a loop, with a try/catch to ensure a failure does not stop the disposal of any remaining objects.

If the whole idea is to loop over a queue, I would probably take a step back and consider if a ConcurrentQueue, or BlockingQueue would be more appropriate. Or perhaps something like DataFlow. The posted code looks kind of old, and quite a few things have improved with threading and parallelism in the last 10 years.

Answer 2

I haven't used C# so this is general advice...

I would start with some simple method extractions, so your code looks something like this:

ctxBefore = doStuffBeforeLock()

if (ctxBefore.isAbort())
    return

if (tryToLock()) {
    try {
        ctxInLock = doStuffInLock(ctxBefore)
    } finally {
        releaseLock()
    }

    doStuffAfterLock(ctxBefore, ctxInLock)

} else {
    doLockFailedStuff(ctxBefore)
}

Hence the purpose of this method is just to manage the lock - it doesn't do any business logic and it doesn't do any error handling. Except for the try/finally which doesn't have a catch clause - so all it will do is ensure the lock is released correctly. In short I am "separating the concern" of locking from the rest of the business logic.

By separating out the locking like this, each of the stuff methods should be much simpler with some business logic a bit of error handling and maybe an abort flag to break out early from the top level method.

I can't just move the PROBLEM code into the try block, since it does some file cleanup after a failure

This isn't true - it's perfectly possible to have a structure like:

fileHandleManager() {
    ctx = new Context()
    try {
        lockManager(ctx)
    } finally {
        cleanupFileHandles(ctx)
    }
}

lockManager(ctx) {
   locks = takeLocks()
   try {
      businessLogic(ctx)
   } finally {
      releaseLocks(locks)
   }
}

businessLogic(ctx) {
    try {
        openFileHandles(ctx)
        // Some logic
    } catch(e) {
        // Error handling
    }
}

In that you can create as many functions (each containing try/finally's) as you want and you can nest them in any order your want. The only complication is that if a deeply nested function creates an object that is needed someone else, you will need some kind of context object so that you can store it for later.

The actual logic of ensuring cleanups always happen, is enforced by the try/finally syntax.

TL;DR - You need to seperate out the concerns, so each function only handles one concern.