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

fix: event bus delete multiple disconnected #281

Merged
merged 1 commit into from
Aug 3, 2023

Conversation

blckngm
Copy link
Contributor

@blckngm blckngm commented Aug 1, 2023

Description

Previous code will delete wrong receivers when there are multiple disconnected receivers.


PR author checklist:

  • Added changelog entry, using unclog.
  • Added tests: integration (for Forcerelay) or unit/mock tests (for modules).
  • Linked to GitHub issue.
  • Updated code comments and documentation (e.g., docs/).
  • Tagged one reviewer who will be the one responsible for shepherding this PR.

Reviewer checklist:

  • Reviewed Files changed in the GitHub PR explorer.
  • Manually tested (in case integration/unit/mock tests are absent).


// Remove all disconnected subscribers
for idx in disconnected {
self.txs.remove(idx);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When there are multiple disconnected receivers, indexes of the latter ones will change after the first receiver is deleted.

Copy link
Contributor

Choose a reason for hiding this comment

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

that's indeed a bug in official Hermes repo

@Flouse Flouse requested a review from ashuralyk August 2, 2023 01:30
@Flouse Flouse requested a review from jjyr August 2, 2023 01:30
@jjyr jjyr added this pull request to the merge queue Aug 3, 2023
Merged via the queue into synapseweb3:dev with commit 2144329 Aug 3, 2023
28 checks passed
@blckngm blckngm deleted the fix-event-bus branch August 24, 2023 02:22
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.

3 participants