Set Listener Callback to Undefined for Specific Index

[QuintinKruger]

Currently the callback array is spliced, this causes the dimensions to change and as a result splice to not happen as expected

This leads to a listener that will never be unregistered as the array of callbacks for the listener might not always be empty when it is expected to be

Please see an example:

The logs printed are as follows

Please notice that although we deregistered all callbacks from the listener DownloadedSeasonsDataListener, we still received data on a callback

This is because no network call was made to unregister from the server

Currently the dispose function performs a splice on the array of callbacks. When a subsequent call to dispose of a callback is made the index used to splice will be outdated with the array that has changed, as a result the callback may not be removed from the array and then the if check fails for the last callback in the array to perform an unregister API call

Here is the array of callbacks before dispose takes place

for the removal of the first callback, the dispose performs the expected behaviour

This now changed the dimension of the array from 3 items to 2, thus the 2nd call to dispose will dispose the 2nd item in the array (which is not the expected callback to remove as the index is for an array of 3 items not 2)

And then finally when the 3rd callback needs to be removed, there is a single item in the array but then the task of the dispose function is to remove item from index 2 which it cannot do on an array of size 1

The if condition then never resolves to true and we are stuck with the websocket remaining open and a single callback receiving data even though all listeners were expected to have been disposed of

With the updates made in this PR, rather than changing the dimension of the array as we dispose of callbacks, I flag them for removal by setting them to undefined, this way we ensure no updates are made to the array dimension which results in outdated index values

The result of this with the test laid out above is as follows:

Finally here is a test case showing that callbacks for those that are not undefined still take place as expected

The code used:

The result

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[wouterlucas]

Good spot!

I think the only issue with this is we're no longer cleaning up the array of listeners[listener_id][] meaning that will always have a length and not shrink. As we're no longer splicing but just setting it to undefined instead. It'll remain as a hole in the array that has an entry to nothing.

Now this might be fine if the amount is limited and not often reassigned?
It will obviously still be cleaned/removed on an unregister as the whole array will be derefernced/deleted from the main Listener object. So it might not be an issue?

It kind of depends on how you use this and what the lifecycle of the individual Listeners are in your project.

[QuintinKruger]

Good spot!

I think the only issue with this is we're no longer cleaning up the array of listeners[listener_id][] meaning that will always have a length and not shrink. As we're no longer splicing but just setting it to undefined instead. It'll remain as a hole in the array that has an entry to nothing.

Now this might be fine if the amount is limited and not often reassigned? It will obviously still be cleaned/removed on an unregister as the whole array will be derefernced/deleted from the main Listener object. So it might not be an issue?

It kind of depends on how you use this and what the lifecycle of the individual Listeners are in your project.

Hi @wouterlucas

Thanx for the comment - yeah I was worried about it not being cleaned but then saw the line delete listeners[listener_id] in the unregister which will happen if all callbacks are undefined

I get what you mean though - the lifecycle of a project might only dispose of the last listener way later than anticipated leading to an array that may have a huge number of undefined values before eventually being cleaned up

Unfortunately I couldnt see any other way of removing a callback via an ID to differentiate it from another hence why I went the route simply setting the item at that index to undefined.

@wouterlucas perhaps a further update is needed to have the register function assign a better ID to the callback (not just an index ?)

[wouterlucas]

I think you can also copy the array over to a const, do the unregistering using that copy and then clean out the overall array. It's slightly more expensive but this way the array is always intact when you are processing it. Just make sure it is a real copy and not a reference.

I think that way you'd avoid race conditions for 2 simultaneous splices happening on starved CPU that is bumping stuff across ticks.

[QuintinKruger]

I think you can also copy the array over to a const, do the unregistering using that copy and then clean out the overall array. It's slightly more expensive but this way the array is always intact when you are processing it. Just make sure it is a real copy and not a reference.

I think that way you'd avoid race conditions for 2 simultaneous splices happening on starved CPU that is bumping stuff across ticks.

Apologies @wouterlucas - I am a bit confused as to how the copied array will help us? We dont want to invalidate items in the copied array as this copied array wont be referenced when callbacks for a listener needs to run (which references the global listeners defined by the store)