Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Conversation

@ordian
Copy link

@ordian ordian commented Mar 31, 2021

This would eliminate the data race between OurViewChange and ActiveLeaves signal and that's what most subsystem do to avoid this problem.

@ordian ordian added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Mar 31, 2021
@rphmeier rphmeier merged commit d532d31 into master Mar 31, 2021
@rphmeier rphmeier deleted the ao-fix-race-in-statement-distribution branch March 31, 2021 21:28
ordian pushed a commit that referenced this pull request Mar 31, 2021
…ators

* master:
  statement-distribution: do not use OurViewChange (#2790)
  Better timeout values now that we are going to be connected to all nodes. (#2778)
  proper executor/block type for benchmarks and try-runtime (#2771)
  Fix future-polling loop in availability and add a better early-exit (#2779)
Copy link
Member

@eskimor eskimor left a comment

Choose a reason for hiding this comment

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

I think we have to fix this the other way round - getting rid of ActiveLeavesUpdate. The view updates are handled in the network bridge and we are sharing a peer set for precisely the purpose of keeping our view in sync with peers. @rphmeier am I missing something?

@ordian
Copy link
Author

ordian commented Apr 1, 2021

I think we have to fix this the other way round - getting rid of ActiveLeavesUpdate. The view updates are handled in the network bridge and we are sharing a peer set for precisely the purpose of keeping our view in sync with peers. @rphmeier am I missing something?

OurViewChange message should be based exactly on ActiveLeavesUpdate signal, and most subsystems are using it, also signals are handled before messages, so this will ensure some ordering. But I agree we could equally use one and get rid of another.

@eskimor
Copy link
Member

eskimor commented Apr 1, 2021

Ok, if we assume signals are handled first, then this is probably fine for activated - but deactivated could remove us a head with still messages in flight - no?

@ordian
Copy link
Author

ordian commented Apr 1, 2021

but deactivated could remove us a head with still messages in flight - no?

yes, but if a head is deactivated we don't want to handle these messages anyway
and this is also the case for OurViewChange, we use retain there, which is essentially the same

@rphmeier
Copy link
Contributor

rphmeier commented Apr 1, 2021

but deactivated could remove us a head with still messages in flight - no?

Yes. What we could do is keep around old heads for a small amount of time, but that's not urgent at all.

rphmeier pushed a commit that referenced this pull request Apr 1, 2021
* quickfix for statement-distribution

* some logs
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants