fix: Replace defer with explicit Close() calls in export/import loops #1109
Conversation
WalkthroughThe update replaces deferred resource closure with explicit calls to Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Main
participant Exporter
participant Importer
User->>Main: Start export/import
Main->>Exporter: Create exporter
Main->>Exporter: Loop: Export data
Main->>Exporter: exporter.Close()
Main->>Importer: Create importer
Main->>Importer: Loop: Import data
Main->>Importer: importer.Close()
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| export = append(export, node) | ||
| stats.AddNode(node) | ||
| } | ||
| exporter.Close() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential resource leak on early error return.
While moving from defer to explicit Close() addresses the accumulation issue in loops, the current placement could lead to resource leaks if errors occur in the processing loop (lines 129-138).
Consider refactoring the per-store logic into a helper function where defer can be used safely:
+func exportStore(name string, ldb idbm.DB, version int64) ([]*iavl.ExportNode, Stats, error) {
+ db := idbm.NewPrefixDB(ldb, []byte("s/k:"+name+"/"))
+ tree := iavl.NewMutableTree(db, 0, false, iavl.NewNopLogger())
+
+ stats := Stats{}
+ export := make([]*iavl.ExportNode, 0, 100000)
+
+ storeVersion, err := tree.LoadVersion(0)
+ if err != nil {
+ return nil, stats, err
+ }
+ if storeVersion == 0 {
+ return export, stats, nil
+ }
+
+ itree, err := tree.GetImmutable(version)
+ if err != nil {
+ return nil, stats, err
+ }
+
+ start := time.Now().UTC()
+ exporter, err := itree.Export()
+ if err != nil {
+ return nil, stats, err
+ }
+ defer exporter.Close()
+
+ for {
+ node, err := exporter.Next()
+ if errors.Is(err, iavl.ErrorExportDone) {
+ break
+ } else if err != nil {
+ return nil, stats, err
+ }
+ export = append(export, node)
+ stats.AddNode(node)
+ }
+
+ stats.AddDurationSince(start)
+ return export, stats, nil
+}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In benchmarks/cosmos-exim/main.go at line 139, calling exporter.Close()
explicitly here risks resource leaks if an error occurs earlier in the
processing loop (lines 129-138). Refactor the per-store processing logic into a
separate helper function so you can use defer exporter.Close() safely within
that function, ensuring the exporter is always closed properly even if errors
occur.
| if err != nil { | ||
| return err | ||
| } | ||
| importer.Close() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential resource leak on early error return.
Similar to the export function, this explicit Close() call could be skipped if errors occur in the processing loop (lines 176-182) or during commit (line 183).
Consider refactoring to a helper function:
+func importStore(name string, version int64, nodes []*iavl.ExportNode) (Stats, error) {
+ tempdir, err := os.MkdirTemp("", name)
+ if err != nil {
+ return Stats{}, err
+ }
+ defer os.RemoveAll(tempdir)
+
+ start := time.Now()
+ stats := Stats{}
+
+ newDB, err := idbm.NewGoLevelDB(name, tempdir)
+ if err != nil {
+ return stats, err
+ }
+
+ newTree := iavl.NewMutableTree(newDB, 0, false, iavl.NewNopLogger())
+ importer, err := newTree.Import(version)
+ if err != nil {
+ return stats, err
+ }
+ defer importer.Close()
+
+ for _, node := range nodes {
+ err = importer.Add(node)
+ if err != nil {
+ return stats, err
+ }
+ stats.AddNode(node)
+ }
+
+ err = importer.Commit()
+ if err != nil {
+ return stats, err
+ }
+
+ stats.AddDurationSince(start)
+ return stats, nil
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| importer.Close() | |
| func importStore(name string, version int64, nodes []*iavl.ExportNode) (Stats, error) { | |
| tempdir, err := os.MkdirTemp("", name) | |
| if err != nil { | |
| return Stats{}, err | |
| } | |
| defer os.RemoveAll(tempdir) | |
| start := time.Now() | |
| stats := Stats{} | |
| newDB, err := idbm.NewGoLevelDB(name, tempdir) | |
| if err != nil { | |
| return stats, err | |
| } | |
| newTree := iavl.NewMutableTree(newDB, 0, false, iavl.NewNopLogger()) | |
| importer, err := newTree.Import(version) | |
| if err != nil { | |
| return stats, err | |
| } | |
| defer importer.Close() | |
| for _, node := range nodes { | |
| if err = importer.Add(node); err != nil { | |
| return stats, err | |
| } | |
| stats.AddNode(node) | |
| } | |
| if err = importer.Commit(); err != nil { | |
| return stats, err | |
| } | |
| stats.AddDurationSince(start) | |
| return stats, nil | |
| } |
🤖 Prompt for AI Agents
In benchmarks/cosmos-exim/main.go at line 187, the explicit call to
importer.Close() may be skipped if an error occurs earlier in the processing
loop or during commit, causing a resource leak. Refactor by moving the Close()
call into a deferred function immediately after the importer is created,
ensuring it always executes regardless of errors. This can be done by defining a
helper function or using defer directly after importer initialization.
MozirDmitriy
changed the title
Replaced defer exporter.Close() and defer importer.Close() with explicit Close() calls inside the export and import loops for each store.
This change ensures that resources (such as file descriptors or handles) are released immediately after processing each store, rather than being held until the end of the entire function.
Using defer inside a loop causes all deferred calls to accumulate and execute only when the surrounding function returns, which can lead to excessive resource usage or even resource exhaustion if the number of stores is large.
Explicitly closing the exporter/importer after each store improves resource management and prevents potential leaks.
Summary by CodeRabbit