I’ve never really been a fan of C#’s object initializer syntax. Yes, it’s a little more convenient to write but it has a big downside which is it forces you to make your types mutable by default. Okay, that’s a bit strong, it doesn’t force you to do anything, but it does promote that way of thinking and allows people to take advantage of mutability outside the initialisation block [1].
This post is inspired by some buggy code I encountered where my suspicion is that the subtleties of the object initialisation syntax got lost along the way and partially constructed objects eventually found their way into the wild.
No Dragons Yet
The method, which was to get the next message from a message queue, was originally written something like this:
Message result = null;
RawMessage message = queue.Receive();
if (message != null)
{
result = new Message
{
Priority = message.Priority,
Type = GetHeader(message, “MessageType”),
Body = message.Body,
};
}
return result;
This was effectively correct. I say “effectively correct” because it doesn’t contain the bug which came later but still relies on mutability which we know can be dangerous.
For example, what would happen if the GetHeader() method threw an exception? At the moment there is no error handling and so the exception propagates out the method and back up the stack. Because we make no effort to recover we let the caller decide what happens when a duff message comes in.
The Dragons Begin Circling
Presumably the behaviour when a malformed message arrived was undesirable because the method was changed slightly to include some recovery fairly soon after:
Message result = null;RawMessage message = queue.Receive();
if (message != null)
{
try
{
result = new Message
{
Priority = message.Priority,
Type = GetHeader(message, “MessageType”),
Body = message.Body,
};
}
catch (Exception e)
{
Log.Error(“Invalid message. Skipping.”);
}
}
return result;
Still no bug yet, but that catch handler falling through to the return at the bottom is somewhat questionable; we are making the reader work hard to track what happens to result under the happy / sad paths to ensure it remains correct under further change.
Object Initialisation Syntax
Before showing the bug, here’s a brief refresher on how the object initialisation syntax works under the covers [2] in the context of our example code. Essentially it invokes the default constructor first and then performs assignments on the various other properties, e.g.
var __m = new Message();__m.Priority = message.Priority;
__m.Type = GetHeader(message, “MessageType”);
__m.Body = message.Body,
result = __m;
Notice how the compiler introduces a hidden temporary variable during the construction which it then assigns to the target at the end? This ensures that any exceptions during construction won’t create partially constructed objects that are bound to variables by accident. (This assumes you don’t use the constructor or property setter to attach itself to any global variables either.)
Hence, with respect to our example, if any part of the initialization fails then result will be left as null and therefore the message is indeed discarded and the caller gets a null reference back.
The Dragons Surface
Time passes and the code is then updated to support a new property which is also passed via a header. And then another, and another. However, being more complicated than a simple string value the logic to parse it is placed outside the object initialisation block, like this:
Message result = null;RawMessage message = queue.Receive();
if (message != null)
{
try
{
result = new Message
{
Priority = message.Priority,
Type = GetHeader(message, “MessageType”),
Body = message.Body,
};
var str = GetHeader(message, “SomeIntValue”);
if (str != null && TryParseInt(str, out var value))
result.IntValue = value;
// ... more of the same ...
}
catch (Exception e)
{
Log.Error(“Invalid message. Skipping.”);
}
}
return result;
Now the problems start. With the latter header parsing code outside the initialisation block result is assigned a partially constructed object while the remaining parsing code runs. Any exceptions that occur [3] mean that result will be left only partially constructed and the caller will be returned the duff object because the exception handler falls out the bottom.
+1 for Tests
The reason I spotted the bug was because I was writing some tests around the code for a new header which also temporarily needed to be optional, like the others, to decouple the deployments. When running the tests there was an error displayed on the console output [4] telling me the message was being discarded, which I didn’t twig at first. It was when I added a retrospective test for the previous optional fields and I found my new one wasn’t be parsed correctly that I realised something funky was going on.
Alternatives
So, what’s the answer? Well, I can think of a number of approaches that would fix this particular code, ranging from small to large in terms of the amount of code that needs changing and our appetite for it.
Firstly we could avoid falling through in the exception handler and make it easier on the reader to comprehend what would be returned in the face of a parsing error:
catch (Exception e){
Log.Error(“Invalid message. Skipping.”);
return null;
}
Secondly we could reduce the scope of the result variable and return that at the end of the parsing block so it’s also clearer about what the happy path returns:
var result = new Message{
// . . .
};
var str = GetHeader(message, “SomeIntValue”);
if (str != null && TryParseInt(str, out var value)
result.IntValue = value;
return result;
We could also short circuit the original check too and remove the longer lived result variable altogether with:
RawMessage message = queue.Receive();if (message == null)
return null;
These are all quite simple changes which are also safe going forward should someone add more header values in the same way. Of course, if we were truly perverse and wanted to show how clever we were, we could fold the extra values back into the initialisation block by doing an Extract Function on the logic instead and leave the original dragons in place, e.g.
try{
result = new Message
{
Priority = message.Priority,
Type = GetHeader(message, “MessageType”),
Body = message.Body,
IntValue = GetIntHeader(message, “SomeIntValue”),
// ... more of the same ...
};
}
catch (Exception e)
{
Log.Error(“Invalid message. Skipping.”);
}
But we would never do that because the aim is to write code that helps stop people making these kinds of mistakes in the first place. If we want to be clever we should make it easier for the maintainers to fall into The Pit of Success.
Other Alternatives
I said at the beginning that I was not a fan of mutability by default and therefore it would be remiss of me not to suggest that the entire Message type be made immutable and all properties set via the constructor instead:
result = new Message(
priority: message.Priority,
type: GetHeader(message, “MessageType”),
body: message.Body,
IntValue: GetIntHeader(message, “SomeIntValue”),
// ... more of the same ...
);
Yes, adding a new property is a little more work but, as always, writing the tests to make sure it all works correctly will dominate here.
I would also prefer to see use of an Optional<> type instead of a null reference for signalling “no message” but that’s a different discussion.
Epilogue
While this bug was merely “theoretical” at the time I discovered it [5] it quickly came back to bite. A bug fix I made on the sending side got deployed before the receiving end and so the misleading error popped up in the logs after all.
Although the system appeared to be functioning correctly it had slowed down noticeably which we quickly discovered was down to the receiving process continually restarting. What I hadn’t twigged just from reading this nugget of code was that due to the catch handler falling through and passing the message on it was being acknowledged on the queue twice –– once in that catch handler, and again after processing it. This second acknowledgment attempt generated a fatal error that caused the process to restart. Deploying the fixed receiver code as well sorted the issue out.
Ironically the impetus for my blog post “Black Hole - The Fail Fast Anti-Pattern” way back in 2012 was also triggered by two-phase construction problems that caused a process to go into a nasty failure mode, but that time it processed messages much too quickly and stayed alive failing them all.
[1] Generally speaking the setting of multiple properties implies it’s multi-phase construction. The more common term Two-Phase Construction comes (I presume) from explicit constructor methods names like Initialise() or Create() which take multiple arguments, like the constructor, rather than setting properties one-by-one.
[2] This is based on my copy of The C# Programming Language: The Annotated Edition.
[3] When the header was missing it was passing a null byte[] reference into a UTF8 decoder which caused it to throw an ArgumentNullException.
[4] Internally it created a logger on-the-fly so it wasn’t an obvious dependency that initially needed mocking.
[5] It’s old, so possibly it did bite in the past but nobody knew why or it magically fixed itself when both ends where upgraded close enough together.
No comments:
Post a Comment