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

runtime/dispatcher: Break recv loop on abort request #3023

Merged
merged 1 commit into from
Jun 17, 2020

Conversation

ptrus
Copy link
Member

@ptrus ptrus commented Jun 17, 2020

Ideally this should be caught by:

select {
case ev := <-evCh:
require.NotNil(ev.Started, "should have received a successful start event")
case <-time.After(recvTimeout):
t.Fatalf("Failed to receive event")
}
// Trigger a non-force abort (runtime should not be restarted).
err = r.Abort(context.Background(), false)
require.NoError(err, "Abort(force=false)")
// There should be no stop event.
select {
case ev := <-evCh:
require.Nil(ev.Stopped, "unexpected stop event")
case <-time.After(recvTimeout):
}

But the problem is that the Abort request makes it to the dispatcher before the first iteration of the dispatch loop, therefore the request is handled before the dispatcher is stuck in the recv loop.
Updated the test to do another abort which also handles the case where dispatcher is stuck in the recv loop.

@ptrus ptrus force-pushed the ptrus/fix/runtime-abort-request branch from 24af4e5 to 3944124 Compare June 17, 2020 11:53
@ptrus ptrus force-pushed the ptrus/fix/runtime-abort-request branch from 3944124 to 7981e8e Compare June 17, 2020 12:02
@codecov
Copy link

codecov bot commented Jun 17, 2020

Codecov Report

Merging #3023 into master will decrease coverage by 0.16%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3023      +/-   ##
==========================================
- Coverage   68.62%   68.45%   -0.17%     
==========================================
  Files         367      367              
  Lines       36130    36130              
==========================================
- Hits        24795    24734      -61     
- Misses       8165     8207      +42     
- Partials     3170     3189      +19     
Impacted Files Coverage Δ
go/worker/compute/executor/committee/state.go 74.07% <0.00%> (-25.93%) ⬇️
go/consensus/tendermint/api/errors.go 86.66% <0.00%> (-13.34%) ⬇️
go/consensus/tendermint/abci/state/state.go 61.53% <0.00%> (-7.70%) ⬇️
go/worker/compute/executor/committee/node.go 61.48% <0.00%> (-5.70%) ⬇️
go/runtime/committee/nodes.go 80.39% <0.00%> (-3.93%) ⬇️
go/consensus/tendermint/apps/staking/state/gas.go 77.58% <0.00%> (-3.45%) ⬇️
go/storage/metrics.go 78.26% <0.00%> (-3.27%) ⬇️
go/worker/compute/txnscheduler/committee/node.go 61.26% <0.00%> (-2.11%) ⬇️
go/storage/mkvs/lookup.go 73.46% <0.00%> (-2.05%) ⬇️
go/runtime/client/watcher.go 73.84% <0.00%> (-1.54%) ⬇️
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b7b2c42...7981e8e. Read the comment docs.

@ptrus ptrus merged commit 18471ae into master Jun 17, 2020
@ptrus ptrus deleted the ptrus/fix/runtime-abort-request branch June 17, 2020 12:41
@ptrus ptrus mentioned this pull request Jun 18, 2020
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