fix: pass FinalizeBlock request and response to ABCIListener #18486
Conversation
WalkthroughWalkthroughThe changes involve updates to the Changes
TipsChat with CodeRabbit Bot (
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
baseapp/abci.go
Outdated
| // call the streaming service hooks with the FinalizeBlock messages | ||
| for _, streamingListener := range app.streamingManager.ABCIListeners { | ||
| if err := streamingListener.ListenFinalizeBlock(app.finalizeBlockState.ctx, *req, *res); err != nil { | ||
| panic(fmt.Errorf("ListenFinalizeBlock listening hook failed, height: %d, err: %w", req.Height, err)) | ||
| } | ||
| } |
There was a problem hiding this comment.
The code correctly iterates over all ABCIListeners and calls the ListenFinalizeBlock method. However, the use of panic to handle errors from listeners is a bit aggressive. While it is true that a failure in the listeners could indicate a critical system failure that requires immediate attention, panicking will abruptly terminate the application, which might not always be the desired behavior. Consider logging the error and deciding on a recovery strategy that does not necessarily involve panicking, unless this behavior is an explicit requirement.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- CHANGELOG.md (1 hunks)
Additional comments: 1
CHANGELOG.md (1)
- 69-69: The changelog entry is clear and concise, providing direct links to the pull requests for further details. It correctly summarizes the changes made in the pull requests, which include bug fixes and improvements across different modules of the Cosmos SDK. The use of parentheses to indicate the specific part of the SDK that is affected (e.g.,
baseapp,client/server,simulation) is helpful for readers to quickly understand the scope of the changes.
baseapp/abci.go
Outdated
| // call the streaming service hooks with the FinalizeBlock messages | ||
| for _, streamingListener := range app.streamingManager.ABCIListeners { | ||
| if err := streamingListener.ListenFinalizeBlock(app.finalizeBlockState.ctx, *req, *res); err != nil { | ||
| panic(fmt.Errorf("ListenFinalizeBlock listening hook failed, height: %d, err: %w", req.Height, err)) |
There was a problem hiding this comment.
We should return an error. It will still panic then.
There was a problem hiding this comment.
What about just logging the error like in ListenCommit?
for _, abciListener := range abciListeners {
if err := abciListener.ListenCommit(ctx, *resp, changeSet); err != nil {
app.logger.Error("Commit listening hook failed", "height", blockHeight, "err", err)
}
}
There was a problem hiding this comment.
lets return an error instead of panicking
There was a problem hiding this comment.
So, just log the error without panicking? Instrucions are a bit unclear
There was a problem hiding this comment.
I think we should stay consistent, so if we return an error, we need to update it for the commit listening hook. If we don't, then this one should as well just log.
Personally, maybe it is a bit hard to stop a node if a streaming method returns an error.
There was a problem hiding this comment.
ah sorry, was thinking wrong, we dont want to error here. There is some consistency within streamer but the streamer should not panic or halt the node in baseapp
There was a problem hiding this comment.
only log, sorry for confusion
There was a problem hiding this comment.
Fix applied here. Listeners will only log and never panic or return errors.
julienrbrt
added
the
backport/v0.50.x
MSalopek
force-pushed
the
masa/18485-abci-listener-finalize-block
branch
from
e465962
to
9f820c4
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (2)
- CHANGELOG.md (1 hunks)
- baseapp/abci.go (1 hunks)
Additional comments: 1
CHANGELOG.md (1)
- 69-69: The changelog entries are clear and provide direct links to the pull requests for easy reference. Each entry is categorized by the part of the system it affects (e.g., baseapp, client/server, simulation), which is good for readability and understanding the scope of the changes.
baseapp/abci.go
Outdated
| if res != nil { | ||
| res.AppHash = app.workingHash() | ||
| } | ||
|
|
||
| // call the streaming service hooks with the FinalizeBlock messages | ||
| for _, streamingListener := range app.streamingManager.ABCIListeners { | ||
| if err := streamingListener.ListenFinalizeBlock(app.finalizeBlockState.ctx, *req, *res); err != nil { | ||
| app.logger.Error("ListenFinalizeBlock listening hook failed, height: %d, err: %w", req.Height, err) | ||
| } | ||
| } |
There was a problem hiding this comment.
The code correctly iterates over the ABCIListeners and calls the ListenFinalizeBlock method for each listener. However, there are a few considerations to be made:
-
Error Handling: The error from
ListenFinalizeBlockis logged but not handled beyond that. Depending on the criticality of the listener's actions, you might want to consider how errors should be handled. Should the error affect the finalization process, or should it only be logged? -
Logging Context: The error logging does not provide the context of which listener failed. Including the listener's identifier in the log message could be helpful for debugging.
-
Context Cancellation: There is no check for context cancellation before calling the listeners. If the context is canceled, it might be prudent to skip the listener invocation or handle it differently.
Here's a proposed change that addresses the logging context issue:
- app.logger.Error("ListenFinalizeBlock listening hook failed, height: %d, err: %w", req.Height, err)
+ app.logger.Error("ListenFinalizeBlock listening hook failed", "listener", streamingListener.Name(), "height", req.Height, "err", err)And for context cancellation, you could add a check before the loop:
select {
case <-app.finalizeBlockState.ctx.Done():
app.logger.Info("Context cancelled before invoking ABCIListeners")
return res, app.finalizeBlockState.ctx.Err()
default:
// continue to invoke listeners
}Please verify if these changes align with the intended behavior and error handling strategy of the application.
Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>
|
after fixing the lint issue, it will be merged automatically |
Head branch was pushed to by a user without write access
| if res != nil { | ||
| res.AppHash = app.workingHash() | ||
| } | ||
|
|
||
| // call the streaming service hooks with the FinalizeBlock messages | ||
| for _, streamingListener := range app.streamingManager.ABCIListeners { | ||
| if err := streamingListener.ListenFinalizeBlock(app.finalizeBlockState.ctx, *req, *res); err != nil { | ||
| app.logger.Error("ListenFinalizeBlock listening hook failed", "height", req.Height, "err", err) | ||
| } | ||
| } | ||
|
|
||
| return res, err | ||
| } |
There was a problem hiding this comment.
The added code block correctly implements the logic to call ListenFinalizeBlock on each ABCIListener registered with the streamingManager. However, there are a few considerations to ensure robustness:
-
Error Handling: The error from
ListenFinalizeBlockis logged but not handled beyond that. If the error indicates a critical failure that should prevent the block from finalizing, this should be considered. If the listeners are not critical to block finalization, the current approach is acceptable. -
Context Cancellation: The context passed to
ListenFinalizeBlockis thefinalizeBlockState.ctx. Ensure that this context is appropriate for the listeners and consider if a separate context with a timeout should be used for each listener to prevent one listener from blocking others in case it hangs or takes too long. -
Logging: The error logging is good, but it might be beneficial to also log successful calls to
ListenFinalizeBlockfor debugging and monitoring purposes. -
Performance: If there are many listeners and
ListenFinalizeBlockis not a lightweight operation, this could introduce a performance bottleneck. Consider if these calls should be made concurrently. -
Resilience: If one listener fails, the loop continues to the next. This is typically desired behavior, but ensure that this aligns with the application's fault tolerance requirements.
-
State Mutation: If
ListenFinalizeBlockmutates shared state, ensure that it's safe from race conditions.
Here's a suggestion to log successful listener invocations and to consider the use of goroutines for concurrent execution if appropriate:
// call the streaming service hooks with the FinalizeBlock messages
for _, streamingListener := range app.streamingManager.ABCIListeners {
if err := streamingListener.ListenFinalizeBlock(app.finalizeBlockState.ctx, *req, *res); err != nil {
app.logger.Error("ListenFinalizeBlock listening hook failed", "height", req.Height, "err", err)
} else {
+ app.logger.Info("ListenFinalizeBlock listening hook succeeded", "height", req.Height)
+ }
}And if concurrency is deemed necessary:
var wg sync.WaitGroup
for _, streamingListener := range app.streamingManager.ABCIListeners {
wg.Add(1)
go func(listener ABCIListener) {
defer wg.Done()
if err := listener.ListenFinalizeBlock(app.finalizeBlockState.ctx, *req, *res); err != nil {
app.logger.Error("ListenFinalizeBlock listening hook failed", "height", req.Height, "err", err)
} else {
app.logger.Info("ListenFinalizeBlock listening hook succeeded", "height", req.Height)
}
}(streamingListener)
}
wg.Wait()Note that if you choose to execute the listeners concurrently, you must ensure that any shared state accessed by ListenFinalizeBlock is thread-safe.
Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> (cherry picked from commit a27cfa0) # Conflicts: # CHANGELOG.md
Description
Closes: #18485
ABCIListenerhooks were not being called inFinalizeBlock.This PR passes FinalizeBlock request/response to registered
ABCIListeners.Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!to the type prefix if API or client breaking changeCHANGELOG.mdmake lintandmake testReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!in the type prefix if API or client breaking changeSummary by CodeRabbit
New Features
FinalizeBlockmessages, enhancing real-time data processing capabilities.Bug Fixes
make test-sim-custom-genesis-fastcommand for simulation tests.Improvements
FinalizeBlockexecution.Documentation