Ensure query is canceled after account rewind. #893
Conversation
winder
force-pushed
the
will/cancel-accountatround
branch
from
1209bbd to
757e7ad
Compare
accounting/rewind.go
Outdated
| @@ -99,7 +99,13 @@ func AccountAtRound(ctx context.Context, account models.Account, round uint64, d | |||
| MinRound: round + 1, | |||
| MaxRound: account.Round, | |||
| } | |||
| ctx, cf := context.WithCancel(ctx) | |||
There was a problem hiding this comment.
I think that the code is fine, but please use a different variable name for ctx to avoid variable shadowing.
Also, I'd suggest you'll defer the cf before the db.Transactions call. ( i.e. so that if the db.Transactions call get panicked, the context would still get destroyed )
There was a problem hiding this comment.
That was intentional since I didn't want ctx to be used afterwards, and I didn't think it was worth breaking out a helper function. Does that make sense or would you still prefer to avoid shadowing?
There was a problem hiding this comment.
It's ok that you don't want to use it afterward.. the naming would just make sure the reader would understand it's a different object.
There was a problem hiding this comment.
using defer cf() is a little problematic because the logic would now depend on the evaluation order of defer. I think splitting the function in half should make the order clearer.
There was a problem hiding this comment.
Actually, splitting up the function forces them to be backwards. It occurs to me now that I can simply call cf multiple times.
|
@winder have you figured out why CI is failing? |
I think there are 2 issues.
|
From the logs, |
| var outCh <-chan idb.TxnRow | ||
| ch := make(chan idb.TxnRow) | ||
| var outCh <-chan idb.TxnRow = ch | ||
| close(ch) |
There was a problem hiding this comment.
@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) |
There was a problem hiding this comment.
I'm not sure why this one was working. It should have started failing once we added the context to this function.
| - run: make test | ||
| - run: | ||
| command: make test | ||
| no_output_timeout: 15m |
There was a problem hiding this comment.
go test times out after 10 minutes. Change the CircleCI timeout to something larger than that so that the timeout error is properly reported.
Codecov Report
@@ Coverage Diff @@
## develop #893 +/- ##
===========================================
+ Coverage 58.73% 58.77% +0.04%
===========================================
Files 38 38
Lines 4495 4500 +5
===========================================
+ Hits 2640 2645 +5
Misses 1528 1528
Partials 327 327
Continue to review full report at Codecov.
|
Summary
Make sure the transaction query is canceled when exiting the AccountAtRound function.
Test Plan
Manual testing. Query psql for active queries. Verified a specific problematic query before and after this change.