I recently decided to refactor a small platform smoke test framework called Splinter I wrote awhile back. Aside from applying lessons learned since instantiation and pulling in the latest Node LTS release, I wanted to cleanup code resembling callback hell by using async/await. It's practically 2020 I thought, promises aren't exactly new, so this will be easy!
I soon discovered good intentions line the road from callback hell... Splinter simply executes minimalistic CRUD tests against MongoDB, MySQL, PostgreSQL, RabbitMQ and Redis. The idea is to quickly validate shared services are consumable. Refactoring MongoDB was quick and easy since I've done most Node.js projects using Mongo. Moving on to MySQL and PostgreSQL was also a delight, since the upstream client libraries from NPM have evolved over the years to make async/await a first class citizen.
Once I got to RabbitMQ, I hit a bit of a roadblock. My first instinct was to refactor using similar patterns as prior services, sprinkling
async/await pixie dust throughout the code and instantly reaping magical benefits. As naive as it sounds, that mostly worked... mostly. I hit a roadblock with channel.publish and channel.consume which weren't willing to let me await as expected. Who says the machines aren't in control?
Stepping back a bit, this all started with the following callback hell (which also contained a number of other code smells we'll address as we go along):
Pardon the first-pass code from a just-get-it-working hacking session now a couple years old... but even setting aside obvious smells, the never ending indentation makes my eyes gloss over. Worse, I wanted to make this test case (which simply used the default exchange) more realistic, adding even more callbacks! 🤯
How do we improve this? As stated above, a goal was refactoring these tests to use
async/await. Let's take a naive first attempt at that by simply replacing nested callbacks with
async/await goodness wrapped in
To keep this article from being even longer, I combined several steps... I don't want to get too far into the weeds, but to follow along note that we've:
- Refactored connection handling into a separate file
- Started using a non-default exchange and binding our queue
- Added more cleanup code
- Refactored helper functions and simplified inputs
Even with more functionality, the code is a lot easier to read. Excessive indentation be damned! There is one problem: We can't actually
await the calls to
consume. Even with
amqplib's promise-friendly version (simply doing
require('amqplib/callback_api')), support is not complete as expected. Perhaps by design, I'm a lowly DevOps zealot that writes code as a hobby vs a RabbitMQ expert. It feels like I am missing something, or perhaps they don't want to tightly couple publishers and consumers even though it's convenient for my use case.
"When you're going through hell, keep on going." —Winston Churchill
One answer might be simply finding an alternate AMQP client library. Since
amqplib is the most populuar option on NPM and used in the official tutorial (albeit with callbacks), I did a bit of research and decided to artistically borrow (read: steal) ideas found while Google Engineering... Specifically, I wrapped the problematic functions in promises, forcing them to do my bidding:
This worked! I did a late-night dance around the living room, and patted myself on the back for being so good at stealing (read: adopting) the work of others. There are at least two problems here. First, it's ugly. In all the other tests I refactored, moving to
async/await not only reduced indentation but also lines of code. This time we've added complexity with our helper functions, which sent me scurrying back to Google. To my surprise, I found simply rolling your own promises is a common anti-pattern. 😱
It seemed clear after more reading that I needed a better solution if I wanted to fit in with the cool kids that really know how to code. Luckily, the same site discussing the anti-pattern also built a solution (which has >18k stars on GitHub and supportive if not aged Stack Overflow threads).
Given my goal of building a lightweight test framework vs something more complex where Bluebird's advanced features might be an obvious win, I had concerns over using a sledgehammer to drive a tack nail... Since my refactoring effort included moving to Node 12.13 (LTS as of writing) and native promises have seen a lot of improvements, I considered giving util.promisfy a chance. After thinking that through, I would end up with an extra import (although of minimal native code),
promisfy wrappers, and some edge cases (not all functions support error-first callbacks). For those reasons, I decided to leave well enough alone... for now.
What would you have done here? Is there an obviously better way to approach this, or simply different options depending on context? Would reverting to the
promisfying everything have been a cleaner route? Is the fact I'm even thinking about this for such a simple use case proof that premature optimization really is the root of all evil?