Skip to content

Commit

Permalink
Use conn.PingContext() instead of Ping() to avoid trying to connect w…
Browse files Browse the repository at this point in the history
…ith context.Background() (#2823)

We've seen that Ping() might be blocking which defeats the purpose of
the loop. Let's use a dedicated timeout instead to properly time out the
ping.
  • Loading branch information
jhrozek authored Mar 27, 2024
1 parent 809687a commit 443a517
Showing 1 changed file with 16 additions and 8 deletions.
24 changes: 16 additions & 8 deletions internal/config/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,19 +56,27 @@ func (c *DatabaseConfig) GetDBConnection(ctx context.Context) (*sql.DB, string,

for i := 0; i < 8; i++ {
zerolog.Ctx(ctx).Info().Int("try number", i).Msg("Trying to connect to DB")
// Ensure we actually connected to the database, per Go docs
err = conn.Ping()
// we don't defer canceling the context because we want to cancel it as soon as we're done
// and we might overwrite the context in the loop
pingCtx, cancel := context.WithTimeout(ctx, 5*time.Second)

err = conn.PingContext(pingCtx)
if err != nil {
zerolog.Ctx(ctx).Warn().Err(err).Msgf("Unable to initialize connection to DB, retry %d", i)
time.Sleep(1 * time.Second)
continue
time.Sleep(1 * time.Second) // Consider exponential backoff here
} else {
zerolog.Ctx(ctx).Info().Msg("Connected to DB")
cancel()
return conn, uri, nil
}
zerolog.Ctx(ctx).Info().Msg("Connected to DB")
return conn, uri, err

cancel()
}

//nolint:gosec // Not much we can do about an error here.
conn.Close()
// Handle the closing of the connection outside the loop if all retries fail
if closeErr := conn.Close(); closeErr != nil {
zerolog.Ctx(ctx).Error().Err(closeErr).Msg("Failed to close DB connection")
}
return nil, "", err
}

Expand Down

0 comments on commit 443a517

Please sign in to comment.