🐛 bug: validate nil services early and during lifecycle#4050
🐛 bug: validate nil services early and during lifecycle#4050ReneWerner87 merged 2 commits intomainfrom
Conversation
WalkthroughAdds nil-service validation during app initialization and in service lifecycle routines. New validation causes New() to panic on invalid configured services; service start/shutdown/logging now return errors when encountering nil services, and startupMessage logs those errors. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @gaby, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a potential source of runtime panics and unclear errors by implementing robust validation for configured services. The changes ensure that Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4050 +/- ##
==========================================
- Coverage 91.21% 91.10% -0.12%
==========================================
Files 119 119
Lines 11121 11138 +17
==========================================
+ Hits 10144 10147 +3
- Misses 620 629 +9
- Partials 357 362 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Code Review
This pull request effectively addresses a potential panic caused by nil services in the configuration by adding validation at various stages of the application lifecycle. The changes are logical and the inclusion of new tests is commendable. My feedback focuses on a couple of opportunities to reduce code duplication, which would enhance the maintainability of the new validation logic.
There was a problem hiding this comment.
Pull request overview
This PR adds validation to prevent nil services from causing panics during the application lifecycle. The changes ensure that configuration mistakes are caught early during app initialization and that defensive checks exist throughout the service lifecycle.
Changes:
- Added early validation in
New()to check for nil services inConfig.Servicesand panic with a clear error message - Added defensive nil checks in
startServices,shutdownServices, andlogServicesto prevent runtime panics - Changed
logServicesto return an error instead of failing silently, with proper error handling in the startup message flow
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| app.go | Added validation call during app initialization to check for nil services |
| services.go | Added validation functions and defensive nil checks throughout service lifecycle methods |
| listen.go | Updated to handle error return from logServices with appropriate logging |
| services_test.go | Added test for nil service validation, made Test_LogServices parallel, and added error assertion |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@services.go`:
- Around line 33-44: The nil check currently in validateServicesSlice (called by
App.validateConfiguredServices) only compares the interface to nil and misses
typed-nil service implementations; add a small helper isNilService(srv Service)
bool that first checks srv == nil and otherwise uses
reflect.ValueOf(srv).Kind()==reflect.Ptr && reflect.ValueOf(srv).IsNil() (or
more generally checks Value.IsValid/IsNil for pointer/interface kinds) to detect
typed-nil underlying values, then replace the plain nil checks in
validateServicesSlice and the other spots where services are
validated/started/shutdown/logged (places that call srv.Start(), srv.State(),
srv.Terminate(), srv.String(), etc.) to call isNilService(srv) and return an
error or skip accordingly so typed-nil services no longer cause panics.
| func (app *App) validateConfiguredServices() error { | ||
| return validateServicesSlice(app.configured.Services) | ||
| } | ||
|
|
||
| func validateServicesSlice(services []Service) error { | ||
| for idx, srv := range services { | ||
| if srv == nil { | ||
| return fmt.Errorf("fiber: service at index %d is nil", idx) | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the services.go file
find . -name "services.go" -type f | head -20Repository: gofiber/fiber
Length of output: 70
🏁 Script executed:
# Get the actual content of services.go to verify the review's claims
head -250 services.go 2>/dev/null || find . -name "services.go" -exec head -250 {} \;Repository: gofiber/fiber
Length of output: 5289
🏁 Script executed:
# Check if there's a gofiber/utils package and what nil-checking helpers exist
find . -path "*/gofiber/utils*" -type d | head -5
rg "isNil|nilService" --type go | head -20Repository: gofiber/fiber
Length of output: 169
🏁 Script executed:
# Search for Service interface definition to understand the type
rg "type Service interface" -A 10 --type goRepository: gofiber/fiber
Length of output: 569
🏁 Script executed:
# Check the isNilableKind implementation and its usage
rg "isNilableKind" -A 5 -B 2 --type goRepository: gofiber/fiber
Length of output: 692
🏁 Script executed:
# Check imports in adapter.go to understand reflect usage pattern
grep -n "^import\|^[[:space:]]*\"" adapter.go | head -30Repository: gofiber/fiber
Length of output: 185
🏁 Script executed:
# Search for all reflect.ValueOf usage in the codebase
rg "reflect.ValueOf|reflect.Kind" --type go | head -20Repository: gofiber/fiber
Length of output: 1922
🏁 Script executed:
# Check if there are any examples of typed-nil services being created
rg "var.*Service.*nil|:=.*nil.*Service" --type goRepository: gofiber/fiber
Length of output: 39
🏁 Script executed:
# Check if isNilableKind is exported (capital letter) or internal
rg "^func isNilableKind|^func IsNilableKind" -B 2 --type goRepository: gofiber/fiber
Length of output: 137
🏁 Script executed:
# Check if there are any imports of functions from adapter.go
rg "from.*adapter|import.*adapter" --type goRepository: gofiber/fiber
Length of output: 39
🏁 Script executed:
# Look at how services are constructed and passed - any evidence of typed-nil services?
rg "configured.Services\s*=" -B 5 -A 5 --type go | head -40Repository: gofiber/fiber
Length of output: 39
🏁 Script executed:
# Check tests to see how services are tested
find . -name "*service*test*" -o -name "*test*service*" | head -10Repository: gofiber/fiber
Length of output: 75
🏁 Script executed:
# Examine the services test file
cat -n services_test.goRepository: gofiber/fiber
Length of output: 24114
🏁 Script executed:
# Check the config.go to see how services are configured
rg "Services.*\[\]Service" -B 3 -A 3 --type goRepository: gofiber/fiber
Length of output: 6578
🏁 Script executed:
# Check if there's any pattern of creating typed-nil services in the codebase
rg "var\s+\w+\s*Service\s*=" --type go -A 1 | head -30Repository: gofiber/fiber
Length of output: 39
🏁 Script executed:
# Check if the reflect package is already imported in services.go
head -20 services.goRepository: gofiber/fiber
Length of output: 500
Nil-interface check misses typed‑nil services (panic still possible).
A Service can be a typed nil pointer; in that case srv == nil is false, but srv.Start()/String/State/Terminate will still panic. That defeats the goal of preventing nil-service panics. Consider a shared helper that detects nil underlying values and reuse it in validation/start/shutdown/log.
🛠️ Suggested fix: detect typed‑nil services once and reuse
@@
-import (
- "context"
- "errors"
- "fmt"
- "io"
-
- "github.com/gofiber/utils/v2"
-)
+import (
+ "context"
+ "errors"
+ "fmt"
+ "io"
+ "reflect"
+
+ "github.com/gofiber/utils/v2"
+)
@@
-func validateServicesSlice(services []Service) error {
- for idx, srv := range services {
- if srv == nil {
- return fmt.Errorf("fiber: service at index %d is nil", idx)
- }
- }
- return nil
-}
+func validateServicesSlice(services []Service) error {
+ for idx, srv := range services {
+ if isNilService(srv) {
+ return fmt.Errorf("fiber: service at index %d is nil", idx)
+ }
+ }
+ return nil
+}
+
+func isNilService(srv Service) bool {
+ if srv == nil {
+ return true
+ }
+ v := reflect.ValueOf(srv)
+ switch v.Kind() {
+ case reflect.Pointer, reflect.Map, reflect.Slice, reflect.Func, reflect.Interface, reflect.Chan:
+ return v.IsNil()
+ default:
+ return false
+ }
+}
@@
- for idx, srv := range app.configured.Services {
- if srv == nil {
+ for idx, srv := range app.configured.Services {
+ if isNilService(srv) {
@@
- for key, srv := range app.state.Services() {
- if srv == nil {
+ for key, srv := range app.state.Services() {
+ if isNilService(srv) {
@@
- for key, srv := range app.state.Services() {
- if srv == nil {
+ for key, srv := range app.state.Services() {
+ if isNilService(srv) {Also applies to: 87-90, 122-125, 146-176
🤖 Prompt for AI Agents
In `@services.go` around lines 33 - 44, The nil check currently in
validateServicesSlice (called by App.validateConfiguredServices) only compares
the interface to nil and misses typed-nil service implementations; add a small
helper isNilService(srv Service) bool that first checks srv == nil and otherwise
uses reflect.ValueOf(srv).Kind()==reflect.Ptr && reflect.ValueOf(srv).IsNil()
(or more generally checks Value.IsValid/IsNil for pointer/interface kinds) to
detect typed-nil underlying values, then replace the plain nil checks in
validateServicesSlice and the other spots where services are
validated/started/shutdown/logged (places that call srv.Start(), srv.State(),
srv.Terminate(), srv.String(), etc.) to call isNilService(srv) and return an
error or skip accordingly so typed-nil services no longer cause panics.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Motivation
Serviceis nil by validating entries before callingString()during startup, shutdown, and logging.New(...)so apps fail-fast with a clear message instead of panicking inside lifecycle code.String()on nil services and that failures are surfaced to the startup message flow.Description
validateConfiguredServicesand helpervalidateServicesSliceto check for nil entries inConfig.Servicesand invoked it during app initialization inNew.startServices,shutdownServices, andlogServicesto return clear errors (including the service index or state key) before callingString()or other methods on services.logServicesto return anerrorand updated the startup message flow inlisten.goto log any error returned fromlogServicesinstead of ignoring it.services_test.goto run in parallel, assertlogServicesreturns no error, and addedTest_NewConfiguredServicesNilto verifyNewpanics with a clear error for nil service entries.