Skip to content

Conversation

@kanongil
Copy link
Contributor

Due to using shared state, the current implementation is broken if multiple iterators are used on the Events class.

This is fixed in this PR by:

  1. Moving the state into the iterator itself.
  2. Changing the semantics so that calling iterator() only returns newly emitted events, and not all emitted events during the Events instance lifetime.

Additionally, this fixes it so trying to iterate further on a completed iterator returns "done", instead of a never-ending Promise.

The updated semantics are required. Otherwise the Events instance will need to hang on to all emitted events, just in case someone calls iterator() on it again. Given that I am not aware of any existing usage of the Events feature, I cannot know if this will cause problems and should be considered a breaking change. It all depends on how early iterator() is called. An alternative fix, that might cause less breakage, is to add an assert that only allow iterator() to be called once.

Note that with the moved state, care must be taken not to cause a leak due to two-way references. I have added logic to handle this automatically when using for-of through the implicit destroy() call, as well as several tests.

@kanongil kanongil added the bug Bug or defect label Aug 20, 2025

#pending = null;
#queue = [];
#iterators = new Set();
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be safer to also use a WeakSet here just in case it's iterated through weird means?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might be safer, but not possible since you can't iterate a WeakSet.

@Marsup Marsup added this to the 6.0.1 milestone Aug 20, 2025
@Marsup Marsup self-assigned this Aug 20, 2025
@Marsup Marsup merged commit e42390e into hapijs:master Aug 21, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Bug or defect

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants