Anti-Patterns and Worst Practices – Heisenbugs

As I mentioned before, a Heisenbug occurs when trying to check the state of an object. These types of defects are common with concurrency issues are present. Microsoft has put out a library to help diagnose these problems: CHESS (http://msdn.microsoft.com/en-us/devlabs/cc950526.aspx). I’m not going to be addressing the concurrency issue, but the more common simpler types of Heisenbugs; the occurrences of these types of flaws in a “normal” application. When digging deeper into the issue, you may be causing more of a problem. This type of bug can manifest itself when not adhering to command-query separation guidelines (CQS). I’ve seen a prime example of this in code that performs calculations and validation inside a C# property setter.

Breaking CQS

When you first glance at this code, you may immediately identify that there’s a problem here. If you do, good for you; this is the kind of code that breaks CQS guidelines and can creep into your codebase if you’re not careful. (Another reason pair-programming and code reviews are extremely helpful)

public IShippingMethod ShippingMethod
{
get
{
return shippingMethod;
}
set
{
if(value.IsValidFor(this))
shippingMethod = value;
}
}

Why is this bad? If you haven’t spotted it already, it’s the conditional inside the the setter. If you’re a consumer of this API, you can set the ShippingMethod and immediately after, the value that you set is not the value currently assigned. This type of behavior inside a setter hasn’t been as common to me as it could be (thankfully), but I’ve seen other behaviors that can cause issues. Take this similar example:

public IShippingMethod ShippingMethod
{
get
{
return shippingMethod;
}
set
{
if(value.IsValidFor(this))
{
shippingMethod = value;
shippingCode = value.GenerateShippingCode(this);
}
else
{
shippingCode = ShippingCode.GROUND;
}
}
}

The setting of one property is deciding a change in another field. You could essentially set the ShippingCode, then set the ShippingMethod and have a different result in the ShippingCode you just assigned. If you want to make things worse, put that shippingCode assignment operator inside the get of the ShippingMethod property. Why would anybody do this? Because if it can be done, it will be done at some point. Talk about driving a developer crazy!?

Here is where CQS comes into play. In our query of what the Order’s ShippingMethod was, we were also commanding it to recalculate another value. (This example is based off of one that I’ve seen. You can replace the set property with a function that sets it, this provides a little bit more visibility to the developer that something is going to happen. In .Net the getters/setters are functions behind the scenes, but there is a bit of an understanding that these aren’t supposed to perform actions. See AutoProperties.) In the previous example I could show commands being performed inside the get, actually doing both a command and query, blatantly violating CQS, but I’ll spare you some pungent code smell.

Stack Memory Allocation

These types of defects can be avoided by being careful and understanding your code base. Not all the time do we have the luxury of working with code we’ve written. This similar type of behavior can exist in places where you’re not used it. Take the following test, it seems to make sense.

    [Test]
public void NotificationService_should_have_first_message_equal_to_one()
{
// Arrange
var service = new NotificationService();

// Act
var first = service.GetNextNotification();

// Assert
Assert.That(first.Message, Is.EqualTo("one"));
}
}

This test passes with no problems if you run it straight through without examining it. However, when you step through the code with the debugger turned on:

debugger

With the highlighted text above, I added a QuickWatch to determine what the value will be when calling the GetNextNotification function on NotificationService. My test was passing, it should return the value I’m asserting below; it should return the string “one”:

quickWatch

In this screen shot of the QuickWatch dialog, you can see that the value of the Notification’s Message property is “two”. Why on earth would it be different this time around? As small as this example is, it proves that Heisenbugs can most certainly exist in very simple environments. Why does this exist? Well, in this example, it’s due to poor implementation. You may have spotted it already with the name of my GetNextNotification function. The Next is the giveaway, I could have omitted the Next in the function name and made it even harder to figure out, it’s returning the top of a Stack<string> collection. The debugger is helping break this (by the debugger examining it, I’m getting commands & queries, thus unexpected behavior).

public Notification GetNextNotification()
{
return stack.Pop();
}

Dealing with Deferred Execution

The last type of Heisenbug I’d like to briefly touch on is a side effect of some newer functionality of the .Net runtime, deferred execution.

Deferred execution means that the evaluation of an expression is delayed until its realized value is actually required. Deferred execution can greatly improve performance when you have to manipulate large data collections, especially in programs that contain a series of chained queries or manipulations. In the best case, deferred execution enables only a single iteration through the source collection. (From MSDN)

I’m not suggesting that deferred execution is a Heisenbug, just that you can get some unexpected behavior when dealing with it. If you check out Chris Williams’ example you’ll see a nice quick example of how this can be “weird”. Here’s the C# version of his example:

var filter = "System";
var query = from a in AppDomain.CurrentDomain.GetAssemblies()
where a.GetName().Name.Contains(filter)
select a.GetName().Name;
//filter = "Xml";

When you’re dealing with LINQ or yield return, just keep in mind what’s going on and you should be fine. The unexpected behavior in my experience is most noticeable when testing with the debugger, all the more reason to avoid it. ;)

[See more Anti-Patterns and Worst Practices]

Related:

Post Footer automatically generated by Add Post Footer Plugin for wordpress.

About Chris Missal

Oh hey, I'm a Senior Consultant for Headspring in Austin, TX. I've been working in software professionally since 2006 and I really, really love it. I'm mostly in the Microsoft world, but enjoy building computer things of all sorts (to be vague). When I'm not slinging code, I'm probably out and about slinging discs, bowling balls, or good beer with great friends.
This entry was posted in Best Practices, Design Principles, Legacy Code, Testing. Bookmark the permalink. Follow any comments here with the RSS feed for this post.
  • http://www.lostechies.com/members/colinjack/default.aspx colinjack

    “If you do, good for you; this is the kind of code that breaks CQS guidelines and can creep into your codebase if you’re careful. ”

    I’m missing something, why do you say this is breaking CQS?

    As you say later if it was in the getter that’d be different, and its not great code either way, but I’m missing why its a CQS violation.

  • http://www.lostechies.com/members/chrismissal/default.aspx Chris Missal

    @Colin

    My mistake, if you’re *not* careful, thanks. :)

  • Joe Moore

    Maybe Chris considers “value.IsValidFor(this)” a query? It certainly does appear to query the current object, though a lot of my methods would also fail CQS horribly for the same reasons.

    The first couple examples smell regardless because they fail the Rule of Least Surprise.

  • http://www.lostechies.com/members/chrismissal/default.aspx Chris Missal

    @Colin

    I should have shown the bad code I wanted to avoid altogether, logic inside the getter (code shown is of the object being called). This would be breaking CQS. I opted to show more realistic code that does some work inside the setter. This is more common in my experience, but in my opinion is just as bad as putting command logic in the getter. Getters/setters serve as a way of, just that, getting values or setting values. When they do more than what is on the surface there are usually problems. While you could consider my setter example above in no way breaking CQS, I’d like to point out that it isn’t as discoverable as it should be as @Joe also points out. Hopefully this clears up some ambiguity.

  • http://www.lostechies.com/members/colinjack/default.aspx colinjack

    @Joe
    Its a query but it doesn’t mean we’re breaking tell dont ask. Agree its not great code, I wouldn’t put this sort of code inside a setter for a start, but I just didn’t see it as particularly violating CQS is all.

    @Chris
    Definitely that makes perfect sense and agree on getters/setters.

  • http://jystic.com Jacob Stanley

    Aren’t methods with semantics like GetNextNotification() required in a concurrent environment? That is, an environment where you need to atomically execute a command as the result of a query.

    How would you handle this kind of operation in a concurrent system without breaking the rules?

  • http://www.lostechies.com/members/chrismissal/default.aspx Chris Missal

    @Jacob

    That’s a very good point. While in my example the name of the method was a giveaway as to what was going on, there’s nothing wrong with that name. Since I wasn’t doing concurrent operations in my sample code, the name seemed to give something away. If you are doing this type of transaction, something along the lines of a Unit of Work implementation is probably more suited to your needs. Really, anything transactional could encounter situations like this. I’ll be honest, I don’t work in these types of environments, so I don’t always keep them in mind.

  • Sorin

    What do you think about something like:

    private int? _cachedValue = null;
    public int CachedValue
    {
    get {
    if(!_cachedValue.HasValue)
    _cachedValue = repository.GetValue();
    return _cachedValue.Value;
    }
    }


    private void ResetCache()
    {
    _cachedValue = null;
    }