Enforcing an "end" call whenever there is a corresponding "start" call

Question

Let's say I want to enforce a rule:

Everytime you call "StartJumping()" in your function, you must call "EndJumping()" before you return.

When a developer is writing their code, they may simply forget to call EndSomething - so I want to make it easy to remember.

I can think of only one way to do this: and it abuses the "using" keyword:

class Jumper : IDisposable {
    public Jumper() {   Jumper.StartJumping(); }
    public void Dispose() {  Jumper.EndJumping(); }

    public static void StartJumping() {...}
    public static void EndJumping() {...}
}

public bool SomeFunction() {
    // do some stuff

    // start jumping...
    using(new Jumper()) {
        // do more stuff
        // while jumping

    }  // end jumping
}

Is there a better way to do this?

Answer 1

I'm going to disagree with Eric: when to do this or not depends on the circumstances. At one point, I was reworking my a large code base to include acquire/release semantics around all accesses to a custom image class. Images were originally allocated in unmoving blocks of memory, but we now had the ability to put the images into blocks that were allowed to be moved if they hadn't been acquired. In my code, it is a serious bug for a block of memory to have slipped past unlocked.

Therefore, it is vital to enforce this. I created this class:

public class ResourceReleaser<T> : IDisposable
{
    private Action<T> _action;
    private bool _disposed;
    private T _val;

    public ResourceReleaser(T val, Action<T> action)
    {
        if (action == null)
            throw new ArgumentNullException("action");
        _action = action;
        _val = val;
    }

    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }

    ~ResourceReleaser()
    {
        Dispose(false);
    }

    protected virtual void Dispose(bool disposing)
    {
        if (_disposed)
            return;

        if (disposing)
        {
            _disposed = true;
            _action(_val);
        }
    }
}

which allows me to do make this subclass:

public class PixelMemoryLocker : ResourceReleaser<PixelMemory>
{
    public PixelMemoryLocker(PixelMemory mem)
        : base(mem,
        (pm =>
            {
                if (pm != null)
                    pm.Unlock();
            }
        ))
    {
        if (mem != null)
            mem.Lock();
    }

    public PixelMemoryLocker(AtalaImage image)
        : this(image == null ? null : image.PixelMemory)
    {
    }
}

Which in turn lets me write this code:

using (var locker = new PixelMemoryLocker(image)) {
    // .. pixel memory is now locked and ready to work with
}

This does the work I need and a quick search tells me I needed it in 186 places that I can guarantee won't ever fail to unlock. And I have to be able to make this guarantee - to do otherwise could freeze a huge chunk of memory in my client's heap. I can't do that.

However, in another case where I do work handling encryption of PDF documents, all strings and streams are encrypted in PDF dictionaries except when they're not. Really. There are a tiny number of edge cases wherein it is incorrect to encrypt or decrypt the dictionaries so while streaming out an object, I do this:

if (context.IsEncrypting)
{
    crypt = context.Encryption;
    if (!ShouldBeEncrypted(crypt))
    {
        context.SuspendEncryption();
        suspendedEncryption = true;
    }
}
// ... more code ...
if (suspendedEncryption)
{
    context.ResumeEncryption();
}

so why did I choose this over the RAII approach? Well, any exception that happens in the ... more code ... means that you are dead in the water. There is no recovery. There can be no recovery. You have to start over from the very beginning and the context object needs to be reconstructed, so it's state is hosed anyway. And by comparison, I only had to do this code 4 times - the possibility for error is way, way less than in the memory locking code, and if I forget one in the future, the generated document is going to be broken immediately (fail fast).

So pick RAII when you absolutely positively HAVE to have the bracketed call and can't fail. Don't bother with RAII if it is trivial to do otherwise.

Answer 2

Jeff,

what you're trying to achieve is generally referred to as Aspect Oriented Programming (AOP). Programming using AOP paradigms in C# is not easy - or reliable... yet. There are some capabilities built directly into the CLR and .NET framework that make AOP possible is certain narrow cases. For example, when you derive a class from ContextBoundObject you can use ContextAttribute to inject logic before/after method calls on the CBO instance. You can see examples of how this is done here.

Deriving a CBO class is annoying and restrictive - and there is another alternative. You can use a tool like PostSharp to apply AOP to any C# class. PostSharp is far more flexible than CBOs because it essentially rewrites your IL code in a postcompilation step. While this may seem a bit scary, it's very powerful because it allows you to weave in code in almost any way you can imagine. Here's a PostSharp example built on your use scenario:

using PostSharp.Aspects;

[Serializable]
public sealed class JumperAttribute : OnMethodBoundaryAspect
{
  public override void OnEntry(MethodExecutionArgs args) 
  { 
    Jumper.StartJumping();
  }     

  public override void OnExit(MethodExecutionArgs args) 
  { 
    Jumper.EndJumping(); 
  }
}

class SomeClass
{
  [Jumper()]
  public bool SomeFunction()  // StartJumping *magically* called...          
  {
    // do some code...         

  } // EndJumping *magically* called...
}

PostSharp achieves the magic by rewriting the compiled IL code to include instructions to run the code that you've defined in the JumperAttribute class' OnEntry and OnExit methods.

Whether in your case PostSharp/AOP is a better alternative than "repurposing" the using statement is unclear to me. I tend to agree with @Eric Lippert that the using keyword obfuscates important semantics of your code and imposes side-effects and semantic menting to the } symbol at the end of a using block - which is unexpected. But is this any different than applying AOP attributes to your code? They also hide important semantics behind a declarative syntax ... but that's sort of the point of AOP.

One point where I whole-heartedly agree with Eric is that redesigning your code to avoid global state like this (when possible) is probably the best option. Not only does it avoid the problem of enforcing correct usage, but it can also help avoid multithreading challenges in the future - which global state is very susceptible to.