🐛 [Bug]: Redis storage constructor panic on network error
Bug Description
Redis storage constructor panic on network or flushing errors.
Using panic in a public library (especially one intended for integration like fiber/storage/redis) breaks Go’s normal error-handling semantics and prevents callers from gracefully handling startup failures.
Why this is problematic:
- Panics in public libraries break normal error handling and make it impossible for applications to gracefully recover or report failures. Go libraries should return errors, not crash the process.
-
Implicit IO (PING) during object creation violates constructor expectations-
New()should only allocate and configure objects, not perform external network calls. This makes testing, offline environments, and lazy initialization harder.
How to Reproduce
Steps to reproduce the behavior:
- Call
New()with address that doesn't exist or not reachable. - The process will crash.
Expected Behavior
The constructor should:
- Return (*Storage, error) instead of panicking.
- Avoid performing an implicit PING during initialization.
- Let the caller decide whether to verify connectivity or defer it.
Storage package Version
v3.4.1
Code Snippet (optional)
// New creates a new Redis storage instance.
func New(config ...Config) *Storage {
// Set default config
cfg := configDefault(config...)
// ........
// Test connection
if err := db.Ping(context.Background()).Err(); err != nil {
panic(err)
}
// Empty collection if Clear is true
if cfg.Reset {
if err := db.FlushDB(context.Background()).Err(); err != nil {
panic(err)
}
}
// Create new store
return &Storage{
db: db,
}
}
Checklist:
- [x] I agree to follow Fiber's Code of Conduct.
- [x] I have checked for existing issues that describe my problem prior to opening this one.
- [x] I understand that improperly formatted bug reports may be closed without explanation.
@Av1shay The idea is that you create your storage connector being your app. If it cant talk to a database, it will panic.
You can also use NewFromConnection() that doesnt do the ping/panic().
We could add a config option to skip doing the ping/panic() as a solution in New(). Would that work?
@gaby Good point, but storage connectors can also be used for things like cache middleware, not necessarily as the app’s operational database. In such cases, you might not want to crash the entire app just because the cache is temporarily unavailable.
As a workaround, I’m currently using NewFromConnection() which works fine.
That said, I wouldn’t add a config flag just to toggle panic behavior. A more idiomatic Go pattern would be to provide two constructors:
- New() -> returns (*Storage, error)
- MustNew() -> panics on error
This follows the standard Go convention (e.g. regexp.Compile / regexp.MustCompile) while giving the caller full control over whether to panic or handle the error gracefully.
@Av1shay We can't change the public interface at this point, its a massive breaking change for anyone using the storage driver if we do that, thus why I suggested the config option.