fix: multiple temp directories on command executions #8726
Conversation
WalkthroughThe changes in the codebase introduce a new deferred function to ensure that the temporary application ( Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
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}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
cmd/osmosisd/cmd/root.go (1)
Line range hint
489-496: Approve changes with a suggestion for cleanup timing.The modifications to the
tempDirfunction improve isolation and error handling. However, the cleanup logic can be improved.Consider moving the
defer os.RemoveAll(dir)to the calling function. This ensures that the temporary directory is cleaned up after it has been used, rather than immediately after creation. Here's a suggested modification:func tempDir() string { dir, err := os.MkdirTemp("", "osmosisd") if err != nil { panic(fmt.Sprintf("failed creating temp directory: %s", err.Error())) } - defer os.RemoveAll(dir) return dir }Then, in the calling function (likely
NewRootCmd), add:tempDir := tempDir() defer os.RemoveAll(tempDir)This ensures that the temporary directory is removed after it has been used by the application.
| defer func() { | ||
| if err := tempApp.Close(); err != nil { | ||
| panic(err) | ||
| } | ||
| if tempDir != osmosis.DefaultNodeHome { | ||
| os.RemoveAll(tempDir) | ||
| } | ||
| }() |
There was a problem hiding this comment.
Approve changes with a suggestion for improved error handling.
The addition of the deferred cleanup function is a good practice for resource management. It ensures that the temporary app is properly closed and the temporary directory is removed when necessary.
Consider improving the error handling in the deferred function. Instead of panicking, log the error. This allows the application to continue running even if there's an issue during cleanup. Here's a suggested modification:
defer func() {
if err := tempApp.Close(); err != nil {
- panic(err)
+ logger.Error("failed to close temp app", "error", err)
}
if tempDir != osmosis.DefaultNodeHome {
- os.RemoveAll(tempDir)
+ if err := os.RemoveAll(tempDir); err != nil {
+ logger.Error("failed to remove temp directory", "error", err)
+ }
}
}()This change ensures that any errors during cleanup are logged but don't cause the application to panic, which could be disruptive in a production environment.
Committable suggestion was skipped due to low confidence.
PaddyMc
added
V:state/compatible/backport
|
Thanks for the PR, would you be able to add a changelog? |
Updated changelog will that work ? |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
CHANGELOG.md (1)
Old sdkmath types still in use
The following files still use the old
sdkmathtypes (DecCoin,DecCoins):
x/concentrated-liquidity/types/incentive_record.pb.gox/concentrated-liquidity/model/tick_info.pb.gox/concentrated-liquidity/client/queryproto/query.pb.goosmoutils/accum/accum.pb.goAnalysis chain
Line range hint
98-99: API Break: Switch to sdkmath typesThis is a significant change that affects many parts of the codebase. The switch to sdkmath types can improve consistency but requires careful migration. Pay attention to the note about Dec and Int initialization behavior changes.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any remaining uses of the old math types echo "Searching for old math types usage:" rg --type go 'github\.com/cosmos/cosmos-sdk/types\.(Dec|Int)' || echo "No old math types found."Length of output: 2591
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- CHANGELOG.md (1 hunks)
Additional comments not posted (16)
CHANGELOG.md (16)
55-56: LGTM: Minor state export fix for cwpool gaugesThis change appears to be a minor fix for state export related to cwpool gauges. It's good to see improvements in state export functionality.
57-57: LGTM: IAVL version bumpUpgrading IAVL to v0.19.7 is a good practice to keep dependencies up-to-date. This may include performance improvements or bug fixes.
Line range hint
61-62: New feature: SDK.Coins helpers addedThe addition of Mul and Quo helpers for sdk.Coins in osmoutils is a useful feature that can simplify coin calculations in the codebase.
Line range hint
63-63: New feature: Mutative version of QuoRoundUp addedAdding a mutative version of QuoRoundUp can help improve performance in certain scenarios by reducing allocations.
Line range hint
64-64: New feature: Efficient BigDec truncationsThe addition of mutative and efficient BigDec truncations with arbitrary decimals is a valuable performance improvement.
Line range hint
65-65: New feature: Query for number of initialized ticks in CLThis new query for the number of initialized ticks in Concentrated Liquidity (CL) can be useful for analytics and debugging purposes.
Line range hint
66-66: New feature: V2 SpotPrice CLI and GRPC queryThe addition of a v2 SpotPrice query with 36 decimals in poolmanager is a significant improvement in precision for price queries.
Line range hint
70-70: API Break: PoolModuleI CalculateSpotPrice return type changeChanging the return type of CalculateSpotPrice to BigDec is an API breaking change. Ensure that all dependent code is updated accordingly.
Line range hint
71-71: API Break: Removal of redundant param in CreateGaugeRefKeysRemoving a redundant parameter from CreateGaugeRefKeys in incentives is a good cleanup, but it's an API breaking change. Make sure all callers are updated.
Line range hint
72-72: API Break: Removal of redundant ctx param in DeleteAllKeysFromPrefixRemoving the redundant ctx parameter from DeleteAllKeysFromPrefix in osmoutils is another API breaking change. Ensure all usage is updated.
Line range hint
76-80: LGTM: New features and performance improvementsThese additions, including SDK.Coins helpers, mutative versions of mathematical operations, and efficient BigDec truncations, are all valuable improvements that can enhance performance and developer experience.
Line range hint
81-81: New feature: Query for number of initialized ticks in CLThis new query can be useful for analytics and debugging in the Concentrated Liquidity module.
Line range hint
85-87: Bug fixes: Improvements to CL math and swap operationsThese bug fixes address important issues in the Concentrated Liquidity module, including reducing error blow-up in calculations and improving rounding behavior. These changes can lead to more accurate and stable operations.
Line range hint
88-88: Bug fix: Convert Int to BigDec in CL mathThis change can help prevent potential precision loss or overflow issues in Concentrated Liquidity calculations.
Line range hint
92-97: API Breaks: Changes in CL module and poolmanagerThese API breaks involve changes to function signatures and return types. While they may improve the overall design and functionality, ensure that all dependent code is updated accordingly. Special attention should be given to the SpotPrice API change, as it affects multiple modules.
Line range hint
1-5589: Overall CHANGELOG reviewThe CHANGELOG for v19.2.0 and v19.1.0 shows a focus on performance improvements, bug fixes, and API enhancements, particularly in the Concentrated Liquidity (CL) module and mathematical operations. The changes are well-documented, and breaking changes are clearly marked.
Key points:
- Several performance optimizations have been implemented, which should improve overall system efficiency.
- New queries and features have been added, enhancing functionality and developer tools.
- There are multiple API breaks, mainly in the CL module and mathematical operations. These changes seem to improve the design but require careful migration of dependent code.
- The switch to sdkmath types is a significant change that affects many parts of the codebase and requires special attention during migration.
Recommendations:
- Ensure comprehensive testing of all affected modules, especially the CL module and areas using mathematical operations.
- Provide detailed migration guides for developers to address the API breaks and the switch to sdkmath types.
- Consider adding more context or links to relevant documentation for some of the more complex changes.
Overall, the changes appear to be positive improvements to the system, but careful attention to breaking changes is necessary during the upgrade process.
|
No that's perfect, ty |
* delete temp dir after it's usage * update changelog (cherry picked from commit 4ce0dbf) # Conflicts: # CHANGELOG.md
…#8729) * fix: multiple temp directories on command executions (#8726) * delete temp dir after it's usage * update changelog (cherry picked from commit 4ce0dbf) # Conflicts: # CHANGELOG.md * chore: update changelog --------- Co-authored-by: Marri Harish <[email protected]> Co-authored-by: ghost <[email protected]>
Closes: #8725
What is the purpose of the change
This pull request fix issue with temp directories and stops creating multiple temp directories
also gives panic if node is unable to create a temp directory instead of returning node default home directory which leads to erasing of entire node data