JavaScript Race Conditions

Yesterday I was rudely reminded that just because Node.js is single threaded doesn’t mean you don’t have to worry about race conditions.

Our architecture is heavily message-oriented and the code that handles ACKing messages looks something lite this. The idea is that application code can call either acknowledge(), which acknowledges that the task has been handled, or retry(), which means that the we should retry (a copy of) the task at a later time and then acknowledge that the original task has been dealt with. It should only be possible to retry or acknowledge a task once, which is why we keep track of the state in a variable @resolved.

class Ack
    constructor: (@task, @queue) ->
        @resolved = false

    acknowledge: =>
        unless @resolved
            @task.acknowledge()
            @resolved = true

    retry: =>
        unless @resolved
            @queue.scheduleRetry(@task)
            .then =>
                @acknowledge()

Can you spot the error? It took me a while to realize that even though retry() makes sure that @resolved hasn’t been set and we call acknowledge() to ACK the task and set @resolved immediately afterwards, that actually happens asynchronously. We are using the Q promise library here, which means that if retry() is called repeatedly within the same process tick, then scheduleRetry() will almost certainly be called multiple times.

I think promises are wonderful, but the fact that they make asynchronous code look synchronous can really trick you. I’m pretty sure I wouldn’t have made this mistake if I’d written this piece in the regular old callback-oriented style.