node icon indicating copy to clipboard operation
node copied to clipboard

diagnostics_channel: fix unsubscribe during publish

Open simon-id opened this issue 1 year ago • 0 comments

Problem:

When unsubscribing a publisher that has already been published to, while inside a subscriber, the next subscriber will not be called. Example:

const dc = require('diagnostics_channel')

const testChannel = dc.channel('test')

function subscriberA () {
  console.log('A called')
  testChannel.unsubscribe(subscriberA)
}

function subscriberB () {
  console.log('B called')
}

function subscriberC () {
  console.log('C called')
}

testChannel.subscribe(subscriberA)
testChannel.subscribe(subscriberB)
testChannel.subscribe(subscriberC)

testChannel.publish()
/*
Expected:
A called
B called
C called

Actual:
A called
C called
*/

This is due to the publish() method iterating over the array with incrementing index:

  publish(data) {
    for (let i = 0; i < this._subscribers.length; i++) {
      const onMessage = this._subscribers[i];
      onMessage(data, this.name);
    }
  }

but unsubscribe() removing an element from that same array while it's being iterated upon:

  unsubscribe(subscription) {
    const index = ArrayPrototypeIndexOf(this._subscribers, subscription);
    if (index === -1) return false;

    // !!! mutate the array and shift the indexes of the array making the loop skip a beat
    ArrayPrototypeSplice(this._subscribers, index, 1);

    return true;
  }

Thank you @iunanua for finding the bug!

Solution:

Instead of removing elements from array directly, set the element as undefined, and remove the undefined values later.

simon-id avatar Sep 25 '24 11:09 simon-id