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.

Sign up for GitHub

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

Ensure query is canceled after account rewind. #893

Merged
merged 8 commits into from
Feb 24, 2022
Merged

Ensure query is canceled after account rewind. #893

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
757e7ad
Cancel transaction when exiting AccountAtRound.
winder Feb 23, 2022
93149db
Split function in two to avoid multiple defers.
winder Feb 23, 2022
815a62d
Call cf twice instead of splitting function, drain accounts channel.
winder Feb 23, 2022
dfe7967
Revert split function change.
winder Feb 23, 2022
aef1602
Extend circleCI timeout.
winder Feb 23, 2022
973dbec
Extend circleCI timeout.
winder Feb 23, 2022
95d4066
Fix test.
winder Feb 24, 2022
1a5ea7d
Add comments.
winder Feb 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions .circleci/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,9 @@ commands:
- run: make lint
- run: make check
- run: make integration
- run: make test
- run:
command: make test
no_output_timeout: 15m
- run: make test-generate
- run: make fakepackage
- run: make e2e
Expand All @@ -115,7 +117,9 @@ commands:
- run: make lint
- run: make check
- run: make integration
- run: make test
- run:
command: make test
no_output_timeout: 15m
Copy link
Contributor Author

Choose a reason for hiding this comment

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

go test times out after 10 minutes. Change the CircleCI timeout to something larger than that so that the timeout error is properly reported.

- run: make test-generate
- run: make fakepackage
- run: make e2e
Expand Down
11 changes: 10 additions & 1 deletion accounting/rewind.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,16 @@ func AccountAtRound(ctx context.Context, account models.Account, round uint64, d
MinRound: round + 1,
MaxRound: account.Round,
}
txns, r := db.Transactions(ctx, tf)
ctx2, cf := context.WithCancel(ctx)
// In case of a panic before the next defer, call cf() here.
defer cf()
winder marked this conversation as resolved.
Show resolved Hide resolved
txns, r := db.Transactions(ctx2, tf)
// In case of an error, make sure the context is cancelled, and the channel is cleaned up.
defer func() {
winder marked this conversation as resolved.
Show resolved Hide resolved
cf()
for range txns {
}
}()
winder marked this conversation as resolved.
Show resolved Hide resolved
if r < account.Round {
err = ConsistencyError{fmt.Sprintf("queried round r: %d < account.Round: %d", r, account.Round)}
return
Expand Down
6 changes: 4 additions & 2 deletions accounting/rewind_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,10 +67,12 @@ func TestStaleTransactions1(t *testing.T) {
Round: 8,
}

var outCh <-chan idb.TxnRow
ch := make(chan idb.TxnRow)
var outCh <-chan idb.TxnRow = ch
close(ch)
Copy link
Contributor Author

@winder winder Feb 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@tolikzinovyev this was the test bug. The channel must be closed now otherwise the channel can't be drained.


db := &mocks.IndexerDb{}
db.On("GetSpecialAccounts").Return(transactions.SpecialAddresses{}, nil)
db.On("GetSpecialAccounts", mock.Anything).Return(transactions.SpecialAddresses{}, nil)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure why this one was working. It should have started failing once we added the context to this function.

db.On("Transactions", mock.Anything, mock.Anything).Return(outCh, uint64(7)).Once()

account, err := AccountAtRound(context.Background(), account, 6, db)
Expand Down
6 changes: 6 additions & 0 deletions api/handlers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -879,6 +879,12 @@ func (si *ServerImplementation) fetchAccounts(ctx context.Context, options idb.A
var accountchan <-chan idb.AccountRow
accountchan, round = si.db.GetAccounts(ctx, options)

// Make sure accountchan is empty at the end of processing.
defer func() {
for range accountchan {
}
}()

if (atRound != nil) && (*atRound > round) {
return fmt.Errorf("%s: the requested round %d > the current round %d",
errRewindingAccount, *atRound, round)
Expand Down