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.

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

do not swallow panic when rolling back inner panic to prevent connection leak #797

Merged
merged 3 commits into from
Nov 19, 2022

Conversation

sio4
Copy link
Member

@sio4 sio4 commented Nov 19, 2022

This PR makes Transaction() not swallow inner panic when rolling back inner panic to prevent connection leak. (followup for #748, #775)

fixes #793

With this configuration,

app.GET("/panic", func(c buffalo.Context) error {
        panic("panic message")
})

Previously we got a generic error but no information about the actual panic:

[POP] 2022/11/19 12:27:13 sql - BEGIN Transaction --- (conn=tx-2490861061155653040, tx=2490861061155653040)
[POP] 2022/11/19 12:27:13 sql - ROLLBACK Transaction (inner function panic) --- (conn=tx-2490861061155653040, tx=2490861061155653040)
ERRO[2022-11-19T12:27:13+09:00] transaction was rolled back due to inner panic conn=tx-2490861061155653040 db=0s params="{}" request_id=f3afa8520e74468e3126-605aa30f89aa98d921a6 status=500 tx=2490861061155653040
INFO[2022-11-19T12:27:13+09:00] /panic/ conn=tx-2490861061155653040 db=0s duration=4.668962ms human_size="33 kB" method=GET params="{}" path=/panic/ request_id=f3afa8520e74468e3126-605aa30f89aa98d921a6 size=33278 status=500 tx=2490861061155653040

now, the panic message will be handled by Buffalo's panic handler (or the application using Pop just gets the panic)

[POP] 2022/11/19 12:29:19 sql - BEGIN Transaction --- (conn=tx-6882700540045613063, tx=6882700540045613063)
[POP] 2022/11/19 12:29:19 sql - ROLLBACK Transaction (inner function panic) --- (conn=tx-6882700540045613063, tx=6882700540045613063)
ERRO[2022-11-19T12:29:19+09:00] panic message conn=tx-6882700540045613063 db=0s params="{}" request_id=d03cd4eaaeb706091e72-bd2ebc6009256e976b7e status=500 tx=6882700540045613063
INFO[2022-11-19T12:29:19+09:00] /panic/ conn=tx-6882700540045613063 db=0s duration=4.387724ms human_size="33 kB" method=GET params="{}" path=/panic/ request_id=d03cd4eaaeb706091e72-bd2ebc6009256e976b7e size=33245 status=500 tx=6882700540045613063

Developers can use https://github.com/gobuffalo/events/ for more details:

func init() {
    events.Listen(func(e events.Event) {
        switch e.Kind {
        case events.ErrPanic:
            fmt.Println("panic error:", e.Error)
            if pl, ok := e.Payload["data"].(map[string]interface{}); ok {
                fmt.Println("panic stacktrace:", pl["stacktrace"])
            }
        }
    }) 
}

(Payload has a bug having additional "data", and will be fixed soon)

The above code will print the following:

panic error: panic message
panic stacktrace: goroutine 66 [running]:
runtime/debug.Stack()
        /opt/google/go/src/runtime/debug/stack.go:24 +0x65
github.com/gobuffalo/buffalo.(*App).PanicHandler.func1.1()
        /home/sio4/go/github.com/gobuffalo/buffalo/errors.go:98 +0x1e5
panic({0xbc8760, 0x1345270})
        /opt/google/go/src/runtime/panic.go:1038 +0x215
github.com/gobuffalo/pop/v6.(*Connection).Transaction.func1.1()
        /home/sio4/go/github.com/gobuffalo/pop/connection.go:173 +0xc5
panic({0xbc8760, 0x1345270})
        /opt/google/go/src/runtime/panic.go:1038 +0x215
coco/actions.App.func1.6({0xc000228001, 0xc000322f60})
        /home/sio4/git/bt/coco/actions/app.go:94 +0x27

@sio4 sio4 added the bug Something isn't working label Nov 19, 2022
@sio4 sio4 added this to the v6.1.0 milestone Nov 19, 2022
@sio4 sio4 requested a review from a team November 19, 2022 04:23
@sio4 sio4 self-assigned this Nov 19, 2022
@sio4 sio4 enabled auto-merge November 19, 2022 06:23
logger.go Show resolved Hide resolved
connection.go Show resolved Hide resolved
@sio4 sio4 merged commit 17f09c0 into main Nov 19, 2022
@sio4 sio4 deleted the prevent-conn-leak-but-do-not-swallow-panic branch November 19, 2022 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: transaction panic recovery swallows panic message
2 participants