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?

EDIT: Thanks for all the confirmation. I didn't realize this was a common idiom.

Answer 1

Would having an abstract base class be helpful? A method in the base class could call StartJumping(), the implementation of the abstract method that child classes would implement, and then call EndJumping().

Answer 2

1. I dont think you want to make those methods static
2. You need to check in the dispose if end jumping has alredy been called.
3. If I call start jumping again what happens?

You could use a reference counter or a flag to keep track of the state of the 'jumping'. Some people would say IDisposable is only for unmanaged resources but I think this is ok. Otherwise you should make start and end jumping private and use a destructor to go with the constructor.

class Jumper
{
    public Jumper() {   Jumper.StartJumping(); }
    public ~Jumper() {  Jumper.EndJumping(); }

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

Answer 3

If you need to control a scoped operation, I would add a method which take an Action<Jumper> to contain the required operations on the jumper instance:

public static void Jump(Action<Jumper> jumpAction)
{
    StartJumping();
    Jumper j = new Jumper();
    jumpAction(j);
    EndJumping();
}

Answer 4

I don't actually see this as an abuse of using; I'm using this idiom in different contexts and never had problems... especially given that using is only a syntactic sugar. One way I use it to set a global flag in one of third party libraries I use, so that the change is reverted when finishing operations:

class WithLocale : IDisposable {
    Locale old;
    public WithLocale(Locale x) { old = ThirdParty.Locale; ThirdParty.Locale = x }
    public void Dispose() { ThirdParty.Locale = old }
}

Note you don't need to assign a variable in using clause. This is enough:

using(WithLocale("jp")) {
    ...
}

I slightly miss C++'s RAII idiom here, where the destructor is always called. using is the closest you can get in C#, I guess.

Answer 5

I like this style and frequently implement it when I want to guarantee some tear down behaviour: often it is much cleaner to read than a try-finally. I don't think you should bother with declaring and naming the reference j, but I do think you should avoid calling the EndJumping method twice, you should check if it's already been disposed. And with reference to your unmanaged code note: it's a finalizer that's typically implemented for that (although Dispose and SuppressFinalize are typically called to free up the resources sooner.)

Answer 6

We have done almost exactly what you propose as a way to add method trace logging in our applications. Beats having to make 2 logging calls, one for entering and one for exiting.

Answer 7

Essentially the problem is:

• I have global state...
• and I want to mutate that global state...
• but I want to make sure that I mutate it back.

You have discovered that it hurts when you do that. My advice is rather than trying to find a way to make it hurt less, try to find a way to not do the painful thing in the first place.

I am well aware of how hard this is. When we added lambdas to C# in v3 we had a big problem. Consider the following:

void M(Func<int, int> f) { }
void M(Func<string, int> f) { }
...
M(x=>x.Length);

How on earth do we bind this successfully? Well, what we do is try both (x is int, or x is string) and see which, if any, gives us an error. The ones that don't give errors become candidates for overload resolution.

The error reporting engine in the compiler is global state. In C# 1 and 2 there had never been a situation where we had to say "bind this entire method body for the purposes of determining if it had any errors but don't report the errors". After all, in this program you do not want to get the error "int doesn't have a property called Length", you want it to discover that, make a note of it, and not report it.

So what I did was exactly what you did. Start suppressing error reporting, but don't forget to STOP suppressing error reporting.

It's terrible. What we really ought to do is redesign the compiler so that errors are output of the semantic analyzer, not global state of the compiler. However, it's hard to thread that through hundreds of thousands of lines of existing code that depends on that global state.

Anyway, something else to think about. Your "using" solution has the effect of stopping jumping when an exception is thrown. Is that the right thing to do? It might not be. After all, an unexpected, unhandled exception has been thrown. The entire system could be massively unstable. None of your internal state invariants might be actually invariant in this scenario.

Look at it this way: I mutated global state. I then got an unexpected, unhandled exception. I know, I think I'll mutate global state again! That'll help! Seems like a very, very bad idea.

Of course, it depends on what the mutation to global state is. If it is "start reporting errors to the user again" as it is in the compiler then the correct thing to do for an unhandled exception is to start reporting errors to the user again: after all, we're going to need to report the error that the compiler just had an unhandled exception!

If on the other hand the mutation to global state is "unlock the resource and allow it to be observed and used by untrustworthy code" then it is potentially a VERY BAD IDEA to automatically unlock it. That unexpected, unhandled exception might be evidence of an attack on your code, from an attacker who is dearly hoping that you are going to unlock access to global state now that it is in a vulnerable, inconsistent form.

Answer 8

I've commented on some of the answers here a bit about what IDisposable is and isn't, but I will reiterate the point that IDisposable is to enable deterministic cleanup, but does not guarantee deterministic cleanup. i.e. It's not guaranteed to be called, and only somewhat guaranteed when paired with a using block.

// despite being IDisposable, Dispose() isn't guaranteed.
Jumper j = new Jumper();

Now, I'm not going to comment on your use of using because Eric Lippert did a far better job of it.

If you do have an IDisposable class without requiring a finalizer, a pattern I've seen for detecting when people forget to call Dispose() is to add a finalizer that's conditionally compiled in DEBUG builds so that you can log something whenever your finalizer is called.

A realistic example is a class that's encapsulates writing to a file in some special way. Because MyWriter holds a reference to a FileStream which is also IDisposable, we should also implement IDisposable to be polite.

public sealed class MyWriter : IDisposable
{
    private System.IO.FileStream _fileStream;
    private bool _isDisposed;

    public MyWriter(string path)
    {
        _fileStream = System.IO.File.Create(path);
    }

#if DEBUG
    ~MyWriter() // Finalizer for DEBUG builds only
    {
        Dispose(false);
    }
#endif

    public void Close()
    {
        ((IDisposable)this).Dispose();
    }

    private void Dispose(bool disposing)
    {
        if (disposing && !_isDisposed)
        {
            // called from IDisposable.Dispose()
            if (_fileStream != null)
                _fileStream.Dispose();

            _isDisposed = true;
        }
        else
        {
            // called from finalizer in a DEBUG build.
            // Log so a developer can fix.
            Console.WriteLine("MyWriter failed to be disposed");
        }
    }

    void IDisposable.Dispose()
    {
        Dispose(true);
#if DEBUG
        GC.SuppressFinalize(this);
#endif
    }

}

Ouch. That's quite complicated, but this is what people expect when they see IDisposable.

The class doesn't even do anything yet but open a file, but that's what you get with IDisposable, and the logging is extremely simplified.

    public void WriteFoo(string comment)
    {
        if (_isDisposed)
            throw new ObjectDisposedException("MyWriter");

        // logic omitted
    }

Finalizers are expensive, and the MyWriter above doesn't require a finalizer, so there's no point adding one outside of DEBUG builds.

Answer 9

An alternative approach that would work in some circumstances (i.e. when the actions can all happen at the end) would be to create a series of classes with a fluent interface and some final Execute() method.

var sequence = StartJumping().Then(some_other_method).Then(some_third_method);
// forgot to do EndJumping()
sequence.Execute();

Execute() can chain back down line and enforce any rules (or you can build the closing sequence as you build the opening sequence).

The one advantage this technique has over others is that you aren't limited by scoping rules. e.g. if you want to build the sequence based on user inputs or other asynchronous events you can do that.

Answer 10

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.

Answer 11

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 12

With the using pattern I can just use a grep (?<!using.*)new\s+Jumper to find all places where there might be a problem.

With StartJumping I need to manually look at each call to find out if there is a possibility that an exception, return, break, continue, goto etc can cause EndJumping to not be called.