storage icon indicating copy to clipboard operation
storage copied to clipboard

🐛 [Bug]: Redis storage constructor panic on network error

Open Av1shay opened this issue 3 months ago • 3 comments

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:

  1. Call New() with address that doesn't exist or not reachable.
  2. 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 avatar Oct 12 '25 12:10 Av1shay

@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 avatar Oct 12 '25 14:10 gaby

@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 avatar Oct 12 '25 15:10 Av1shay

@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.

gaby avatar Oct 12 '25 15:10 gaby