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

Event Subscriptions Should Be Initialized in the Constructor #7492

Open
nisdas opened this issue Oct 10, 2020 · 1 comment
Open

Event Subscriptions Should Be Initialized in the Constructor #7492

nisdas opened this issue Oct 10, 2020 · 1 comment
Labels
Cleanup Code health! Enhancement New feature or request Help Wanted Extra attention is needed

Comments

@nisdas
Copy link
Member

nisdas commented Oct 10, 2020

💎 Issue

Currently a few services initialize event subscriptions for state events(State Initialization, Syncing) in the Start routine of a service. This should instead be done in the constructor of the service.

Background

In #7468, the initial PR failed in runtime during pre-genesis in Zinken . This was because the initial-sync service instead skipped waiting for state initialization and directly when to syncing . This would trigger a race condition where we would received the synced event before state initialization . Which is logically incomprehensible, as we would need the state to be initialized before we can continue deciding on whether to sync or not. This was initially introduced in #3633 , so as to fix a race condition in where initial sync would be permanently stalled.

Description

The reason for the race stated above is bad implementation of event subscriptions in our services , where most of them are subscribed to when starting the service in Start . This leads to a race between all these services and the blockchain service from which the state initialization event is emitted. If we subscribe after that event is emitted we will permanently miss it and lead to undefined behaviour.

The correct way to resolve this is to subscribe to events in NewService , this ensures that all services will always be able to listen to the events correctly and avoid any race conditions.

@nisdas nisdas added Cleanup Code health! Enhancement New feature or request Help Wanted Extra attention is needed labels Oct 10, 2020
@rkapka rkapka self-assigned this Oct 26, 2020
@rkapka rkapka removed their assignment Oct 27, 2020
@shashank-priyadarshi
Copy link

@rkapka @nisdas is this issue still relevant? If yes, I would like to take this up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cleanup Code health! Enhancement New feature or request Help Wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants