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 Canceling Goroutines in Validator Client #2324

Merged
merged 4 commits into from
Apr 21, 2019
Merged

Conversation

rauljordan
Copy link
Contributor

Follow up to #2317


Description

Write why you are making the changes in this pull request

We were canceling the context to each of the goroutines in the block of code in validator/runner.go:

 for id, role := range v.RolesAt(slot) {
                go func(role pb.ValidatorRole, id string) {
                    switch role {
                    case pb.ValidatorRole_ATTESTER:
                        v.AttestToBlockHead(slotCtx, slot, id)
                 ...
}
cancel()

Write a summary of the changes you are making

This PR removes the cancel call.

@rauljordan rauljordan self-assigned this Apr 21, 2019
@rauljordan rauljordan added the Bug Something isn't working label Apr 21, 2019
@@ -62,7 +62,7 @@ func run(ctx context.Context, v Validator) {
return // Exit if context is canceled.
case slot := <-v.NextSlot():
span.AddAttributes(trace.Int64Attribute("slot", int64(slot)))
slotCtx, cancel := context.WithDeadline(ctx, v.SlotDeadline(slot))
slotCtx, cancel := context.WithDeadline(context.Background(), v.SlotDeadline(slot))
Copy link
Member

Choose a reason for hiding this comment

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

Revert this

terencechain
terencechain previously approved these changes Apr 21, 2019
prestonvanloon
prestonvanloon previously approved these changes Apr 21, 2019
Copy link
Member

@prestonvanloon prestonvanloon left a comment

Choose a reason for hiding this comment

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

Looks good to me.

We should be careful about adding these exclusions. However, this one feels necessary, at the moment, to maintain a clean implementation.

@prestonvanloon prestonvanloon dismissed stale reviews from terencechain and themself via a1e98a7 April 21, 2019 20:34
@codecov
Copy link

codecov bot commented Apr 21, 2019

Codecov Report

Merging #2324 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2324      +/-   ##
==========================================
- Coverage   68.78%   68.77%   -0.01%     
==========================================
  Files         117      117              
  Lines        9193     9192       -1     
==========================================
- Hits         6323     6322       -1     
  Misses       2188     2188              
  Partials      682      682

@prestonvanloon prestonvanloon merged commit ccf7beb into master Apr 21, 2019
terencechain pushed a commit that referenced this pull request Apr 22, 2019
* do not cancel assignments goroutines

* exclude rule

* disable lostcancel for now
terencechain pushed a commit that referenced this pull request Apr 22, 2019
* do not cancel assignments goroutines

* exclude rule

* disable lostcancel for now
prestonvanloon pushed a commit that referenced this pull request Apr 23, 2019
* update attestation related protos

* use root for hash32

* fixed a few typos

* review comments

* update historical batch, deposit and blk header fields

* Add nogo to introduce built time linting (#2317)

* Add nogo and fix lint issues

* Run gazelle

* better gazelle

* ignore external struct tags

* Exclude all third party code from unsafeptr (#2321)

* Fix Assingments Bug (#2320)

* fix

* fix tests

* Add feature flag to toggle gossip sub in p2p (#2322)

* add feature flag to enable gossip sub in p2p

* invert the enable/disable logic

* add the flag in k8s and fix tests

* gazellle

* return empty config if nil

* Prevent Canceling Goroutines in Validator Client (#2324)

* do not cancel assignments goroutines

* exclude rule

* disable lostcancel for now

* Fix Pending Attestations RPC Call (#2304)

* pending atts

* use proposal slot

* attestation inclusion fix

* lint

* advance state transitions

* gazelle

* lint tests pass

* Do Not Update Validator Registry on Nil Block (#2326)

* no registry update if block is nil

* regression test

* lint

* Ensure Block Processing Failures Return an Error (#2325)

* ensure block failed processing returns an error

* fixed test

* test assertion corrected

* comments

* fix tests

* imports

* rebase

* add spec badge. Thanks to ChainSafe/lodestar#166 for the idea :)

* Update Crosslink Protobuf Fields (#2313)

* rebase

* rebase

* rebase

* Prevent Canceling Goroutines in Validator Client (#2324)

* do not cancel assignments goroutines

* exclude rule

* disable lostcancel for now

* starting proposer slashing

* starting attester slashing

* revert gen file

* Update types.pb.go
@prestonvanloon prestonvanloon deleted the broken-cancels branch June 6, 2019 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants