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

Prevent race condition when cluster is being initialized #104

Merged

Conversation

guusdk
Copy link
Member

@guusdk guusdk commented Nov 2, 2024

The existing implementation suffers from a race condition that is described in #103.

This PR provides fixes in the first two commits:

  • the first commit fixes the race condition in a clunky way
  • the second commit improves the fix

Each commit has a commit message that describes the changes made in that commit.

Upon merge, these commits should possibly be squashed. I left them unsquashed for your reviewing pleasure.

Initial tests are promising, but more testing is needed. I'm creating this PR mainly because I've got a nagging feeling that there's room for improvement, that I can't quite put my finger on. I'm hoping for reviewing feedback. :)

A race condition can occur between the cluster start and cluster event listeners being registered with the cluster, causing the listeners to miss events that are sent before the listener registers.

This commit is a first clunky attempt at fixing this problem. It registers the listeners prior to the cluster being started. It then blocks processing of event handling until the cluster is initialized. However, some of this blocking prevents the cluster from initializing, which is why some selected events are mostly ignored.

It's all rather clunky, but it seems to work. I'm committing this as a first working solution, to be improved on further.
…ution

This builds on the fix to resolve a cluster event listener race, as introduced by the previous commit.

In the previous commit, event handling is blocked until the cluster is fully initialized. A chicken/egg problem (where blockage of certain events prevented the cluster to initialize) caused a haphazardous implementation in which some selected events were _not_ blocked.

In this commit, events are queued rather than blocked. This allows the cluster to initialize, without 'special treatment' of certain events.
Copy link
Contributor

@GregDThomas GregDThomas left a comment

Choose a reason for hiding this comment

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

This seems fine, a few picky comments but I can't see a better way of doing it, tbh,

@guusdk
Copy link
Member Author

guusdk commented Nov 5, 2024

Feedback from @viv on this was: "looks reasonable" - with that', I'll merge as-is.

@guusdk guusdk merged commit 612c803 into igniterealtime:main Nov 5, 2024
3 checks passed
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.

2 participants