jackc / pgx

PostgreSQL driver and toolkit for Go
MIT License
10.86k stars 846 forks source link

Option to wrap goroutines with a custom handler #2064

Closed madisonchamberlain closed 4 months ago

madisonchamberlain commented 5 months ago

Is your feature request related to a problem? Please describe. When there is some sort of panic which happens in a goroutine kicked off in the pgx library this will take down the binary which causes a series of undesirable behavior. Rather than let it take down the binary I would like to have the ability to recover the panic and log it so I can get more information about what caused the panic so it can be fixed. Describe the solution you'd like A clear and concise description of what you want to happen.

Describe alternatives you've considered I would be happy to propose a pr to upstream if this is something you would consider, but here is what I am thinking:

goroutineWrapper.go

import "context"

// GoroutineWrapperFunc is used to wrap goroutines. This is useful if the caller wants
// to recover panics, rather than letting panics cause a system crash. A suggestion would be to
// use use the recover functionality, and log the panic as is most useful to you
type GoroutineWrapperFunc func(ctx context.Context, f func())

// The default GoroutineWrapperFunc; this does nothing. With this default wrapper
// panics will take down binary as expected
var noopGoroutineWrapper = func(_ context.Context, f func()) {
    f()
}

// GoroutineWrapper is used to hold the GoroutineWrapperFunc set by the client, or to
// store the default goroutine wrapper which does nothing
var GoroutineWrapper GoroutineWrapperFunc = noopGoroutineWrapper

And then anytime the goroutine is called, instead of it looking like this

go func() {
        do something
}()

it would look like this

go GoroutineWrapper(
    ctx,
    func() {
        do something
    },
)

Additional context Let me know if you would be open to me putting up a pr for this

jackc commented 5 months ago

Do you have any examples of this happening? pgx's use goroutines is fairly limited. Any panic that does occur in a pgx spawned goroutine is a bug. I'm concerned that arbitrarily rescuing panics could leave the system in an undefined state. IMO, better to let the application crash. Then fix the bug.

madisonchamberlain commented 5 months ago

Yeah! This is the what I saw when it was panicking

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x2672365]

goroutine 97522 [running]:
github.com/sigmacomputing/mono-go/multiplex/db.getSshConfigs.func1({0x36fc678?, 0xc02d2f38c0?}, {0x30b2203?, 0x36f54d0?}, {0xc02d2e0fc0?, 0x0?})
    /go/src/github.com/sigmacomputing/mono-go/multiplex/db/pgx.go:3836 +0x45
github.com/jackc/pgconn.connect({0x36fc678, 0xc02d2f38c0}, 0xc02d39f440, 0xc02d2b55c0, 0x0)
    /go/pkg/mod/github.com/jackc/pgconn@v1.14.3/pgconn.go:270 +0x1a3
github.com/jackc/pgconn.ConnectConfig({0x36fe7b8, 0xc02d3a28c0}, 0xc02d39f440)
    /go/pkg/mod/github.com/jackc/pgconn@v1.14.3/pgconn.go:171 +0x515
github.com/jackc/pgx/v4.connect({0x36fe7b8, 0xc02d3a28c0}, 0xc02d39f200)
    /go/pkg/mod/github.com/jackc/pgx/v4@v4.18.3/conn.go:222 +0x365
github.com/jackc/pgx/v4.ConnectConfig(...)
    /go/pkg/mod/github.com/jackc/pgx/v4@v4.18.3/conn.go:113
github.com/jackc/pgx/v4/pgxpool.ConnectConfig.func1({0x36fc608?, 0xc02632b860?})
    /go/pkg/mod/github.com/jackc/pgx/v4@v4.18.3/pgxpool/pool.go:232 +0x16a
github.com/jackc/puddle.(*Pool).constructResourceValue(...)
    /go/pkg/mod/github.com/jackc/puddle@v1.3.0/pool.go:558
github.com/jackc/puddle.(*Pool).CreateResource(0xc02d158200, {0x36fc608, 0xc02632b860})
    /go/pkg/mod/github.com/jackc/puddle@v1.3.0/pool.go:479 +0x9b
github.com/jackc/pgx/v4/pgxpool.(*Pool).createIdleResources.func1()
    /go/pkg/mod/github.com/jackc/pgx/v4@v4.18.3/pgxpool/pool.go:512 +0x3c
created by github.com/jackc/pgx/v4/pgxpool.(*Pool).createIdleResources
    /go/pkg/mod/github.com/jackc/pgx/v4@v4.18.3/pgxpool/pool.go:510 +0xa5

I think somehow the puddle is nil but I couldn't find anywhere in the code that could be setting the puddle to nil

jackc commented 4 months ago

That's not a pgx panic. It is the DialFunc that you have configured panicking.

madisonchamberlain commented 4 months ago

Oh my bad thank you!