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.

Sign up for GitHub

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

Jump to bottom

fix: pass FinalizeBlock request and response to ABCIListener #18486

Merged
merged 5 commits into from
Nov 16, 2023
Merged

fix: pass FinalizeBlock request and response to ABCIListener #18486

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2eb307f
fix: pass FinalizeBlock request and response to ABCIListener
MSalopek Nov 16, 2023
fccfed1
chore: add changelog entry
MSalopek Nov 16, 2023
9f820c4
fix: log errors in FinalizeBlock ABCIListeners instead of panicking
MSalopek Nov 16, 2023
9ba0740
Update baseapp/abci.go
MSalopek Nov 16, 2023
caa228b
chore: appease lint rules
MSalopek Nov 16, 2023
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ Ref: https://keepachangelog.com/en/1.0.0/

### Bug Fixes

* (baseapp) [#18486](https://github.com/cosmos/cosmos-sdk/pull/18486) Fixed FinalizeBlock calls not being passed to ABCIListeners
* (baseapp) [#18383](https://github.com/cosmos/cosmos-sdk/pull/18383) Fixed a data race inside BaseApp.getContext, found by end-to-end (e2e) tests.
* (client/server) [#18345](https://github.com/cosmos/cosmos-sdk/pull/18345) Consistently set viper prefix in client and server. It defaults for the binary name for both client and server.
* (simulation) [#17911](https://github.com/cosmos/cosmos-sdk/pull/17911) Fix all problems with executing command `make test-sim-custom-genesis-fast` for simulation test.
Expand Down
8 changes: 8 additions & 0 deletions baseapp/abci.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -891,6 +891,14 @@ func (app *BaseApp) FinalizeBlock(req *abci.RequestFinalizeBlock) (*abci.Respons
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 {
tac0turtle marked this conversation as resolved.
Show resolved Hide resolved
app.logger.Error("ListenFinalizeBlock listening hook failed, height: %d, err: %w", req.Height, err)
MSalopek marked this conversation as resolved.
Show resolved Hide resolved
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The code correctly iterates over the ABCIListeners and calls the ListenFinalizeBlock method for each listener. However, there are a few considerations to be made:

  1. Error Handling: The error from ListenFinalizeBlock is 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?

  2. 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.

  3. 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.


return res, err
}

Expand Down
Loading