Skip to content

Commit

Permalink
Stops notifying consumers added during the notification process.
Browse files Browse the repository at this point in the history
While trying out a synchronous scheduler, I found a case where notifying a `Consumer` caused a new `Consumer` to be added. This happened recursively and caused and infinite loop, repeatedly adding and then notifying a new `Consumer` endlessly.

`Producer` objects should only notify existing `Consumer` objects, because those are the objects which are potentially dirty. Any `Consumer` added during the notification already observes the new value of the `Producer` and does not need to be notified. This avoids an infinite loop in the sync scheduler case and also reduces unnecessary notifications for a `Consumer` added at the wrong time.
  • Loading branch information
dgp1130 committed Dec 2, 2024
1 parent ab672f4 commit 283fee9
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 1 deletion.
20 changes: 20 additions & 0 deletions src/signals/graph.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,26 @@ describe('graph', () => {
expect(consumer1.notifyListeners).toHaveBeenCalledOnceWith();
expect(consumer2.notifyListeners).not.toHaveBeenCalled();
});

it('ignores consumers added during notification', () => {
const producer = Producer.from(() => 1);

const consumer1 = Consumer.from();
const consumer2 = Consumer.from();

spyOn(consumer1, 'notifyListeners').and.callThrough();
spyOn(consumer2, 'notifyListeners').and.callThrough();

producer.addConsumer(consumer1);
consumer1.listen(() => {
producer.addConsumer(consumer2);
});

producer.notifyConsumers();

expect(consumer1.notifyListeners).toHaveBeenCalled();
expect(consumer2.notifyListeners).not.toHaveBeenCalled();
});
});

describe('Consumer', () => {
Expand Down
6 changes: 5 additions & 1 deletion src/signals/graph.ts
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,11 @@ export class Producer<Value> {
* {@link Producer} has changed.
*/
public notifyConsumers(): void {
for (const consumer of this.#consumers) consumer.notifyListeners();
// Snapshot current consumers with `Array.from`, because they might change
// during notification callbacks.
for (const consumer of Array.from(this.#consumers)) {
consumer.notifyListeners();
}
}
}

Expand Down

0 comments on commit 283fee9

Please sign in to comment.