ADR 022 - Custom baseapp panic handling #6072

iTiky · 2020-04-24T14:44:07Z

/cc @alexanderbez

For contributor use:

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer

For admin use:

Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
Reviewers assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

codecov · 2020-04-24T14:55:53Z

Codecov Report

Merging #6072 into master will decrease coverage by 0.24%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #6072      +/-   ##
==========================================
- Coverage   54.64%   54.39%   -0.25%     
==========================================
  Files         426      430       +4     
  Lines       25890    26106     +216     
==========================================
+ Hits        14148    14201      +53     
- Misses      10763    10922     +159     
- Partials      979      983       +4

aaronc · 2020-04-24T15:00:02Z

I think we should allow more customizability of these sorts of things in general. The approach in #6053 handles one such case and I don't think we should block it. But I do think we should reconsider a composable middleware approach to BaseApp, rather than handling each edge case one by one.

docs/architecture/adr-022-custom-panic-handling.md

alexanderbez

Thanks @iTiky! Left some minor feedback.

docs/architecture/adr-022-custom-panic-handling.md

fedekunze

Well written ADR. Left a few suggestions. Thanks @iTiky! 👍

docs/architecture/adr-022-custom-panic-handling.md

alexanderbez

I took another pass through over this ADR and things started to make more sense to me. I think certain sections can be either clarified or omitted entirely. But overall, the premise looks solid and I see no reason why we can't support this.

