MetaMask / MetaMask/core

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

Open
#4,700 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug team-wallet-framework wf-bugs
Dominant language
TypeScript
Stars
413
Forks
308
Avg merge
1d 4h
Merged PRs (30d)
253

Description

## 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

- Proposed by @matthewwalsh0:
> 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

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start at the ControllerMessenger publish and subscription behavior described in the issue, then inspect the surrounding implementation and existing tests. Reproduce a subscription added during publication and verify that it is not invoked for the current event; done means existing subscribers run once and the subscriber collection is not retroactively processed.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
backend-api-design
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
52/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.