Incorrect listener is triggered when one listener unsubscribes another

[dmitryshimkin]

Hi,

First thank you for making web faster!

I am using the version 2.3.0 of the library and encountered the following issue:

Test case:

const store = unistore.createStore()

const handlerA = () => {
  console.log('Handler A invoked')
}

const handlerB = () => {
  console.log('Handler B invoked')
  store.unsubscribe(handlerA)
}

const handlerC = () => {
  console.log('Handler C invoked')
}

const handlerD = () => {
  console.log('Handler D invoked')
}

store.subscribe(handlerA)
store.subscribe(handlerB)
store.subscribe(handlerC)
store.subscribe(handlerD)

store.setState({ foo: 'bar' })

Expected output:

Handler A invoked
Handler B invoked
Handler C invoked
Handler D invoked

Actual output:

Handler A invoked
Handler B invoked
Handler D invoked

[cfenzo]

It's a side effect from using splice to remove listeners: the index is reset, and when that happen inside a for loop, it will cause problems.

I don't know which strategy is best suited here..
Prior art (Redux) suggests not allowing to unsubscribe from within a listener (Redux also do not allow to subscribe from within a listener, probably for related reasons).
Another approach could be to use delete listeners[i] over splice, but what consequences that might give in terms of memory consumption, performance, or other side effects, is unknown to me..

@developit might have some more knowledge here?

[developit]

Hi there - looks exactly like the issue I just fixed in mitt: developit/mitt#65.

[developit]

Two options available to us.

Option 1: clone listeners when unsubscribing:

This adds a bit of overhead from the clone, and is 683b.

function unsubscribe(listener) {
  let i = listeners.indexOf(listener);
  (listeners = listeners.slice()).splice(i, !!~i);
}

Option 2: use a backwards loop for setState() listener invocation

This technically still breaks if a listener earlier in the list is removed (it will trigger double-calls). Size is 682b.

setState(update, overwrite) {
  state = overwrite ? update : assign(assign({}, state), update);
  for (let i=listeners.length; i--; ) listeners[i](state);
}

Option 3: manually clone in unsubscribe()

Thanks to gzip, this is by far the smallest option. It's also the cheapest at 674b (a savings!).

function unsubscribe(listener) {
  let out = [];
  for (let i=0; i<listeners.length; i++) {
    if (listeners[i]!==listener) {
      out.push(listeners[i]);
    }
  }
  listeners = out;
}

[dmitryshimkin]

I don't see how the Option 1 would work.

The issue is caused by the fact, that it's not safe to mutate the array of listeners while iterating over them. Since the code inside the loop uses a direct reference to the listeners (see https://github.com/developit/unistore/blob/master/unistore.js#L35) it will use mutated version of the array.

The safest way would be to clone the array of listeners before iterating over it to make sure it cannot be mutated during the loop. Not sure about performance/GC penalty though.

[developit]

Ah you're right. It makes me sad that we will be cloning in such a hot function, but it seems like the only way to go.

[dmitryshimkin]

Actually it can be done with unsubscribe, which is more preferable performance wise (I guess). But you need to use a wrapper instead of a direct link to listeners.

[dmitryshimkin]

E.g.

const obj = {
  listeners: []
}

function unsubscribe(listener) {
  let i = listeners.indexOf(listener)
  obj.listeners = obj.listeners.slice().splice(i, !!~i)
}

function invokeListeners() {
  const listeners = obj.listeners // This will keep a reference to the initial array
  for (...) {
    // call listener 
  }
}

[developit]

Ahh that's a great option, I'll check the size.

[developit]

Here's some benchmarks btw:

https://esbench.com/bench/5a2abc8399634800a034951e

[developit]

Here's an implementation of your technique, at 684b:

function unsubscribe(listener) {
  let out = [];
  for (let i=0; i<listeners.length; i++) {
    if (listeners[i]!==listener) {
      out.push(listeners[i]);
    }
  }
  listeners = out;
}

setState(update, overwrite) {
  state = overwrite ? update : assign(assign({}, state), update);
  for (let l=listeners, i=0; i<l.length; i++) l[i](state);
}

[dmitryshimkin]

Ah, right. There is no need for an extra wrapper - just a variable :-) I didn't think carefully.

[developit]

This is where I landed - I realized my initial loop copy implementation for unsubscribe() didn't respect the current behavior, which only removes the first occurrence of a given listener. This one does. 687b.

function unsubscribe(listener) {
  let out = [];
  for (let i=0; i<listeners.length; i++) {
    if (listeners[i]===listener) {
      listener = null;
    }
    else {
      out.push(listeners[i]);
    }
  }
  listeners = out;
}