Skip to content

market: Errorf->Fatalf, and eliminate not-a-race#427

Merged
chappjc merged 2 commits intodecred:masterfrom
chappjc:market-test-fatal
May 29, 2020
Merged

market: Errorf->Fatalf, and eliminate not-a-race#427
chappjc merged 2 commits intodecred:masterfrom
chappjc:market-test-fatal

Conversation

@chappjc
Copy link
Copy Markdown
Member

@chappjc chappjc commented May 28, 2020

This resolves a test timeout seen in https://github.com/decred/dcrdex/runs/715296974 for PR #372, although it's still unclear why the market wasn't running.

The unguarded set of a channel was also technically a race but it couldn't ever be hit. It wasn't necessary to nil it out, so don't.

@chappjc
Copy link
Copy Markdown
Member Author

chappjc commented May 28, 2020

Can no longer trigger any timing related errors with parallel -P 46 -N0 --halt soon,fail=1 go test -race -count=1 -short ./server/market :::: <(seq 1 2000) (tweak 46 to be ~150% of your CPU logical core count).

ctx, cancel := context.WithCancel(context.Background())
startEpochIdx := 1 + encode.UnixMilli(time.Now())/epochDurationMSec
defer cancel()
startEpochIdx := 2 + encode.UnixMilli(time.Now())/epochDurationMSec
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The first part of this test is submitting and order to a market that hasn't hit it's start epoch yet, and if the code between here and SubmitOrder here takes too long, the market will be running and the expected error won't be hit. So 1 -> 2 for some extra time.

err = mkt.SubmitOrder(oRecord)
if err != nil {
t.Error(err)
t.Fatal(err)
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

All of these Errors needed to be Fatals because the rest of the test won't work (and the channel receives a couple lines down may hang).

<-auth.handlePreimageDone
// and for matching to complete (in processReadyEpoch).
<-storage.epochInserted
storage.epochInserted = nil
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That's the last epoch with this test market anyway, so there are no more sends from TArchivist on this channel anyway. Remove this since it was unguarded and technically a race.

@chappjc chappjc merged commit d2ce442 into decred:master May 29, 2020
@chappjc chappjc deleted the market-test-fatal branch May 29, 2020 21:58
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