Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[base-controller] ControllerMessenger should prohibit retroactive subscriptions to published events #4700

Open
MajorLift opened this issue Sep 13, 2024 · 0 comments
Labels

Comments

@MajorLift
Copy link
Contributor

MajorLift commented Sep 13, 2024

Motivation

Listeners in typical event systems like EventEmitter only trigger if they existed when the event was emitted. However, the messenger event pubsub system in our ControllerMessenger class does not follow this norm. This behavior should be fixed for maintenance and predictability reasons.

Acceptance Criteria

  • Subscribers should receive events only if they were already registered to the event at the time it was published.
  • After a given event has been published, its subscribers collection should not be retroactively mutated.
    • Eliminate the risk of a mutable subscribers collection triggering an infinite loop in React/Redux due to a circular dependency (e.g. a subscription handler mutates a hook dependency, which re-triggers its subscription).

Further Research

  • Consider introducing "Topics" for a collection of events that may need to be published repeatedly, potentially to a growing list of subscribers.

References

Currently, when an event is published, we iterate through all the current subscribers using entries().
This returns an iterator, meaning that the subscribers collection is dynamically processed, and so if any of the listeners add an additional subscriber, it too will be processed in the same loop, even though the event was technically published before the new listener existed.
I worry this is undesirable conceptually and technically, as in typical event systems (like the legacy EventEmitter we used), listeners are only triggered if they existed when the event was emitted.
This issue has already presented itself within a recent mobile bug I was looking at where we were did a subscribe within a useEffect as the listener relied on dynamic React properties and Redux state. As the listener ultimately mutated the dependencies that re-triggered the subscription, the above behaviour meant that the subsequent listeners were executed immediately and crashed the app due to a render loop.
Whether the above example is good practice is a separate issue as I've resolved it by refactoring the dependencies, but I still feel this publish behaviour isn't ideal simply for the maintenance and predictability implications.
I've confirmed this was the issue by updating my node_modules directly to process the subscribers upfront with a [...entries()].
-- https://consensys.slack.com/archives/C01V1L10W2E/p1726146922994539

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant