Const Incorrectness

Recently, one of my work colleagues experienced a bug. So far so boring, but it was in such simple code that it just didn’t seem possible that there could be a bug lurking in it. A rough approximation of the code looked like this:

1
2
3
4
5
6
7
8
9
10
11
12
13
14

const subscribers = [];

subscribe = observer => subscribers.push(observer);

unsubscribe = observer => {
    const idx = subscribers.indexOf(observer);
    if (idx > -1) {
        subscribers.splice(idx, 1);
    }
}

notifySubscribers = data => {
    subscribers.forEach(subscriber => subscriber.notify(data));
}

I really do doff my cap to anyone who can see what the bug is. Can you see it? Have a think and if you need a hint, read on below. If you have worked out the answer, congratulations, your prize is you get to read a few paragraphs of text telling you what you already know.

A hint

Imagine how your code would look like if you subscribed to information about an object/some data and that object/data has been deleted or removed.

A heavy hint and the answer

What you would do in the case that the information you wanted to track has been deleted or removed would be to immediately unsubscribe from any further notifications. That would trigger the removal of your subscription mid-way through the notifySubscribers function iterating over the number of subscribers. In the code below I write a “one shot” class that will unsubscribe when it is first notified. The code is just to illustrate the bug, so it’s somewhat unrealistic in that it doesn’t do much beyond that.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17

class OneShot {
  constructor(name) {
    this.name = name;
  }
  notify(data) {
    console.log(this.name + ' notified of: ' + data);
    unsubscribe(this);
  }
};

oneshot1 = new OneShot('One shot obj 1');
oneshot2 = new OneShot('One shot obj 2');

subscribe(oneshot1);
subscribe(oneshot2);

notifySubscribers('data deleted');

Output:

1

> One shot obj 1 notified of: data deleted

So the bug is that you have two subscribers but only one gets notified because the subscribers list is being modified as it is being iterated over.

Taking a step back

The problem here is that the list of subscribers is being modified while other parts of the code are still using it.

To fix this you either want the data to be immutable so it can’t be modified or you want to use code that does not modify data – which is what you would have to do anyway if the data was immutable.

JavaScript doesn’t provide any built-in immutable data structures. You can freeze objects or use things like seamless-immutable that prevent modification of objects but do you really want the sweat beading on your brow every time you write code that modifies some data that may or may not have been frozen? I tend to think you don’t, and while some of my colleagues apparently do, the madness that ensues is probably worth covering in its own future post.

That leaves us with using code that does not modify the data. How can we have useful programs that don’t modify data? Rather than modify the subscribers data, instead we can change what subscribers points to, pointing it at new data which is different to the old data.

This simple change of approach means that you can safely unsubscribe to further notifications when you are notified because doing so won’t affect the list of subscribers that is being iterated over. The data being enumerated is still intact, and subscribers has just been changed to point at a different array.

The fixed code would look like this if we used Ramda (which we don’t – I’ll probably complain about that in detail some other time):

1
2
3
4
5
6
7
8
9
10
11
12
13

let subscribers = [];

const subscribe = observer => {
  subscribers = append(observer, subscribers);
}

const unsubscribe = observer => {
    subscribers = reject(equals(observer)), subscribers);
}

const notifySubscribers = data => {
    map(subscriber => subscriber.notify(data), subscribers);
}

I’m not going to evangelise for this style of code here, I’ve done that plenty in the past. The relevant point is that I had to make a change to replace the definition of subscribers to use let instead of const. I think this is better code but this style of coding is incompatible with const! In fact const seems to encourage us to use mutable data as it prevents you from re-assigning your variables to point at different data.

As ever, I find myself convinced that the typical mid-wit opinion on things are much less true than you would expect. You can see some such opinions presented as answers to this question on Stack Exchange: “const all the things!”, “Using the const keyword gives us more safety, which will surely lead to more robust applications.” and “I know that the majority of bugs that I see involve unexpected state changes. You won’t get rid of all of these bugs by liberally using const, but you will get rid of a lot of them!”. Note that this bug is a result of unexpected state changes, so I am not sure that const is really helping.

In summary, I think the way to avoid bugs is to avoid mutating data structures. Ideally your language would give you immutable data structures, but JavaScript does not and it seems fine to just treat the data like it was immutable. Immutable data by convention if you will. Fortunately, libraries like Ramda exist that do exactly that and all you need to do is to use Ramda consistently.