Tuesday, April 7, 2015

Sometimes the best code is that which is never written

Someone wrote some moderately over-complicated code to provide functionality in a service. That developer has moved on. A little time passes and suddenly there are a bunch of stack traces popping up in the logs indicating that a status message isn't getting published.
First reaction - dig into the error, figure out exactly what's wrong and fix it, ASAP!

Here's the error:  Invalid parameter: Message too long (Service: AmazonSNS; Status Code: 400; Error Code: InvalidParameter; Request ID: <redacted>)

Seems pretty straightforward; two seconds of Googling turns up that SNS messages have a limit of 256KB. Go into the appropriate class and add something detect how many bytes are in the message. Hmmm... maybe these messages are really large, and our app has a ton of threads - don't want to use up too much memory. A quick check on StackOverflow suggests using a CountingInputStream (there's one in the Apache commons-io, but it's also pretty trivial to write). Add it. Write a couple tests to make sure that it counts bytes correctly for UTF-8, UTF-16, etc... Looking good and have only spent 20mins so far fixing this problem.

Now that we can reliably detect the message size, we need to deal with ones that are too large for SNS... Uh-oh, the message body is actually something getting serialized into a JSON payload, we can't simply truncate or the message won't be valid JSON any more. This will require some thought, but I'm totally up for the challenge!

STOP! Take a step back from the technical details and think about what problem you're trying to solve.


Basically, the point of this code was to "make sure a notification goes out that something failed to process". There's nothing in there to suggest that the entire response needs to be serialized and sent on the wire (although that's what it was doing originally). Think to yourself, "what's the simplest thing that could work?". In this case, all that really needs to be sent is an ID and a status of success or failure:

{ id: 1234, status: "failure" }

There's NO way that message will ever approach 256KB, so do I need to worry about truncating this message? No. Looking back on it, did I need a CountingOutputStream even with the truncating solution? Probably not, it was a JSON payload in UTF-8; UTF-8 handles the ASCII range in 1 byte per character so simply getting the length of the String should have been "close enough".

In the end, I had to write a negligible amount of code to create that new JSON payload, and I removed a bunch of complexity from the original codebase along the way.

The take-away from this exercise? Step back and understand the problem-space from the start and resist the impulse to dive in and fix the errors immediately.

No comments: