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

refactor: remove defer in loop #20223

Merged
merged 29 commits into from
May 29, 2024
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
248ad76
refactor defer in loop, and add error handling for Close function
tuantran1702 Apr 30, 2024
306cf5b
Update store/storage/storage_test_suite.go
tuantran1702 Apr 30, 2024
7b0116e
Update x/group/keeper/invariants.go
tuantran1702 Apr 30, 2024
e45b71f
Update store/commitment/store.go
tuantran1702 Apr 30, 2024
023247e
Update store/commitment/store.go
tuantran1702 Apr 30, 2024
93c36dc
Update store/migration/manager.go
tuantran1702 Apr 30, 2024
b0c99b4
refactor to use anonymous closure
tuantran1702 May 1, 2024
badd9c5
Merge branch 'refactor-defer' of github.com:notional-labs/cosmos-sdk …
tuantran1702 May 1, 2024
9995356
linting
tuantran1702 May 1, 2024
b6c8dbe
rollback importer defer, bring pw.Close() to closure
tuantran1702 May 1, 2024
097384b
remove incorrect Close()
tuantran1702 May 1, 2024
e7b1a17
refactor
tuantran1702 May 2, 2024
b6a63be
fix redundant err check
tuantran1702 May 2, 2024
95fc4a7
refactor defer in loop, and add error handling for Close function
tuantran1702 Apr 30, 2024
869ec3e
refactor to use anonymous closure
tuantran1702 May 1, 2024
d819ddd
Update store/storage/storage_test_suite.go
tuantran1702 Apr 30, 2024
4543e67
Update store/commitment/store.go
tuantran1702 Apr 30, 2024
fce217a
Update store/commitment/store.go
tuantran1702 Apr 30, 2024
fbaef03
linting
tuantran1702 May 1, 2024
953cfac
rollback importer defer, bring pw.Close() to closure
tuantran1702 May 1, 2024
879999d
remove incorrect Close()
tuantran1702 May 1, 2024
e9df29b
refactor
tuantran1702 May 2, 2024
8701127
fix redundant err check
tuantran1702 May 2, 2024
0415482
add missing import
tuantran1702 May 9, 2024
a737df0
add err handling
tuantran1702 May 9, 2024
2f0ac3f
Merge branch 'main' into refactor-defer
tuantran1702 May 11, 2024
5bc9597
Merge branch 'refactor-defer' of github.com:notional-labs/cosmos-sdk …
tuantran1702 May 11, 2024
076a43f
Merge branch 'main' into refactor-defer
tuantran1702 May 17, 2024
101706b
lint errorf
tuantran1702 May 17, 2024
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
4 changes: 3 additions & 1 deletion store/commitment/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -399,7 +399,9 @@ loop:
if err := importer.Commit(); err != nil {
return snapshotstypes.SnapshotItem{}, fmt.Errorf("failed to commit importer: %w", err)
}
importer.Close()
if err := importer.Close(); err != nil {
return snapshotstypes.SnapshotItem{}, fmt.Errorf("failed to close importer: %w", err)
}
}

storeKey = item.Store.Name
Expand Down
21 changes: 15 additions & 6 deletions store/migration/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -134,13 +134,22 @@ func (m *Manager) writeChangeset() error {
}

batch := m.db.NewBatch()
defer batch.Close()

if err := batch.Set(csKey, csBytes); err != nil {
return fmt.Errorf("failed to write changeset to db.Batch: %w", err)
}
if err := batch.Write(); err != nil {
return fmt.Errorf("failed to write changeset to db: %w", err)
// Invoking this code in a closure so that defer is called immediately on return
// yet not in the for-loop which can leave resource lingering.
err = func() error {
defer batch.Close()

if err := batch.Set(csKey, csBytes); err != nil {
return fmt.Errorf("failed to write changeset to db.Batch: %w", err)
}
if err := batch.Write(); err != nil {
return fmt.Errorf("failed to write changeset to db: %w", err)
}
return nil
}()
if err != nil {
return err
}
}

Expand Down
17 changes: 12 additions & 5 deletions store/snapshots/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,7 @@ func (s *Store) Load(height uint64, format uint32) (*types.Snapshot, <-chan io.R
}

ch := make(chan io.ReadCloser)

go func() {
defer close(ch)
for i := uint32(0); i < snapshot.Chunks; i++ {
Expand All @@ -172,14 +173,20 @@ func (s *Store) Load(height uint64, format uint32) (*types.Snapshot, <-chan io.R
_ = pw.CloseWithError(err)
return
}
defer chunk.Close()
tuantran1702 marked this conversation as resolved.
Show resolved Hide resolved
_, err = io.Copy(pw, chunk)
err = func() error {
defer chunk.Close()
if _, err := io.Copy(pw, chunk); err != nil {
return fmt.Errorf("failed to copy chunk %d: %w", i, err)
}
if err != nil {
_ = pw.CloseWithError(err)
return err
}
return pw.Close()
}()
if err != nil {
odeke-em marked this conversation as resolved.
Show resolved Hide resolved
_ = pw.CloseWithError(err)
return
}
chunk.Close()
pw.Close()
}
}()

Expand Down
12 changes: 8 additions & 4 deletions store/storage/storage_test_suite.go
Original file line number Diff line number Diff line change
Expand Up @@ -256,8 +256,6 @@ func (s *StorageTestSuite) TestDatabase_Iterator() {
itr, err := db.Iterator(storeKey1Bytes, v, []byte("key000"), nil)
s.Require().NoError(err)

defer itr.Close()

var i, count int
for ; itr.Valid(); itr.Next() {
s.Require().Equal([]byte(fmt.Sprintf("key%03d", i)), itr.Key(), string(itr.Key()))
Expand All @@ -271,15 +269,16 @@ func (s *StorageTestSuite) TestDatabase_Iterator() {

// seek past domain, which should make the iterator invalid and produce an error
s.Require().False(itr.Valid())

err = itr.Close()
s.Require().NoError(err, "Failed to close iterator")
}

// iterator with a start and end domain over multiple versions
for v := uint64(1); v < 5; v++ {
itr2, err := db.Iterator(storeKey1Bytes, v, []byte("key010"), []byte("key019"))
s.Require().NoError(err)

defer itr2.Close()

i, count := 10, 0
for ; itr2.Valid(); itr2.Next() {
s.Require().Equal([]byte(fmt.Sprintf("key%03d", i)), itr2.Key())
Expand All @@ -293,6 +292,11 @@ func (s *StorageTestSuite) TestDatabase_Iterator() {

// seek past domain, which should make the iterator invalid and produce an error
s.Require().False(itr2.Valid())

err = itr2.Close()
if err != nil {
return
}
}

// start must be <= end
Expand Down
3 changes: 2 additions & 1 deletion tests/integration/auth/client/cli/suite_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -207,14 +207,15 @@ func (s *CLITestSuite) TestCLISignBatchTotalFees() {
sdk.NewCoins(sendTokens), clitestutil.TestTxConfig{GenOnly: true})
s.Require().NoError(err)
txFile := testutil.WriteToNewTempFile(s.T(), tx.String()+"\n")
defer txFile.Close()
txFiles[i] = txFile.Name()

unsignedTx, err := txCfg.TxJSONDecoder()(tx.Bytes())
s.Require().NoError(err)
txBuilder, err := txCfg.WrapTxBuilder(unsignedTx)
s.Require().NoError(err)
expectedBatchedTotalFee += txBuilder.GetTx().GetFee().AmountOf(tc.denom).Int64()
err = txFile.Close()
s.NoError(err)
}

// Test batch sign
Expand Down
57 changes: 30 additions & 27 deletions x/group/keeper/invariants.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,14 +55,14 @@ func GroupTotalWeightInvariantHelper(ctx sdk.Context, storeService storetypes.KV
msg += fmt.Sprintf("LoadNext failure on group table iterator\n%v\n", err)
return msg, broken
}

groups[groupInfo.Id] = groupInfo
}

groupByIDs := maps.Keys(groups)
sort.Slice(groupByIDs, func(i, j int) bool {
return groupByIDs[i] < groupByIDs[j]
})

for _, groupID := range groupByIDs {
groupInfo := groups[groupID]
membersWeight, err := groupmath.NewNonNegativeDecFromString("0")
Expand All @@ -71,35 +71,38 @@ func GroupTotalWeightInvariantHelper(ctx sdk.Context, storeService storetypes.KV
return msg, broken
}

memIt, err := groupMemberByGroupIndex.Get(kvStore, groupInfo.Id)
if err != nil {
msg += fmt.Sprintf("error while returning group member iterator for group with ID %d\n%v\n", groupInfo.Id, err)
return msg, broken
}
defer memIt.Close()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Your change to move it down below creates an even less likely chance that memIt.Close() due to the large number of branches that could return before it is invoked. You could apply that anonymous closure trick that I showed up above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Much appreciated @odeke-em for your insightful review. I have refactored the code, could you kindly take a look. Thank you!


for {
var groupMember group.GroupMember
_, err = memIt.LoadNext(&groupMember)
if errors.ErrORMIteratorDone.Is(err) {
break
}
if err != nil {
msg += fmt.Sprintf("LoadNext failure on member table iterator\n%v\n", err)
return msg, broken
}

curMemWeight, err := groupmath.NewPositiveDecFromString(groupMember.GetMember().GetWeight())
err = func() error {
memIt, err := groupMemberByGroupIndex.Get(kvStore, groupInfo.Id)
if err != nil {
msg += fmt.Sprintf("error while parsing non-nengative decimal for group member %s\n%v\n", groupMember.Member.Address, err)
return msg, broken
return fmt.Errorf("error while returning group member iterator for group with ID %d\n%v", groupInfo.Id, err)
}

membersWeight, err = groupmath.Add(membersWeight, curMemWeight)
if err != nil {
msg += fmt.Sprintf("decimal addition error while adding group member voting weight to total voting weight\n%v\n", err)
return msg, broken
defer memIt.Close()

for {
var groupMember group.GroupMember
_, err = memIt.LoadNext(&groupMember)
if errors.ErrORMIteratorDone.Is(err) {
break
Copy link
Contributor

Choose a reason for hiding this comment

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

personal preference: break is good and does the job but you can also do return nil here already to exit early. There is nothing going on after the loop and a return would make it slightly more readable.

}
if err != nil {
return fmt.Errorf("LoadNext failure on member table iterator\n%v", err)
}

curMemWeight, err := groupmath.NewPositiveDecFromString(groupMember.GetMember().GetWeight())
if err != nil {
return fmt.Errorf("error while parsing non-nengative decimal for group member %s\n%v", groupMember.Member.Address, err)
}

membersWeight, err = groupmath.Add(membersWeight, curMemWeight)
if err != nil {
return fmt.Errorf("decimal addition error while adding group member voting weight to total voting weight\n%v", err)
}
}
return nil
}()
if err != nil {
msg += err.Error() + "\n"
return msg, broken
}

groupWeight, err := groupmath.NewNonNegativeDecFromString(groupInfo.GetTotalWeight())
Expand Down
Loading