One thing I would like note (I've mentioned this in a comment), is that we should guarantee or assert that the last handler in the decorator/middleware stack is guaranteed to return an error -- a fail-safe/terminator.

docs/architecture/adr-022-custom-panic-handling.md

Co-Authored-By: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>

alexanderbez

conceptACK as I'm generally in favor of (opinionated) flexibility where the concept is clean and modular and extensible. I would like to see an ACK from @aaronc and @cwgoes potentially before proceeding with the implementation.

cwgoes

Concept ACK but I would like to see more clearly explained examples

cwgoes · 2020-04-27T14:37:15Z

docs/architecture/adr-022-custom-panic-handling.md

+might be handled in a "standard" way (middleware) alongside the others.
+
+We propose middleware-solution, which could help developers implement the following cases:
+* add external logging (let's say sending reports to external services like [Sentry](https://sentry.io));


I do not understand how this would work. Would every node make an HTTP call to Sentry?

Those are examples of how developers can use those middlewares. They can send panic logs to Sentry if they need to or do other struf needed.

I still don't understand how Sentry would be used in a replicated state machine.

Those are just examples of what developers of Cosmos SDK based project could use those handlers for. May be they want to get notifications on every panic() event (Sentry), that doesn't collide with state machine ideology.

The issue is that (in a production blockchain) the state machine is replicated, so many copies of every error would be sent to Sentry, and you would need to be careful to ensure that making HTTP requests doesn't introduce non-determinism. It's an alright example, but these points need to be clarified.

Actually Sentry can aggregate multiple similar event into one issue with some meta and statistics. As for non-deterministic behavior: developers can do that in so many ways including the proposed middlewares =)

That's nice. The determinism bit is more important. I think it would be sufficient if we add a warning that middlewares must be deterministic, or consensus safety will be lost.

docs/architecture/adr-022-custom-panic-handling.md

aaronc · 2020-04-27T21:23:27Z

docs/architecture/adr-022-custom-panic-handling.md

+    // ...
+    defer func() {
+        if r := recover(); r != nil {
+            recoveryMW := newOutOfGasRecoveryMiddleware(gasWanted, ctx, app.runTxRecoveryMiddleware)


Why are we hard-coding newOutOfGasRecoveryMiddleware into BaseApp? I thought the idea was to defer to app.runTxRecoveryMiddleware?

That is because OutOfGas error handling needs some extra context and can't be statically created.

aaronc

Generally I'm in favor of well-reasoned customizability. As I noted before, I would like us to think of a more general framework for BaseApp being composed of middleware in the future.

But, this seems like a good starting point with a concrete use case and I'd like to see it move forward.

Co-Authored-By: Federico Kunze <31522760+fedekunze@users.noreply.github.com>

cwgoes

ACK

AdityaSripal · 2020-04-28T19:22:50Z

docs/architecture/adr-022-custom-panic-handling.md

+func newOutOfGasRecoveryMiddleware(gasWanted uint64, ctx sdk.Context, next recoveryMiddleware) recoveryMiddleware {
+    handler := func(recoveryObj interface{}) error {
+        err, ok := recoveryObj.(sdk.ErrorOutOfGas)
+        if !ok { return nil }


Should middleware decorators simply pass recoveryObj along to the next middleware if it isn't an error they handle instead of returning?

Actually they do. The example above creates a handler and passes it to func newRecoveryMiddleware which creates a middleware. That way developer only needs to create a RecoveryHandler (which only return an error / nil). The rest is done with helper functions automatically.

iTiky · 2020-04-29T15:57:04Z

@alexanderbez ADR's PR is approved, what are the next steps? I can update the implementation PR (add link to the ADR, make some minor improvements proposed in the discussion).

alexanderbez · 2020-04-29T16:00:09Z

@iTiky, yes let's update this PR so we can merge it and then we can proceed with reviewing the implementation 👍

Thank you for all the hard work :)

aaronc · 2020-05-05T17:41:29Z

Looks like the main thing needed to move this forward is merging master into this branch. @iTiky can you do that and then we'll merge.

iTiky · 2020-05-13T10:02:29Z

@alexanderbez Can we merge this ADR as it breaks the markdown analyser for PR (invalid link to the ADR file).

alexanderbez · 2020-05-13T13:31:26Z

Yes, we just need to rebase 👍

alexanderbez · 2020-05-13T14:14:41Z

Something did not go right in the rebase @iTiky

iTiky · 2020-05-13T14:39:32Z

@alexanderbez Yes, you're right. Can I just close this PR and make a new cleaner one preserving all changes? I'm not sure I can do the force push right.

alexanderbez · 2020-05-13T14:48:06Z

I don't think that is necessary. Revert back to 35fc451, checkout and pull latest master, and finally merge master into this branch.

iTiky · 2020-05-13T15:03:04Z

@alexanderbez Thanks! I did the rollback and merged the latest master.

ADR-22 propose added

3f3fd64

iTiky requested review from alessio, alexanderbez, fedekunze, hschoenburg and nylira as code owners April 24, 2020 14:44

alexanderbez added T: ADR An issue or PR relating to an architectural decision record R4R labels Apr 24, 2020

alexanderbez requested review from aaronc, AdityaSripal, cwgoes and jgimeno April 24, 2020 15:03

alexanderbez added the C:baseapp label Apr 24, 2020

aaronc reviewed Apr 24, 2020

View reviewed changes

docs/architecture/adr-022-custom-panic-handling.md Outdated Show resolved Hide resolved

alexanderbez suggested changes Apr 24, 2020

View reviewed changes

Mikhail Kornilov added 2 commits April 24, 2020 19:42

ADR-022: decision implementation details added

1a8c5ae

ADR-022: feedback fix

d71b3a0

iTiky requested a review from alexanderbez April 24, 2020 16:59

fedekunze reviewed Apr 24, 2020

View reviewed changes

ADR-022: feedback fix

34f1609

alexanderbez reviewed Apr 24, 2020

View reviewed changes

ADR-022: feedback fix

c7de4d5

alexanderbez reviewed Apr 25, 2020

View reviewed changes

Mikhail Kornilov and others added 2 commits April 27, 2020 13:31

Update docs/architecture/adr-022-custom-panic-handling.md

07f895b

Co-Authored-By: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>

ADR-022: feedback based improvements

931bb85

alexanderbez approved these changes Apr 27, 2020

View reviewed changes

cwgoes suggested changes Apr 27, 2020

View reviewed changes

fedekunze reviewed Apr 27, 2020

View reviewed changes

aaronc reviewed Apr 27, 2020

View reviewed changes

aaronc approved these changes Apr 27, 2020

View reviewed changes

Mikhail Kornilov and others added 7 commits April 28, 2020 12:15

Update docs/architecture/adr-022-custom-panic-handling.md

54830e1

Co-Authored-By: Federico Kunze <31522760+fedekunze@users.noreply.github.com>

ADR-022: feedback based improvements

e0edaa8

Merge remote-tracking branch 'origin/adr-022' into adr-022

18468eb

Update docs/architecture/adr-022-custom-panic-handling.md

5e470bd

Co-Authored-By: Federico Kunze <31522760+fedekunze@users.noreply.github.com>

Update docs/architecture/adr-022-custom-panic-handling.md

5c511ff

Co-Authored-By: Federico Kunze <31522760+fedekunze@users.noreply.github.com>

ADR-022: feedback based improvements

bf1cbdf

Merge branch 'master' into adr-022

fb594bf

iTiky requested a review from cwgoes April 28, 2020 15:41

cwgoes approved these changes Apr 28, 2020

View reviewed changes

AdityaSripal reviewed Apr 28, 2020

View reviewed changes

aaronc added the A:automerge Automatically merge PR once all prerequisites pass. label May 5, 2020

Merge branch 'master' into adr-022

99b6ccd

iTiky requested a review from clevinson as a code owner May 5, 2020 18:10

fedekunze approved these changes May 5, 2020

View reviewed changes

iTiky mentioned this pull request May 13, 2020

Baseapp recovery middleware #6053

Merged

11 tasks

Merge branch 'master' into adr-022

4379e23

iTiky force-pushed the adr-022 branch from 74274f8 to 4379e23 Compare May 13, 2020 15:00

alexanderbez merged commit b1b46db into cosmos:master May 13, 2020

iTiky deleted the adr-022 branch August 26, 2020 15:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ADR 022 - Custom baseapp panic handling #6072

ADR 022 - Custom baseapp panic handling #6072

iTiky commented Apr 24, 2020 •

edited by alexanderbez

Loading

codecov bot commented Apr 24, 2020 •

edited

Loading

aaronc commented Apr 24, 2020

alexanderbez left a comment

fedekunze left a comment

alexanderbez left a comment

alexanderbez left a comment

cwgoes left a comment

cwgoes Apr 27, 2020

iTiky Apr 28, 2020

cwgoes Apr 28, 2020

iTiky Apr 28, 2020

cwgoes Apr 28, 2020

iTiky Apr 28, 2020

cwgoes Apr 28, 2020

aaronc Apr 27, 2020

iTiky Apr 28, 2020

aaronc left a comment

cwgoes left a comment

AdityaSripal Apr 28, 2020

iTiky Apr 28, 2020

iTiky commented Apr 29, 2020

alexanderbez commented Apr 29, 2020

aaronc commented May 5, 2020

iTiky commented May 13, 2020

alexanderbez commented May 13, 2020

alexanderbez commented May 13, 2020

iTiky commented May 13, 2020

alexanderbez commented May 13, 2020 •

edited

Loading

iTiky commented May 13, 2020

ADR 022 - Custom baseapp panic handling #6072

ADR 022 - Custom baseapp panic handling #6072

Conversation

iTiky commented Apr 24, 2020 • edited by alexanderbez Loading

codecov bot commented Apr 24, 2020 • edited Loading

Codecov Report

aaronc commented Apr 24, 2020

alexanderbez left a comment

Choose a reason for hiding this comment

fedekunze left a comment

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

cwgoes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronc left a comment

Choose a reason for hiding this comment

cwgoes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iTiky commented Apr 29, 2020

alexanderbez commented Apr 29, 2020

aaronc commented May 5, 2020

iTiky commented May 13, 2020

alexanderbez commented May 13, 2020

alexanderbez commented May 13, 2020

iTiky commented May 13, 2020

alexanderbez commented May 13, 2020 • edited Loading

iTiky commented May 13, 2020

iTiky commented Apr 24, 2020 •

edited by alexanderbez

Loading

codecov bot commented Apr 24, 2020 •

edited

Loading

alexanderbez commented May 13, 2020 •

edited

Loading