5

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?

6
  • 2
    More a side-note than an answer, so written as comment: If you are planning a significant overhaul, then you should with priority fix the lack of communicating failure. If BigMethod doesn't successfully complete its job, it just writes something into a log, but there's no way for the caller to know about success or failure. The original author probably didn't embrace that exceptions were invented exactly for that purpose, as a means to communicate failure to the caller. So, I'd recommend to generally not catch exceptions, or at least to re-throw in a catch block. Oct 24, 2023 at 7:06
  • 3
    Logging is a very basic feature you should be able to rely on. Not sure it's worth trying every time. Email exceptions could be caught higher up the call stack in a catch-all clause. Oct 24, 2023 at 7:12
  • I've been watching the log output produced from this method for a while, the middle catch is probably safe to remove, so long as I double-check there's a higher catch up the stack. My concern was more that it might cause unexpected behavior in one of the callers up the stack. Thanks.
    – rdi_pck
    Oct 24, 2023 at 16:30
  • 2
    ... of bullet points that describe the high-level operation ("do this, do that", but in terms of the logic flow, not in terms of manipulating variables and calling libraries and frameworks). From there, you can figure out what to do next, cause now you have an overview of the overall structure. See which of those calls actually require the try block. Look for methods that look the same or almost the same and try to DRY them up. Extract some of those methods into separate special-purpose, narrowly focused classes if that helps you to shorten and simplify the code, or even delete some of it. 2/2 Oct 24, 2023 at 20:42
  • 1
    This reminds me of the 1000-line do-loop that I inherited 25 years ago.
    – RonJohn
    Oct 25, 2023 at 3:42

2 Answers 2

14

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.

6
  • 3
    +1 OP needs a class SelfReleasingMonitor: IDisposable which encapsulates the lockTaken value & the monitor. It has a static SelfReleasingMonitor TryEnter()" method which returns SelfReleasingMonitor, and an Exit method which calls monitor.Exit(I) if lockTaken is true. (it also sets locktaken to false). The Dispose method simply calls Exit (Exit can safely be called multiple times). Oct 24, 2023 at 15:55
  • 4
    Note that the lock could also be managed this way. I actually did this for another project a few months ago that had concurrency issues. Looks like I'll be bringing it back here. Thank you.
    – rdi_pck
    Oct 24, 2023 at 16:07
  • As for purpose, yes, this method operates on a single retrieved value (a path to a .zip) pulled from a ConcurrentQueue. The method is run within a WinForms BackgroundWorker that's called every 30s by a Timer (supposedly disabling the Timer until completion). Even with a ConcurrentQueue, the lock may still be necessary though because the method operates on a specific set of prepared folders (separate from the ones deleted in the finally), and two iterations doing work on those folders at the same time would break the process.
    – rdi_pck
    Oct 24, 2023 at 16:17
  • 1
    @rdi_pck Sounds like a PeriodicTimer might be of use. The underlying timer is managed automatically, no need for manually starting and stopping, and there is no risk for concurrent iterations.
    – JonasH
    Oct 25, 2023 at 6:49
  • 1
    @rdi_pck I'm guessing you are using a Timers.Timer? If you use a Threading.Timer instead you can set it to trigger once only, and achieve much the same effect. You just need to reset it manually.
    – JonasH
    Oct 26, 2023 at 7:33
10

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.

2
  • 2
    This is the way: each "finally" block should have responsibility for as little as possible.
    – pjc50
    Oct 24, 2023 at 9:24
  • A finally block should make sure every code inside is executed (as little responsibility as possible obviously helps). I prefer when a language lets you write the cleanup code at the point of using the resource. C++ destructors, C# Idisposable, Swift “defer” that lets you say “take the lock now and release it later”. All things that work if you throw exceptions or return.
    – gnasher729
    Oct 25, 2023 at 8:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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