Skip to content

fix(rtc): emit events to handlers in registration order - #802

Open
avionicharshit-byte wants to merge 1 commit into
livekit:mainfrom
avionicharshit-byte:fix/emit-in-registration-order
Open

fix(rtc): emit events to handlers in registration order#802
avionicharshit-byte wants to merge 1 commit into
livekit:mainfrom
avionicharshit-byte:fix/emit-in-registration-order

Conversation

@avionicharshit-byte

Copy link
Copy Markdown

fixes #778, and the approach is the one 404n0signal proposed there.

EventEmitter kept handlers in a set, so emit() dispatched in hash order rather than registration order. A handler that mutates the emitted object and one that reads it therefore race, and since handler identities are stable the winner is decided once per process: whole sessions come out one way, whole sessions the other. AgentActivity._on_metrics_collected in livekit-agents is the live case, it stamps speech_id onto the metrics object before re-emitting, so a user handler on the same event sees the id or None for the entire run. Nothing raises and nothing logs, the metrics are just unkeyed.

_events now maps each event to a dict keyed by handler. Insertion-ordered, still O(1) add, remove and dedup, so on / off / duplicate registration behave exactly as before.

the two ordering tests use handlers with fixed hashes, so a set is guaranteed to yield them in the opposite order to the one they were registered in. without that they pass on main by luck: three consecutively defined functions happen to land in the set in order. on main the suite is 2 failed, 6 passed, At index 0 diff: None != 'speech_1'; with the change, 8 passed.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

Devin Review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

EventEmitter dispatches handlers in Set order — peer handlers observe pre-mutation event state non-deterministically

1 participant