mgutz / dat

Go Postgres Data Access Toolkit
Other
612 stars 62 forks source link

MustPing with timeout #58

Open heynemann opened 7 years ago

heynemann commented 7 years ago

Hey @mgutz do you think you could make MustPing receive an optional parameter (with ...) to specify a timeout?

The issue I'm trying to avoid is that upon launching my servers, if there's no connectivity to a database, they'll just stay there trying indefinitely to connect, silently. The behavior I would expect of a well-behaved server is to fail with the proper error ("Could not connect to database at somethingsomething:3845" or something like that). That would trigger our error handling infrastructure and we'd be notified.

Do you think this is something you'd like to support? I can contribute with a PR if you'd like.

mgutz commented 7 years ago

The convention in Go is anything prefixed by "Must" will panic. If you'd like to submit a PR for a ShouldPing function, I can add that.

heynemann commented 7 years ago

I totally agree, the problem here is not that it won't panic (although I will send a PR with ShouldPing as I'd rather have that). The problem here is that it will never panic. It will just keep retrying time after time to ping.

I looked at the backoff lib and it has a MaxElapsedTime property that can be used for this purpose, like:

func shouldPing(db *sql.DB, timeout time.Duration) error {
    var err error
    b := backoff.NewExponentialBackOff()
    b.MaxElapsedTime = timeout
    ticker := backoff.NewTicker(b)

    // Ticks will continue to arrive when the previous operation is still running,
    // so operations that take a while to fail could run in quick succession.
    for range ticker.C {
        if err = db.Ping(); err != nil {
            continue
        }

        ticker.Stop()
        return nil
    }

    return fmt.Errorf("Could not ping database!")
}

I'm doing a PR now! :)

heynemann commented 7 years ago

Created PR #59.