From 80470d91d1f5dfb1cbc64b55e067b00e7f8bb56e Mon Sep 17 00:00:00 2001 From: Mikhail Kornilov Date: Wed, 22 Apr 2020 00:23:32 +0300 Subject: [PATCH 1/6] baseapp: custom panic handling withing app.runTx method --- baseapp/baseapp.go | 35 ++++++++----------- baseapp/baseapp_test.go | 43 +++++++++++++++++++++++ baseapp/recovery.go | 77 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 134 insertions(+), 21 deletions(-) create mode 100644 baseapp/recovery.go diff --git a/baseapp/baseapp.go b/baseapp/baseapp.go index 26bf3274947a..2ec626549478 100644 --- a/baseapp/baseapp.go +++ b/baseapp/baseapp.go @@ -3,7 +3,6 @@ package baseapp import ( "fmt" "reflect" - "runtime/debug" "strings" abci "github.com/tendermint/tendermint/abci/types" @@ -94,6 +93,9 @@ type BaseApp struct { // nolint: maligned // application's version string appVersion string + + // recovery handler for app.runTx method + runTxRecoveryMiddleware recoveryMiddleware } // NewBaseApp returns a reference to an initialized BaseApp. It accepts a @@ -124,6 +126,8 @@ func NewBaseApp( app.cms.SetInterBlockCache(app.interBlockCache) } + app.runTxRecoveryMiddleware = newDefaultRecoveryMiddleware() + return app } @@ -345,6 +349,13 @@ func (app *BaseApp) GetConsensusParams(ctx sdk.Context) *abci.ConsensusParams { return cp } +// AddRunTxRecoveryHandler adds custom app.runTx panic handlers. +func (app *BaseApp) AddRunTxRecoveryHandler(handlers ...RecoveryHandler) { + for _, h := range handlers { + app.runTxRecoveryMiddleware = newRecoveryMiddleware(h, app.runTxRecoveryMiddleware) + } +} + func (app *BaseApp) storeConsensusParams(ctx sdk.Context, cp *abci.ConsensusParams) { if app.paramStore == nil { panic("cannot store consensus params with no params store set") @@ -485,26 +496,8 @@ func (app *BaseApp) runTx(mode runTxMode, txBytes []byte, tx sdk.Tx) (gInfo sdk. defer func() { if r := recover(); r != nil { - switch rType := r.(type) { - // TODO: Use ErrOutOfGas instead of ErrorOutOfGas which would allow us - // to keep the stracktrace. - case sdk.ErrorOutOfGas: - err = sdkerrors.Wrap( - sdkerrors.ErrOutOfGas, fmt.Sprintf( - "out of gas in location: %v; gasWanted: %d, gasUsed: %d", - rType.Descriptor, gasWanted, ctx.GasMeter().GasConsumed(), - ), - ) - - default: - err = sdkerrors.Wrap( - sdkerrors.ErrPanic, fmt.Sprintf( - "recovered: %v\nstack:\n%v", r, string(debug.Stack()), - ), - ) - } - - result = nil + recoveryMW := newOutOfGasRecoveryMiddleware(gasWanted, ctx, app.runTxRecoveryMiddleware) + err, result = processRecovery(r, recoveryMW), nil } gInfo = sdk.GasInfo{GasWanted: gasWanted, GasUsed: ctx.GasMeter().GasConsumed()} diff --git a/baseapp/baseapp_test.go b/baseapp/baseapp_test.go index 31f049636477..af49d3a4c155 100644 --- a/baseapp/baseapp_test.go +++ b/baseapp/baseapp_test.go @@ -1265,6 +1265,49 @@ func TestMaxBlockGasLimits(t *testing.T) { } } +// Test custom panic handling within app.DeliverTx method +func TestCustomRunTxPanicHandler(t *testing.T) { + const customPanicMsg = "test panic" + anteErr := sdkerrors.Register("fakeModule", 100500, "fakeError") + + anteOpt := func(bapp *BaseApp) { + bapp.SetAnteHandler(func(ctx sdk.Context, tx sdk.Tx, simulate bool) (newCtx sdk.Context, err error) { + panic(sdkerrors.Wrap(anteErr, "anteHandler")) + return + }) + } + routerOpt := func(bapp *BaseApp) { + bapp.Router().AddRoute(routeMsgCounter, func(ctx sdk.Context, msg sdk.Msg) (*sdk.Result, error) { + return &sdk.Result{}, nil + }) + } + + app := setupBaseApp(t, anteOpt, routerOpt) + + header := abci.Header{Height: 1} + app.BeginBlock(abci.RequestBeginBlock{Header: header}) + + app.AddRunTxRecoveryHandler(func(recoveryObj interface{}) error { + err, ok := recoveryObj.(error) + if !ok { + return nil + } + + if anteErr.Is(err) { + panic(customPanicMsg) + } else { + return nil + } + }) + + // Transaction should panic with custom handler above + { + tx := newTxCounter(0, 0) + + require.PanicsWithValue(t, customPanicMsg, func() { app.Deliver(tx) }) + } +} + func TestBaseAppAnteHandler(t *testing.T) { anteKey := []byte("ante-key") anteOpt := func(bapp *BaseApp) { diff --git a/baseapp/recovery.go b/baseapp/recovery.go new file mode 100644 index 000000000000..41a74db8942f --- /dev/null +++ b/baseapp/recovery.go @@ -0,0 +1,77 @@ +package baseapp + +import ( + "fmt" + "runtime/debug" + + sdk "github.com/cosmos/cosmos-sdk/types" + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" +) + +// RecoveryHandler handles recovery() object: wraps to an error with context / performs custom actions. +type RecoveryHandler func(recoveryObj interface{}) error + +// recoveryMiddleware is wrapper for RecoveryHandler to create chained recovery handling. +type recoveryMiddleware func(recoveryObj interface{}) (recoveryMiddleware, error) + +// processRecovery processes recoveryMiddleware chain for recovery() object. +// Chain processing stops on non-nil error or when chain is processed. +func processRecovery(recoveryObj interface{}, middleware recoveryMiddleware) error { + if middleware == nil { + return nil + } + + next, err := middleware(recoveryObj) + if err != nil { + return err + } + + if next != nil { + return processRecovery(recoveryObj, next) + } + + return nil +} + +// newRecoveryMiddleware creates a RecoveryHandler middleware. +func newRecoveryMiddleware(handler RecoveryHandler, next recoveryMiddleware) recoveryMiddleware { + return func(recoveryObj interface{}) (recoveryMiddleware, error) { + if err := handler(recoveryObj); err != nil { + return nil, err + } + + return next, nil + } +} + +// newOutOfGasRecoveryMiddleware creates a standard OutOfGas recovery middleware for app.runTx method. +func newOutOfGasRecoveryMiddleware(gasWanted uint64, ctx sdk.Context, next recoveryMiddleware) recoveryMiddleware { + handler := func(recoveryObj interface{}) error { + err, ok := recoveryObj.(sdk.ErrorOutOfGas) + if !ok { + return nil + } + + return sdkerrors.Wrap( + sdkerrors.ErrOutOfGas, fmt.Sprintf( + "out of gas in location: %v; gasWanted: %d, gasUsed: %d", + err.Descriptor, gasWanted, ctx.GasMeter().GasConsumed(), + ), + ) + } + + return newRecoveryMiddleware(handler, next) +} + +// newDefaultRecoveryMiddleware creates a default (last in chain) recovery middleware for app.runTx method. +func newDefaultRecoveryMiddleware() recoveryMiddleware { + handler := func(recoveryObj interface{}) error { + return sdkerrors.Wrap( + sdkerrors.ErrPanic, fmt.Sprintf( + "recovered: %v\nstack:\n%v", recoveryObj, string(debug.Stack()), + ), + ) + } + + return newRecoveryMiddleware(handler, nil) +} From e0606c768f47a0335e8916618216553c75580b2e Mon Sep 17 00:00:00 2001 From: Mikhail Kornilov Date: Wed, 22 Apr 2020 14:58:13 +0300 Subject: [PATCH 2/6] baseapp: recovery middleware chain unit test added --- baseapp/recovery_test.go | 64 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) create mode 100644 baseapp/recovery_test.go diff --git a/baseapp/recovery_test.go b/baseapp/recovery_test.go new file mode 100644 index 000000000000..b75892c63818 --- /dev/null +++ b/baseapp/recovery_test.go @@ -0,0 +1,64 @@ +package baseapp + +import ( + "fmt" + "testing" + + "github.com/stretchr/testify/require" +) + +// Test that recovery chain produces expected error at specific middleware layer +func TestRecoveryChain(t *testing.T) { + createError := func(id int) error { + return fmt.Errorf("error from id: %d", id) + } + + createHandler := func(id int, handle bool) RecoveryHandler { + return func(_ interface{}) error { + if handle { + return createError(id) + } + return nil + } + } + + // check recovery chain [1] -> 2 -> 3 + { + mw := newRecoveryMiddleware(createHandler(3, false), nil) + mw = newRecoveryMiddleware(createHandler(2, false), mw) + mw = newRecoveryMiddleware(createHandler(1, true), mw) + receivedErr := processRecovery(nil, mw) + + require.Equal(t, createError(1), receivedErr) + } + + // check recovery chain 1 -> [2] -> 3 + { + mw := newRecoveryMiddleware(createHandler(3, false), nil) + mw = newRecoveryMiddleware(createHandler(2, true), mw) + mw = newRecoveryMiddleware(createHandler(1, false), mw) + receivedErr := processRecovery(nil, mw) + + require.Equal(t, createError(2), receivedErr) + } + + // check recovery chain 1 -> 2 -> [3] + { + mw := newRecoveryMiddleware(createHandler(3, true), nil) + mw = newRecoveryMiddleware(createHandler(2, false), mw) + mw = newRecoveryMiddleware(createHandler(1, false), mw) + receivedErr := processRecovery(nil, mw) + + require.Equal(t, createError(3), receivedErr) + } + + // check recovery chain 1 -> 2 -> 3 + { + mw := newRecoveryMiddleware(createHandler(3, false), nil) + mw = newRecoveryMiddleware(createHandler(2, false), mw) + mw = newRecoveryMiddleware(createHandler(1, false), mw) + receivedErr := processRecovery(nil, mw) + + require.Nil(t, receivedErr) + } +} From b54ae64cca7d0c6e7e87a6f0a920cf1ad5625ec0 Mon Sep 17 00:00:00 2001 From: Mikhail Kornilov Date: Wed, 22 Apr 2020 16:04:54 +0300 Subject: [PATCH 3/6] CHANGELOG.md update --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b45d148567ef..a1ed0a1e4ada 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -217,6 +217,7 @@ functionality that requires an online connection. * (client) [\#5895](https://github.com/cosmos/cosmos-sdk/issues/5895) show config options in the config command's help screen. * (types/rest) [\#5900](https://github.com/cosmos/cosmos-sdk/pull/5900) Add Check*Error function family to spare developers from replicating tons of boilerplate code. * (x/evidence) [\#5952](https://github.com/cosmos/cosmos-sdk/pull/5952) Tendermint Consensus parameters can now be changed via parameter change proposals through `x/gov`. +* (baseapp) [\#6053](https://github.com/cosmos/cosmos-sdk/pull/6053) Panic recovery custom handling (middleware) added for `app.runTx()` method. Adds ability for developers to register custom panic handlers extending standard ones. ## [v0.38.3] - 2020-04-09 From bb16e834225e045a3d3983f8066553a9da8a6417 Mon Sep 17 00:00:00 2001 From: Mikhail Kornilov Date: Wed, 29 Apr 2020 19:25:15 +0300 Subject: [PATCH 4/6] ADR-22 link added; godocs update --- CHANGELOG.md | 2 +- baseapp/baseapp.go | 2 +- baseapp/recovery.go | 12 ++++++------ 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a1ed0a1e4ada..e5bf13207b07 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -217,7 +217,7 @@ functionality that requires an online connection. * (client) [\#5895](https://github.com/cosmos/cosmos-sdk/issues/5895) show config options in the config command's help screen. * (types/rest) [\#5900](https://github.com/cosmos/cosmos-sdk/pull/5900) Add Check*Error function family to spare developers from replicating tons of boilerplate code. * (x/evidence) [\#5952](https://github.com/cosmos/cosmos-sdk/pull/5952) Tendermint Consensus parameters can now be changed via parameter change proposals through `x/gov`. -* (baseapp) [\#6053](https://github.com/cosmos/cosmos-sdk/pull/6053) Panic recovery custom handling (middleware) added for `app.runTx()` method. Adds ability for developers to register custom panic handlers extending standard ones. +* (baseapp) [\#6053](https://github.com/cosmos/cosmos-sdk/pull/6053) Customizable panic recovery handling added for `app.runTx()` method (as proposed in the [ADR 22](https://github.com/cosmos/tree/master/docs/architecture/adr-022-custom-panic-handling.md)). Adds ability for developers to register custom panic handlers extending standard ones. ## [v0.38.3] - 2020-04-09 diff --git a/baseapp/baseapp.go b/baseapp/baseapp.go index 2ec626549478..67cb95b5bede 100644 --- a/baseapp/baseapp.go +++ b/baseapp/baseapp.go @@ -349,7 +349,7 @@ func (app *BaseApp) GetConsensusParams(ctx sdk.Context) *abci.ConsensusParams { return cp } -// AddRunTxRecoveryHandler adds custom app.runTx panic handlers. +// AddRunTxRecoveryHandler adds custom app.runTx method panic handlers. func (app *BaseApp) AddRunTxRecoveryHandler(handlers ...RecoveryHandler) { for _, h := range handlers { app.runTxRecoveryMiddleware = newRecoveryMiddleware(h, app.runTxRecoveryMiddleware) diff --git a/baseapp/recovery.go b/baseapp/recovery.go index 41a74db8942f..7f0687800c65 100644 --- a/baseapp/recovery.go +++ b/baseapp/recovery.go @@ -8,10 +8,14 @@ import ( sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" ) -// RecoveryHandler handles recovery() object: wraps to an error with context / performs custom actions. +// RecoveryHandler handles recovery() object. +// Return a non-nil error if recoveryObj was processed. +// Return nil if recoveryObj was not processed. type RecoveryHandler func(recoveryObj interface{}) error // recoveryMiddleware is wrapper for RecoveryHandler to create chained recovery handling. +// returns (recoveryMiddleware, nil) if recoveryObj was not processed and should be passed to the next middleware in chain. +// returns (nil, error) if recoveryObj was processed and middleware chain processing should be stopped. type recoveryMiddleware func(recoveryObj interface{}) (recoveryMiddleware, error) // processRecovery processes recoveryMiddleware chain for recovery() object. @@ -26,11 +30,7 @@ func processRecovery(recoveryObj interface{}, middleware recoveryMiddleware) err return err } - if next != nil { - return processRecovery(recoveryObj, next) - } - - return nil + return processRecovery(recoveryObj, next) } // newRecoveryMiddleware creates a RecoveryHandler middleware. From bd81674e84265c2d23dfe185f815c85e63811c0f Mon Sep 17 00:00:00 2001 From: Mikhail Kornilov Date: Sun, 17 May 2020 14:11:52 +0300 Subject: [PATCH 5/6] CHANGELOG.md: ADR-022 url fix --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2fdc32db2538..45b35ee74e2e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -234,7 +234,7 @@ functionality that requires an online connection. * (x/ibc) [\#5948](https://github.com/cosmos/cosmos-sdk/issues/5948) Add `InitGenesis` and `ExportGenesis` functions for `ibc` module. * (types) [\#6128](https://github.com/cosmos/cosmos-sdk/pull/6137) Add `String()` method to `GasMeter`. * (types) [\#6195](https://github.com/cosmos/cosmos-sdk/pull/6195) Add codespace to broadcast(sync/async) response. -* (baseapp) [\#6053](https://github.com/cosmos/cosmos-sdk/pull/6053) Customizable panic recovery handling added for `app.runTx()` method (as proposed in the [ADR 22](https://github.com/cosmos/tree/master/docs/architecture/adr-022-custom-panic-handling.md)). Adds ability for developers to register custom panic handlers extending standard ones. +* (baseapp) [\#6053](https://github.com/cosmos/cosmos-sdk/pull/6053) Customizable panic recovery handling added for `app.runTx()` method (as proposed in the [ADR 22](https://github.com/cosmos/cosmos-sdk/blob/master/docs/architecture/adr-022-custom-panic-handling.md)). Adds ability for developers to register custom panic handlers extending standard ones. ## [v0.38.3] - 2020-04-09 From f78fc026d236330103a4c0c487eef75ec69cf1bc Mon Sep 17 00:00:00 2001 From: Mikhail Kornilov Date: Fri, 29 May 2020 15:17:42 +0300 Subject: [PATCH 6/6] CoreDocs added for custom panic recovery middleware --- docs/core/README.md | 1 + docs/core/runtx_panics.md | 66 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+) create mode 100644 docs/core/runtx_panics.md diff --git a/docs/core/README.md b/docs/core/README.md index fab8a5361ba3..f8a024d59edc 100644 --- a/docs/core/README.md +++ b/docs/core/README.md @@ -16,5 +16,6 @@ This repository contains reference documentation on the core concepts of the Cos 6. [Encoding](./encoding.md) 7. [Events](./events.md) 8. [Object-Capabilities](./ocap.md) +9. [RunTx recovery middleware](./runtx_panics.md) After reading about the core concepts, head on to the [Building Modules documentation](../building-modules/README.md) to learn more about the process of building modules. \ No newline at end of file diff --git a/docs/core/runtx_panics.md b/docs/core/runtx_panics.md new file mode 100644 index 000000000000..66a7576ca626 --- /dev/null +++ b/docs/core/runtx_panics.md @@ -0,0 +1,66 @@ + + +# RunTx recovery middleware + +`BaseApp.runTx()` function handles Golang panics that might occur during transactions execution, for example, keeper has faced an invalid state and paniced. +Depending on the panic type different handler is used, for instance the default one prints an error log message. +Recovery middleware is used to add custom panic recovery for SDK application developers. + +More context could be found in the corresponding [ADR-022](../architecture/adr-022-custom-panic-handling.md). + +Implementation could be found in the [recovery.go](../../baseapp/recovery.go) file. + +## Interface + +```go +type RecoveryHandler func(recoveryObj interface{}) error +``` + +`recoveryObj` is a return value for `recover()` function from the `buildin` Golang package. + +**Contract:** +* RecoveryHandler returns `nil` if `recoveryObj` wasn't handled and should be passed to the next recovery middleware; +* RecoveryHandler returns a non-nil `error` if `recoveryObj` was handled; + +## Custom RecoveryHandler register + +``BaseApp.AddRunTxRecoveryHandler(handlers ...RecoveryHandler)`` + +BaseApp method adds recovery middleware to the default recovery chain. + +## Example + +Lets assume we want to emit the "Consensus failure" chain state if some particular error occurred. + +We have a module keeper that panics: +```go +func (k FooKeeper) Do(obj interface{}) { + if obj == nil { + // that shouldn't happen, we need to crash the app + err := sdkErrors.Wrap(fooTypes.InternalError, "obj is nil") + panic(err) + } +} +``` + +By default that panic would be recovered and an error message will be printed to log. To override that behaviour we should register a custom RecoveryHandler: +```go +// SDK application constructor +customHandler := func(recoveryObj interface{}) error { + err, ok := recoveryObj.(error) + if !ok { + return nil + } + + if fooTypes.InternalError.Is(err) { + panic(fmt.Errorf("FooKeeper did panic with error: %w", err)) + } + + return nil +} + +baseApp := baseapp.NewBaseApp(...) +baseApp.AddRunTxRecoveryHandler(customHandler) +```