A simple question: What did you have for breakfast this morning?
Me? I had toast. 2 slices, white, with Flora and some great peach jam that we brought back from Spain.And a cup of tea, white with 2 sugars.
Another question: What didn’t you have for breakfast this morning?
Me? I didn’t have oatmeal. Or eggs. Or a hippopotamus, or lego, or chainsaw oil. The list of things I didn’t have is endless. Literally endless.
It’s impossible to fully scope the list of things that something doesn’t do, and yet as developers it’s important that we don’t just know what our code does, we also know what it doesn’t do.
This weekend I found and fixed a potentially nasty hole in the robotlegs SignalCommandMap – more details of the actual hole a little later, and if you’re using the utility already you should consider updating to get this fix.
I’d love to say that it was a piece of inspired insight on my part, but I think there’s more value in the truth – that it was a series of happy co-incidences.
Collaborating on the GuardedCommandMap
On reading about the GuardedCommandMap, Neil Manuell asked about a GuardedSignalCommandMap implementation. It’s something I’d been meaning to work on for a while, but hadn’t found the time. As it happens, we both started working on it simultaneously, but from opposite ends – he was hacking the code and, as is my habit, I began by writing tests. So, Neil sent me a pull request on github:
Its basically a cut and paste job, from your’s and Joel’s code, the only additional thing that I have done is to slightly restructure the SignalCommandMap so that we can share code.
no tests have been broken
I also implemented the mapGuardedSignalClass method, so tests will be need for that too.
I pulled his code (github is amazing isn’t it?) and used my normal process for retrospectively adding tests; I put early returns in the class methods under test, to confirm that the code is what’s causing them to pass.
Some tests passed, but I quickly ran into
InjectorErrors – the injector was complaining that it couldn’t unmap a mapping that didn’t exist.
Normally I would be adding a bunch of
traces at this point, but I’m a project-sprouts user and this was a Flash Builder project with some
ant tasks. I don’t have Flash Builder and I could only get the FlexUnit tests to run as a headless build, and then check the test output. I couldn’t seem to pick up the trace output using fdb or any of my normal tools, so I pushed on without
traces. This was important because it forced me to use tests rather than debugging to explore the error, and as a result I went down a different set of mental, and programming, paths.
The error was being thrown in an
unmapSignalValues function that Neil had broken out in refactoring the SignalCommandMap so that the two maps could share code. The SignalCommandMap tests were running without error, so I assumed it was something in the Guarded implementation causing the problems.
I did a quick project-wide search for the
unmapSignalValues method being used anywhere – hoping to play spot-the-difference – but I couldn’t find it. In fact, I couldn’t find anything in the SignalCommandMap related to unmapping the values between signal firings. But that made no sense – surely it was using something so clever that I couldn’t see it?
So I added a test that was designed to fail (to throw an error to be precise):
The test involved a
Command that required injector types that were dispatched by
Signal1, but the
Command was mapped to
Signal2. I was expecting a “Don’t know how to fulfil those injections!” error when the command was created, but I found that as long as I’d fired
Signal1 first, there was no error.
Peering down the hole
At first this doesn’t sound too serious – does it really matter if values persist between one Signal dispatch and the next?
Here is the potential problem:
- Assume we have a command with 2 injected dependencies A and B.
- Map a signalA that dispatches A to another command.
- Map a signalB that dispatches B to this command (the one with dependencies A and B).
- Dispatch signalA, followed by signalB – and the dependencies will be met using the two values.
- Dispatch signalB before signalA – and you’ll get a dependency injection error. In itself this isn’t too hard to diagnose.
The really hard to debug problem would arise if you had another signal, which also dispatched a value of the same type as signalA, and which fired between the two. If the value was a simple type – such as a Number – you’d find it very hard to pick this up. You can’t detect this with unit tests, only with end-to-end tests of complete paths through the software, and if you have user interaction it’s virtually impossible to get complete coverage for this.
You might argue that this is a ‘corner case’. The thing about corner cases is that if you’ve thought of them (and often we use this term when we realise we messed up by not thinking of something), and you’ve genuinely weighed up the pros and cons and decided to ignore them, you should close off the path. If it’s not safe to go into the corner, don’t let me go there!
Any other variation – ‘kinda-sort-of works but the data gets messed up’ – should be avoided. Yeah, this stuff is hard to get right – nobody ever told you that being a developer was going to be easy, did they?
Confirming the fix
A quick check with Till and Joel, and we confirmed that yes, the dispatched values were supposed to be unmapped between firings. The SignalCommandMap should not support situations where you’re relying on values that weren’t dispatched in this Signal. This was the case in the first release of the SignalCommandMap, but successive revisions had silently created the hole. Existing tests still passed, but we hadn’t tested for what the SignalCommandMap shouldn’t do.
Easy – just add the unmapping code… er…
Which is when the SignalCommandMap test-suite exploded. Puking the same ‘I can’t unmap something I haven’t mapped!’ error all over the place that had originally led to me finding the hole.
So I looked at the unmapping code that Neil had added in a little more detail.
Seems sensible enough. The problem is that mapValue and unmap aren’t actually symmetrical:
In most situations, if you try to pass an argument of the wrong type you get a nice compiler error. However, when the argument is a
String, and the value is coming out of an
Array (or any other ambiguous type reference), it will be coerced to
So – the compiler lets it pass, and the bug most likely shows up as one of the following:
- A weird value in a textfield, file or data where you output the
- A null pointer error, where the
Stringis used as a reference.
- Data overwriting errors – because if you’re using this value as a key in a
Object), objects of the same type will coerce to the same
Stringand map to the same entry in the
- An incorrect retrieval error – as a result of a combination of 2 and 3 – you get *some* data but it’s not the data you were expecting.
- A strongly typed, informative error because you’ve defended against a null pointer error / overwriting etc.
Fortunately in this case the SwiftSuspenders Injector gives us number 5 – and actually if I’d read the error in more detail I might have figured it out sooner, but then I would have missed the fact that the SignalCommandMap wasn’t unmapping values.
Four lessons from this episode:
1. Don’t use Strings for anything other than actual Text values if you can help it.
Strings are the worst kind of weak magic. While robotlegs includes the ability to make named injections we recommend that you avoid them any time it’s possible. I know that Till hates the named injections!
2. Aim for the impossible – complete test coverage
It’s probably impossible to completely capture the set of things your Class shouldn’t do, but if we at least make this a regular part of our TDD routine then we’ve got a hope of catching some of these hard-to-debug holes. We should aim to find the Oatmeal we didn’t eat, and not worry too much about the Hippopotamus.
3. Debugging gives you data – tests give you information
Instead of concerning ourselves with code (debugging) we should favour concerning ourselves with the behaviour of our code (through tests) – thanks to Neil for drawing this one out in our discussions about what happened. The tests for behaviour will still be relevant even when the code is refactored.
4. Collaboration pays dividends
Any time you get an opportunity to have someone other than the programmer specify the tests (not necessarily code them, but list them out) – take it. Between us, Joel, Till, Neil and myself have a smaller shared set of assumptions than any of us hold individually.
(Read the maps, maps and more maps post for detail on what it does.)