Remove() function in Observer remove other observers

Bugs
1

Hi!

When I have multiple observers subscribed to the same observable, calling the remove() function on one of them causes the others to stop receiving notifications as well.

It seems that the remove() function in an observer removes all observers from the observable, not just the one that called it.

Here a playground with the issue:

2

I believe it should be testObservable.remove(observer2);

3

But in this case I’m calling the remove() function on a specific observer. As I understand it, that should only remove that particular observer and can be an alternative to using the observable’s remove function, as noted in the documentation.

4

As I see from the console messages, your code also works as expected - https://playground.babylonjs.com/#DZH3F3#2

5

The problem is with observer2: if you add a console.log there, it won’t be triggered if you call remove() on observer1 before

6

Removing an Observer modifies the array with all Observers from an Observable.

The array is only a reference, and any modifications during the loop execution will affect the next iteration on the same loop. Adds on top of it that the observers are executed in sequence, not in parallel.

Get the following example:

const arr = [0 , 1, 2, 3];

for (const val of arr) {
  console.log(val);
  arr.shift(); // Removes the first position of the array
}

The result is:

0
2

This is due of the usage of a pointer to get the next position on the array and modifying the array during the loop.
In your example, if you had a third observer, the third one would be executed.

Thus, at this moment you cannot remove the handlers inside of the handler itself, which can be considered a bug, or just “working as intended”.

Nevertheless, if you want to execute only once, you can use Observable.addOnce() instead of Observable.add(). If that is not the cause, you have to find another way to remove the handlers outside the loop of execution.

2 Likes
7

Thanks, I understand the issue now. However, I’ve noticed that if I call testObservable.remove(observer1) instead of calling remove() from within the observer itself, the issue doesn’t occur: all observers are executed as expected.

Given that, wouldn’t it make sense for both approaches (observer.remove() and observable.remove(observer)) to manage the observers in the same way internally, so that they behave consistently? Or is there a specific reason why they’re handled differently?

8

Internally when calling Observer.remove(), it is a function that calls Observable._remove(observer).

Here is the code:

// attach the remove function to the observer
const observableWeakRef = isWeakRefSupported ? new WeakRef(this) : { deref: () => this };
observer._remove = () => {
    const observable = observableWeakRef.deref();
    if (observable) {
        observable._remove(observer);
    }
};

On the other hand, calling Observable.remove(), calls Observable._deferUnregister(), which defers the call to Observable._remove.

/**
 * @internal
 */
public _deferUnregister(observer: Observer<T>): void {
    if (observer._willBeUnregistered) {
        return;
    }
    this._numObserversMarkedAsDeleted++;
    observer.unregisterOnNextCall = false;
    observer._willBeUnregistered = true;
    setTimeout(() => {
        this._remove(observer);
    }, 0);
}

It might be a design choice to defer only when calling Observable directly.

9

I think the solution is to call the public remove function instead of the private one when attaching it to the observer to avoid modifying the observers array during iteration.

Let’s see if a core dev can confirm that.

10

Can you summarize the issue for me? Not sure to get what is wrong in the repro :slight_smile:

11

Observer.remove() has a different behaviour than Observable.remove(observer).

Basically, Observer.remove() doesn’t defer the removal of the observer, and it affects the array that is being iterated inside Obversable.notifyObservers(), like showed on my comment here.

12

Oh I see! Ok yeah we can totally use the public remove then instead of _.

BUT that will trigger a large breaking change so we may want to have an option to have it off by default but let the user choose to do it (like Observer.remove(TRUE))

13

Something like this?

// attach the remove function to the observer
const observableWeakRef = isWeakRefSupported ? new WeakRef(this) : { deref: () => this };
observer._remove = (defer = false) => {
    const observable = observableWeakRef.deref();
    if (observable) {
        defer ? observable.remove(observer) : observable._remove(observer);
    }
};
1 Like
14

exactly…and available on the public remove function

15

I’ve created a PR with the change:

3 Likes
16

sorry to bump.. but wouldn’t it be better to use a reverse loop in this case?

aka

for (let i = this._observers.length - 1; i > -1; i--) {
  const obs = this._observers[i];

then there’s no need for timeouts, multiple different remove / unregister functions etc
unless i’m missing something obvious :laughing:

Edit; i guess order in which the observers are called compared to when they are registered would change :man_shrugging:
But that can be sorted “behind the scene” by always inserting first and making insertFirst? push to the end of the array instead, so it still means “execute first”

1 Like
17

I guess even with the “inserFirst” adjustment this could prod unexpected side effects (especially when interacting with the array directly, even if that’s not recommended). So it might be considered a significant breaking change.

18

I tend to agree here: this would be too big of a change