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 bug where Subscribe command would sometimes get dropped in the event monitor #3095

Merged
merged 1 commit into from
Feb 17, 2023

Conversation

romac
Copy link
Member

@romac romac commented Feb 16, 2023

Closes: #3070

Description

By checking whether the event monitor got a Shutdown command after processing an event bactch, the event monitor would drop the Subscribe command if one had been received instead in the meantime.

By dropping the check, and only shutting down the event monitor after having processed the last received batch, we ensure we do not drop such commands in the future.


PR author checklist:

  • Added changelog entry, using unclog.
  • Added tests: integration (for Hermes) 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).

By checking whether the event monitor got a `Shutdown` command after processing
an event bactch, the event monitor would drop the `Subscribe` command if one
had been received instead in the meantime.

By dropping the check, and only shutting down the event monitor after having
processed the last received batch, we ensure we do not drop such commands
in the future.
Copy link
Contributor

@ljoss17 ljoss17 left a comment

Choose a reason for hiding this comment

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

Seems good to me!

@romac romac merged commit 97cf4be into master Feb 17, 2023
@romac romac deleted the romac/fix-listen branch February 17, 2023 09:43
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.

Hermes listen command is broken
2 participants