🔥 feat: Add Support for service dependencies #3434
Conversation
|
Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
WalkthroughService lifecycle management has been integrated into the application. A Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant Service1
participant Service2
User->>App: New(Config{Services: [Service1, Service2]})
App->>Service1: Start(context.Background)
App->>Service2: Start(context.Background)
App-->>User: App instance returned
Note over App: On shutdown
App->>Service1: Terminate(context.Background)
App->>Service2: Terminate(context.Background)
Suggested reviewers
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (6)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3434 +/- ##
==========================================
+ Coverage 83.85% 83.99% +0.14%
==========================================
Files 119 120 +1
Lines 11897 11977 +80
==========================================
+ Hits 9976 10060 +84
+ Misses 1490 1487 -3
+ Partials 431 430 -1
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.
Actionable comments posted: 9
🧹 Nitpick comments (4)
devtime_dependencies_test.go (3)
135-152: Context cancellation test could be more robust.The current test uses an extremely short timeout (1 nanosecond) which might be flaky. Additionally, we should verify if the dependency was actually started before context cancellation.
Consider using a more realistic timeout and checking the dependency's state:
- ctx, cancel := context.WithTimeout(context.Background(), 1*time.Nanosecond) + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Millisecond) defer cancel() // Start dependencies with canceled context err := app.startDevTimeDependencies(ctx) require.ErrorIs(t, err, context.DeadlineExceeded) + require.False(t, slowDep.started, "Dependency should not have been started due to context cancellation")Also, "cancelled" is the British English spelling. The standard Go library uses "canceled" (American English).
🧰 Tools
🪛 GitHub Check: lint
[failure] 149-149:
cancelledis a misspelling ofcanceled(misspell)
[failure] 145-145:
cancelledis a misspelling ofcanceled(misspell)
154-175: Verify dependency state after termination attempt.Similar to the start test, this test should verify that the dependency's terminated flag is set to false when context is canceled.
// Shutdown dependencies with canceled context err = app.shutdownDevTimeDependencies(ctx) require.ErrorIs(t, err, context.DeadlineExceeded) + require.False(t, slowDep.terminated, "Dependency should not have been terminated due to context cancellation")🧰 Tools
🪛 GitHub Check: lint
[failure] 164-164:
cancelledis a misspelling ofcanceled(misspell)
177-198: Consider verifying dependency states after errors.The test confirms that errors are properly aggregated, but doesn't verify the state of each dependency after the operation.
Add verification of dependency states:
// Test start errors err := app.startDevTimeDependencies(context.Background()) require.Error(t, err) // Verify error message contains both error messages errMsg := err.Error() require.Contains(t, errMsg, "start error 1") require.Contains(t, errMsg, "start error 2") + // Verify all dependencies were attempted to be started + for _, dep := range deps { + mockDep := dep.(*mockDependency) + require.True(t, mockDep.started, "Dependency should have been started despite errors") + }devtime_dependencies.go (1)
27-75: Consider using error wrapping consistentlyThe error handling pattern is good, but notice the inconsistency between wrapping errors in
startDevTimeDependenciesversusshutdownDevTimeDependencies.For consistency, consider using the same format for both functions. For example:
- errs = append(errs, fmt.Errorf("context canceled: %w", err)) + errs = append(errs, fmt.Errorf("dependency %s terminate: %w", "unknown", err))This maintains consistent error message formatting throughout the codebase.
🧰 Tools
🪛 GitHub Check: lint
[failure] 60-60:
cancelledis a misspelling ofcanceled(misspell)
[failure] 59-59:
cancelledis a misspelling ofcanceled(misspell)
[failure] 36-36:
cancelledis a misspelling ofcanceled(misspell)
[failure] 34-34:
cancelledis a misspelling ofcanceled(misspell)🪛 GitHub Actions: golangci-lint
[error] 34-34:
cancelledis a misspelling ofcanceled(misspell)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app.go(2 hunks)devtime_dependencies.go(1 hunks)devtime_dependencies_test.go(1 hunks)docs/api/constants.md(1 hunks)docs/api/devtime_dependencies.md(1 hunks)docs/whats_new.md(2 hunks)listen.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
app.go (3)
devtime_dependencies.go (1)
DevTimeDependency(10-20)log/fiberlog.go (1)
Warnf(54-56)hooks.go (1)
Hooks(21-35)
🪛 markdownlint-cli2 (0.17.2)
docs/whats_new.md
810-810: Hard tabs
Column: 1
(MD010, no-hard-tabs)
816-816: Hard tabs
Column: 1
(MD010, no-hard-tabs)
817-817: Hard tabs
Column: 1
(MD010, no-hard-tabs)
823-823: Hard tabs
Column: 1
(MD010, no-hard-tabs)
828-828: Hard tabs
Column: 1
(MD010, no-hard-tabs)
829-829: Hard tabs
Column: 1
(MD010, no-hard-tabs)
835-835: Hard tabs
Column: 1
(MD010, no-hard-tabs)
836-836: Hard tabs
Column: 1
(MD010, no-hard-tabs)
838-838: Hard tabs
Column: 1
(MD010, no-hard-tabs)
docs/api/devtime_dependencies.md
70-70: Hard tabs
Column: 1
(MD010, no-hard-tabs)
71-71: Hard tabs
Column: 1
(MD010, no-hard-tabs)
73-73: Hard tabs
Column: 1
(MD010, no-hard-tabs)
74-74: Hard tabs
Column: 1
(MD010, no-hard-tabs)
75-75: Hard tabs
Column: 1
(MD010, no-hard-tabs)
79-79: Hard tabs
Column: 1
(MD010, no-hard-tabs)
84-84: Hard tabs
Column: 1
(MD010, no-hard-tabs)
85-85: Hard tabs
Column: 1
(MD010, no-hard-tabs)
86-86: Hard tabs
Column: 1
(MD010, no-hard-tabs)
87-87: Hard tabs
Column: 1
(MD010, no-hard-tabs)
88-88: Hard tabs
Column: 1
(MD010, no-hard-tabs)
90-90: Hard tabs
Column: 1
(MD010, no-hard-tabs)
91-91: Hard tabs
Column: 1
(MD010, no-hard-tabs)
97-97: Hard tabs
Column: 1
(MD010, no-hard-tabs)
102-102: Hard tabs
Column: 1
(MD010, no-hard-tabs)
103-103: Hard tabs
Column: 1
(MD010, no-hard-tabs)
107-107: Hard tabs
Column: 1
(MD010, no-hard-tabs)
109-109: Hard tabs
Column: 1
(MD010, no-hard-tabs)
110-110: Hard tabs
Column: 1
(MD010, no-hard-tabs)
111-111: Hard tabs
Column: 1
(MD010, no-hard-tabs)
113-113: Hard tabs
Column: 1
(MD010, no-hard-tabs)
115-115: Hard tabs
Column: 1
(MD010, no-hard-tabs)
117-117: Hard tabs
Column: 1
(MD010, no-hard-tabs)
118-118: Hard tabs
Column: 1
(MD010, no-hard-tabs)
119-119: Hard tabs
Column: 1
(MD010, no-hard-tabs)
120-120: Hard tabs
Column: 1
(MD010, no-hard-tabs)
121-121: Hard tabs
Column: 1
(MD010, no-hard-tabs)
122-122: Hard tabs
Column: 1
(MD010, no-hard-tabs)
124-124: Hard tabs
Column: 1
(MD010, no-hard-tabs)
125-125: Hard tabs
Column: 1
(MD010, no-hard-tabs)
126-126: Hard tabs
Column: 1
(MD010, no-hard-tabs)
127-127: Hard tabs
Column: 1
(MD010, no-hard-tabs)
128-128: Hard tabs
Column: 1
(MD010, no-hard-tabs)
129-129: Hard tabs
Column: 1
(MD010, no-hard-tabs)
131-131: Hard tabs
Column: 1
(MD010, no-hard-tabs)
132-132: Hard tabs
Column: 1
(MD010, no-hard-tabs)
134-134: Hard tabs
Column: 1
(MD010, no-hard-tabs)
135-135: Hard tabs
Column: 1
(MD010, no-hard-tabs)
136-136: Hard tabs
Column: 1
(MD010, no-hard-tabs)
137-137: Hard tabs
Column: 1
(MD010, no-hard-tabs)
139-139: Hard tabs
Column: 1
(MD010, no-hard-tabs)
152-152: Hard tabs
Column: 1
(MD010, no-hard-tabs)
153-153: Hard tabs
Column: 1
(MD010, no-hard-tabs)
154-154: Hard tabs
Column: 1
(MD010, no-hard-tabs)
156-156: Hard tabs
Column: 1
(MD010, no-hard-tabs)
157-157: Hard tabs
Column: 1
(MD010, no-hard-tabs)
158-158: Hard tabs
Column: 1
(MD010, no-hard-tabs)
162-162: Hard tabs
Column: 1
(MD010, no-hard-tabs)
163-163: Hard tabs
Column: 1
(MD010, no-hard-tabs)
164-164: Hard tabs
Column: 1
(MD010, no-hard-tabs)
168-168: Hard tabs
Column: 1
(MD010, no-hard-tabs)
173-173: Hard tabs
Column: 1
(MD010, no-hard-tabs)
174-174: Hard tabs
Column: 1
(MD010, no-hard-tabs)
175-175: Hard tabs
Column: 1
(MD010, no-hard-tabs)
176-176: Hard tabs
Column: 1
(MD010, no-hard-tabs)
177-177: Hard tabs
Column: 1
(MD010, no-hard-tabs)
179-179: Hard tabs
Column: 1
(MD010, no-hard-tabs)
180-180: Hard tabs
Column: 1
(MD010, no-hard-tabs)
186-186: Hard tabs
Column: 1
(MD010, no-hard-tabs)
191-191: Hard tabs
Column: 1
(MD010, no-hard-tabs)
192-192: Hard tabs
Column: 1
(MD010, no-hard-tabs)
196-196: Hard tabs
Column: 1
(MD010, no-hard-tabs)
198-198: Hard tabs
Column: 1
(MD010, no-hard-tabs)
199-199: Hard tabs
Column: 1
(MD010, no-hard-tabs)
200-200: Hard tabs
Column: 1
(MD010, no-hard-tabs)
202-202: Hard tabs
Column: 1
(MD010, no-hard-tabs)
204-204: Hard tabs
Column: 1
(MD010, no-hard-tabs)
206-206: Hard tabs
Column: 1
(MD010, no-hard-tabs)
207-207: Hard tabs
Column: 1
(MD010, no-hard-tabs)
208-208: Hard tabs
Column: 1
(MD010, no-hard-tabs)
209-209: Hard tabs
Column: 1
(MD010, no-hard-tabs)
210-210: Hard tabs
Column: 1
(MD010, no-hard-tabs)
211-211: Hard tabs
Column: 1
(MD010, no-hard-tabs)
213-213: Hard tabs
Column: 1
(MD010, no-hard-tabs)
214-214: Hard tabs
Column: 1
(MD010, no-hard-tabs)
215-215: Hard tabs
Column: 1
(MD010, no-hard-tabs)
216-216: Hard tabs
Column: 1
(MD010, no-hard-tabs)
217-217: Hard tabs
Column: 1
(MD010, no-hard-tabs)
218-218: Hard tabs
Column: 1
(MD010, no-hard-tabs)
220-220: Hard tabs
Column: 1
(MD010, no-hard-tabs)
221-221: Hard tabs
Column: 1
(MD010, no-hard-tabs)
223-223: Hard tabs
Column: 1
(MD010, no-hard-tabs)
224-224: Hard tabs
Column: 1
(MD010, no-hard-tabs)
225-225: Hard tabs
Column: 1
(MD010, no-hard-tabs)
226-226: Hard tabs
Column: 1
(MD010, no-hard-tabs)
228-228: Hard tabs
Column: 1
(MD010, no-hard-tabs)
229-229: Hard tabs
Column: 1
(MD010, no-hard-tabs)
231-231: Hard tabs
Column: 1
(MD010, no-hard-tabs)
232-232: Hard tabs
Column: 1
(MD010, no-hard-tabs)
233-233: Hard tabs
Column: 1
(MD010, no-hard-tabs)
234-234: Hard tabs
Column: 1
(MD010, no-hard-tabs)
235-235: Hard tabs
Column: 1
(MD010, no-hard-tabs)
237-237: Hard tabs
Column: 1
(MD010, no-hard-tabs)
238-238: Hard tabs
Column: 1
(MD010, no-hard-tabs)
239-239: Hard tabs
Column: 1
(MD010, no-hard-tabs)
240-240: Hard tabs
Column: 1
(MD010, no-hard-tabs)
241-241: Hard tabs
Column: 1
(MD010, no-hard-tabs)
243-243: Hard tabs
Column: 1
(MD010, no-hard-tabs)
244-244: Hard tabs
Column: 1
(MD010, no-hard-tabs)
245-245: Hard tabs
Column: 1
(MD010, no-hard-tabs)
246-246: Hard tabs
Column: 1
(MD010, no-hard-tabs)
247-247: Hard tabs
Column: 1
(MD010, no-hard-tabs)
248-248: Hard tabs
Column: 1
(MD010, no-hard-tabs)
250-250: Hard tabs
Column: 1
(MD010, no-hard-tabs)
251-251: Hard tabs
Column: 1
(MD010, no-hard-tabs)
253-253: Hard tabs
Column: 1
(MD010, no-hard-tabs)
254-254: Hard tabs
Column: 1
(MD010, no-hard-tabs)
256-256: Hard tabs
Column: 1
(MD010, no-hard-tabs)
257-257: Hard tabs
Column: 1
(MD010, no-hard-tabs)
258-258: Hard tabs
Column: 1
(MD010, no-hard-tabs)
259-259: Hard tabs
Column: 1
(MD010, no-hard-tabs)
261-261: Hard tabs
Column: 1
(MD010, no-hard-tabs)
262-262: Hard tabs
Column: 1
(MD010, no-hard-tabs)
263-263: Hard tabs
Column: 1
(MD010, no-hard-tabs)
264-264: Hard tabs
Column: 1
(MD010, no-hard-tabs)
265-265: Hard tabs
Column: 1
(MD010, no-hard-tabs)
266-266: Hard tabs
Column: 1
(MD010, no-hard-tabs)
267-267: Hard tabs
Column: 1
(MD010, no-hard-tabs)
269-269: Hard tabs
Column: 1
(MD010, no-hard-tabs)
270-270: Hard tabs
Column: 1
(MD010, no-hard-tabs)
272-272: Hard tabs
Column: 1
(MD010, no-hard-tabs)
🪛 GitHub Check: lint
devtime_dependencies_test.go
[failure] 164-164:
cancelled is a misspelling of canceled (misspell)
[failure] 149-149:
cancelled is a misspelling of canceled (misspell)
[failure] 145-145:
cancelled is a misspelling of canceled (misspell)
[failure] 30-30:
unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _ (revive)
[failure] 21-21:
unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _ (revive)
[failure] 13-13:
fieldalignment: struct with 48 pointer bytes could be 40 (govet)
devtime_dependencies.go
[failure] 60-60:
cancelled is a misspelling of canceled (misspell)
[failure] 59-59:
cancelled is a misspelling of canceled (misspell)
[failure] 36-36:
cancelled is a misspelling of canceled (misspell)
[failure] 34-34:
cancelled is a misspelling of canceled (misspell)
🪛 GitHub Actions: golangci-lint
devtime_dependencies.go
[error] 34-34: cancelled is a misspelling of canceled (misspell)
🪛 GitHub Check: markdownlint
docs/api/devtime_dependencies.md
[failure] 86-86: Hard tabs
docs/api/devtime_dependencies.md:86:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
[failure] 85-85: Hard tabs
docs/api/devtime_dependencies.md:85:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
[failure] 84-84: Hard tabs
docs/api/devtime_dependencies.md:84:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
[failure] 79-79: Hard tabs
docs/api/devtime_dependencies.md:79:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
[failure] 75-75: Hard tabs
docs/api/devtime_dependencies.md:75:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
[failure] 74-74: Hard tabs
docs/api/devtime_dependencies.md:74:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
[failure] 73-73: Hard tabs
docs/api/devtime_dependencies.md:73:1 M
10000
D010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
[failure] 71-71: Hard tabs
docs/api/devtime_dependencies.md:71:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
[failure] 70-70: Hard tabs
docs/api/devtime_dependencies.md:70:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
[failure] 9-9: Multiple consecutive blank lines
docs/api/devtime_dependencies.md:9 MD012/no-multiple-blanks Multiple consecutive blank lines [Expected: 1; Actual: 2] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md012.md
🪛 GitHub Actions: markdownlint
docs/api/devtime_dependencies.md
[error] 9-9: markdownlint MD012/no-multiple-blanks: Multiple consecutive blank lines [Expected: 1; Actual: 2]
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Compare
- GitHub Check: Analyse
- GitHub Check: unit (1.23.x, macos-13)
- GitHub Check: unit (1.24.x, macos-latest)
- GitHub Check: unit (1.24.x, windows-latest)
- GitHub Check: unit (1.23.x, windows-latest)
- GitHub Check: repeated
🔇 Additional comments (10)
docs/api/constants.md (1)
5-5: Update sidebar_position to accommodate new documentation
Thesidebar_positionvalue has been incremented from 9 to 10 to ensure this Constants page now sits in the correct place in the sidebar after the newly added DevTimeDependencies docs.listen.go (1)
386-393: Good implementation of dev-time dependencies display in startup log!This addition nicely follows the existing pattern for displaying startup information, using consistent formatting and color schemes. The dependency count and individual entries with checkmarks provide clear visibility of active development dependencies.
app.go (2)
414-417: Well-documented configuration field for development dependencies.The documentation clearly indicates this is an optional field with a sensible default. This addition aligns with Fiber's pattern of providing flexible configuration options.
613-626: Verify context handling for long-running dependencies.The implementation correctly starts dependencies at app creation and registers a shutdown hook, but using a background context without timeout could potentially cause issues with dependencies that hang during startup or shutdown.
Consider whether timeouts should be applied to prevent potential resource leaks:
-ctx := context.Background() +ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) +defer cancel()The error handling with log warnings is appropriate for informational purposes while allowing the application to continue functioning.
docs/whats_new.md (1)
848-868: Great output example.The console output example clearly shows how the dependencies appear in the server startup logs, helping users understand what to expect when implementing this feature.
devtime_dependencies.go (3)
9-20: Well-designed interface with clear lifecycle methodsThe
DevTimeDependencyinterface provides a clean and concise API with three essential methods that cover the complete lifecycle of a development-time dependency.
22-25: LGTM: Simple helper functionThis helper function clearly encapsulates the check for configured dependencies.
29-50: Good error handling pattern for context cancellationThe implementation correctly handles context cancellation by checking for errors early and returning immediately, which allows the user to respond appropriately.
🧰 Tools
🪛 GitHub Check: lint
[failure] 36-36:
cancelledis a misspelling ofcanceled(misspell)
[failure] 34-34:
cancelledis a misspelling ofcanceled(misspell)🪛 GitHub Actions: golangci-lint
[error] 34-34:
cancelledis a misspelling ofcanceled(misspell)docs/api/devtime_dependencies.md (2)
17-29: Interface documentation looks goodThe interface definition is well-documented with clear descriptions of each method.
60-273: Excellent comprehensive examplesThe documentation provides two well-constructed examples that showcase:
- Basic integration with Redis using Testcontainers
- Advanced integration with dependency injection through Fiber's State
These examples will be very helpful for users to understand how to implement and use development-time dependencies.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
70-70: Hard tabs
Column: 1(MD010, no-hard-tabs)
71-71: Hard tabs
Column: 1(MD010, no-hard-tabs)
73-73: Hard tabs
Column: 1(MD010, no-hard-tabs)
74-74: Hard tabs
Column: 1(MD010, no-hard-tabs)
75-75: Hard tabs
Column: 1(MD010, no-hard-tabs)
79-79: Hard tabs
Column: 1(MD010, no-hard-tabs)
84-84: Hard tabs
Column: 1(MD010, no-hard-tabs)
85-85: Hard tabs
Column: 1(MD010, no-hard-tabs)
86-86: Hard tabs
Column: 1(MD010, no-hard-tabs)
87-87: Hard tabs
Column: 1(MD010, no-hard-tabs)
88-88: Hard tabs
Column: 1(MD010, no-hard-tabs)
90-90: Hard tabs
Column: 1(MD010, no-hard-tabs)
91-91: Hard tabs
Column: 1(MD010, no-hard-tabs)
97-97: Hard tabs
Column: 1(MD010, no-hard-tabs)
102-102: Hard tabs
Column: 1(MD010, no-hard-tabs)
103-103: Hard tabs
Column: 1(MD010, no-hard-tabs)
107-107: Hard tabs
Column: 1(MD010, no-hard-tabs)
109-109: Hard tabs
Column: 1(MD010, no-hard-tabs)
110-110: Hard tabs
Column: 1(MD010, no-hard-tabs)
111-111: Hard tabs
Column: 1(MD010, no-hard-tabs)
113-113: Hard tabs
Column: 1(MD010, no-hard-tabs)
115-115: Hard tabs
Column: 1(MD010, no-hard-tabs)
117-117: Hard tabs
Column: 1(MD010, no-hard-tabs)
118-118: Hard tabs
Column: 1(MD010, no-hard-tabs)
119-119: Hard tabs
Column: 1(MD010, no-hard-tabs)
120-120: Hard tabs
Column: 1(MD010, no-hard-tabs)
121-121: Hard tabs
Column: 1(MD010, no-hard-tabs)
122-122: Hard tabs
Column: 1(MD010, no-hard-tabs)
124-124: Hard tabs
Column: 1(MD010, no-hard-tabs)
125-125: Hard tabs
Column: 1(MD010, no-hard-tabs)
126-126: Hard tabs
Column: 1(MD010, no-hard-tabs)
127-127: Hard tabs
Column: 1(MD010, no-hard-tabs)
128-128: Hard tabs
Column: 1(MD010, no-hard-tabs)
129-129: Hard tabs
Column: 1(MD010, no-hard-tabs)
131-131: Hard tabs
Column: 1(MD010, no-hard-tabs)
132-132: Hard tabs
Column: 1(MD010, no-hard-tabs)
134-134: Hard tabs
Column: 1(MD010, no-hard-tabs)
135-135: Hard tabs
Column: 1(MD010, no-hard-tabs)
136-136: Hard tabs
Column: 1(MD010, no-hard-tabs)
137-137: Hard tabs
Column: 1(MD010, no-hard-tabs)
139-139: Hard tabs
Column: 1(MD010, no-hard-tabs)
152-152: Hard tabs
Column: 1(MD010, no-hard-tabs)
153-153: Hard tabs
Column: 1(MD010, no-hard-tabs)
154-154: Hard tabs
Column: 1(MD010, no-hard-tabs)
156-156: Hard tabs
Column: 1(MD010, no-hard-tabs)
157-157: Hard tabs
Column: 1(MD010, no-hard-tabs)
158-158: Hard tabs
Column: 1(MD010, no-hard-tabs)
162-162: Hard tabs
Column: 1(MD010, no-hard-tabs)
163-163: Hard tabs
Column: 1(MD010, no-hard-tabs)
164-164: Hard tabs
Column: 1(MD010, no-hard-tabs)
168-168: Hard tabs
Column: 1(MD010, no-hard-tabs)
173-173: Hard tabs
Column: 1(MD010, no-hard-tabs)
174-174: Hard tabs
Column: 1(MD010, no-hard-tabs)
175-175: Hard tabs
Column: 1(MD010, no-hard-tabs)
176-176: Hard tabs
Column: 1(MD010, no-hard-tabs)
177-177: Hard tabs
Column: 1(MD010, no-hard-tabs)
179-179: Hard tabs
Column: 1(MD010, no-hard-tabs)
180-180: Hard tabs
Column: 1(MD010, no-hard-tabs)
186-186: Hard tabs
Column: 1(MD010, no-hard-tabs)
191-191: Hard tabs
Column: 1(MD010, no-hard-tabs)
192-192: Hard tabs
Column: 1(MD010, no-hard-tabs)
196-196: Hard tabs
Column: 1(MD010, no-hard-tabs)
198-198: Hard tabs
Column: 1(MD010, no-hard-tabs)
199-199: Hard tabs
Column: 1(MD010, no-hard-tabs)
200-200: Hard tabs
Column: 1(MD010, no-hard-tabs)
202-202: Hard tabs
Column: 1(MD010, no-hard-tabs)
204-204: Hard tabs
Column: 1(MD010, no-hard-tabs)
206-206: Hard tabs
Column: 1(MD010, no-hard-tabs)
207-207: Hard tabs
Column: 1(MD010, no-hard-tabs)
208-208: Hard tabs
Column: 1(MD010, no-hard-tabs)
209-209: Hard tabs
Column: 1(MD010, no-hard-tabs)
210-210: Hard tabs
Column: 1(MD010, no-hard-tabs)
211-211: Hard tabs
Column: 1(MD010, no-hard-tabs)
213-213: Hard tabs
Column: 1(MD010, no-hard-tabs)
214-214: Hard tabs
Column: 1(MD010, no-hard-tabs)
215-215: Hard tabs
Column: 1(MD010, no-hard-tabs)
216-216: Hard tabs
Column: 1(MD010, no-hard-tabs)
217-217: Hard tabs
Column: 1(MD010, no-hard-tabs)
218-218: Hard tabs
Column: 1(MD010, no-hard-tabs)
220-220: Hard tabs
Column: 1(MD010, no-hard-tabs)
221-221: Hard tabs
Column: 1(MD010, no-hard-tabs)
223-223: Hard tabs
Column: 1(MD010, no-hard-tabs)
224-224: Hard tabs
Column: 1(MD010, no-hard-tabs)
225-225: Hard tabs
Column: 1(MD010, no-hard-tabs)
226-226: Hard tabs
Column: 1(MD010, no-hard-tabs)
228-228: Hard tabs
Column: 1(MD010, no-hard-tabs)
229-229: Hard tabs
Column: 1(MD010, no-hard-tabs)
231-231: Hard tabs
Column: 1(MD010, no-hard-tabs)
232-232: Hard tabs
Column: 1(MD010, no-hard-tabs)
233-233: Hard tabs
Column: 1(MD010, no-hard-tabs)
234-234: Hard tabs
Column: 1(MD010, no-hard-tabs)
235-235: Hard tabs
Column: 1(MD010, no-hard-tabs)
237-237: Hard tabs
Column: 1(MD010, no-hard-tabs)
238-238: Hard tabs
Column: 1(MD010, no-hard-tabs)
239-239: Hard tabs
Column: 1(MD010, no-hard-tabs)
240-240: Hard tabs
Column: 1(MD010, no-hard-tabs)
241-241: Hard tabs
Column: 1(MD010, no-hard-tabs)
243-243: Hard tabs
Column: 1(MD010, no-hard-tabs)
244-244: Hard tabs
Column: 1(MD010, no-hard-tabs)
245-245: Hard tabs
Column: 1(MD010, no-hard-tabs)
246-246: Hard tabs
Column: 1(MD010, no-hard-tabs)
247-247: Hard tabs
Column: 1(MD010, no-hard-tabs)
248-248: Hard tabs
Column: 1(MD010, no-hard-tabs)
250-250: Hard tabs
Column: 1(MD010, no-hard-tabs)
251-251: Hard tabs
Column: 1(MD010, no-hard-tabs)
253-253: Hard tabs
Column: 1(MD010, no-hard-tabs)
254-254: Hard tabs
Column: 1(MD010, no-hard-tabs)
256-256: Hard tabs
Column: 1(MD010, no-hard-tabs)
257-257: Hard tabs
Column: 1(MD010, no-hard-tabs)
258-258: Hard tabs
Column: 1(MD010, no-hard-tabs)
259-259: Hard tabs
Column: 1(MD010, no-hard-tabs)
261-261: Hard tabs
Column: 1(MD010, no-hard-tabs)
262-262: Hard tabs
Column: 1(MD010, no-hard-tabs)
263-263: Hard tabs
Column: 1(MD010, no-hard-tabs)
264-264: Hard tabs
Column: 1(MD010, no-hard-tabs)
265-265: Hard tabs
Column: 1(MD010, no-hard-tabs)
266-266: Hard tabs
Column: 1(MD010, no-hard-tabs)
267-267: Hard tabs
Column: 1(MD010, no-hard-tabs)
269-269: Hard tabs
Column: 1(MD010, no-hard-tabs)
270-270: Hard tabs
Column: 1(MD010, no-hard-tabs)
272-272: Hard tabs
Column: 1(MD010, no-hard-tabs)
🪛 GitHub Check: markdownlint
[failure] 86-86: Hard tabs
docs/api/devtime_dependencies.md:86:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
[failure] 85-85: Hard tabs
docs/api/devtime_dependencies.md:85:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
[failure] 84-84: Hard tabs
docs/api/devtime_dependencies.md:84:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
[failure] 79-79: Hard tabs
docs/api/devtime_dependencies.md:79:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
[failure] 75-75: Hard tabs
docs/api/devtime_dependencies.md:75:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
[failure] 74-74: Hard tabs
docs/api/devtime_dependencies.md:74:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
[failure] 73-73: Hard tabs
docs/api/devtime_dependencies.md:73:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
[failure] 71-71: Hard tabs
docs/api/devtime_dependencies.md:71:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
[failure] 70-70: Hard tabs
docs/api/devtime_dependencies.md:70:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
There was a problem hiding this comment.
Pull Request Overview
This pull request introduces a new feature to enable the management of development-time dependencies for Fiber applications. Key changes include the addition of the DevTimeDependency interface and its lifecycle management (start and terminate), updates to application startup and shutdown to support dependencies, and comprehensive documentation with examples.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| listen.go | Adds log output for reporting the status of development dependencies |
| docs/whats_new.md | Updates the “What's New” documentation to include the new feature |
| docs/api/devtime_dependencies.md | Provides API documentation for the DevTimeDependency interface |
| docs/api/constants.md | Adjusts sidebar position to include new documentation entries |
| devtime_dependencies_test.go | Implements unit tests to verify dependency start and shutdown flows |
| devtime_dependencies.go | Introduces the core implementation and management functions |
| app.go | Updates application initialization to configure and manage the new dependencies |
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (10)
devtime_dependencies_test.go (3)
21-24:⚠️ Potential issueHandle context parameter in the Start method.
The mock currently ignores the context parameter in the Start method, which doesn't properly test context cancellation behavior.
func (m *mockDependency) Start(ctx context.Context) error { + select { + case <-ctx.Done(): + return ctx.Err() + default: + } m.started = true return m.startError }
30-33:⚠️ Potential issueHandle context parameter in the Terminate method.
Similarly, the mock ignores the context parameter in the Terminate method, which doesn't properly test context cancellation behavior.
func (m *mockDependency) Terminate(ctx context.Context) error { + select { + case <-ctx.Done(): + return ctx.Err() + default: + } m.terminated = true return m.terminateError }
154-175:⚠️ Potential issueSimilar issue with testing context cancellation for termination.
The Terminate test has the same issue as the Start test - it doesn't ensure the mock implementation actually respects context cancellation.
func TestDevTimeDependenciesTerminateWithContextCancellation(t *testing.T) { // Create a dependency that takes some time to terminate - slowDep := &mockDependency{name: "slow-dep"} + slowDep := &mockDependency{ + name: "slow-dep", + // Add artificial delay to ensure context actually times out + terminateFunc: func(ctx context.Context) error { + select { + case <-ctx.Done(): + return ctx.Err() + case <-time.After(100 * time.Millisecond): + return nil + } + }, + } app := &App{ configured: Config{ DevTimeDependencies: []DevTimeDependency{slowDep}, }, } // Start dependencies with cancelled context err := app.startDevTimeDependencies(context.Background()) require.NoError(t, err) // Create a new context for shutdown ctx, cancel := context.WithTimeout(context.Background(), 1*time.Nanosecond) defer cancel() // Shutdown dependencies with cancelled context err = app.shutdownDevTimeDependencies(ctx) require.ErrorIs(t, err, context.DeadlineExceeded) }docs/api/devtime_dependencies.md (7)
8-9: Fix consecutive blank lines.Remove the extra blank line to fix the markdown linting error.
Development-time services provide external dependencies needed to run the application while developing it. They are only supposed to be used while developing and are disabled when the application is deployed 6D4E . - ## DevTimeDependency Interface
81-91:⚠️ Potential issueFix incorrect interface reference.
The comment references "fiber.RuntimeDependency" when it should be "fiber.DevTimeDependency".
-// Start initializes and starts the dependency. It implements the fiber.RuntimeDependency interface. +// Start initializes and starts the dependency. It implements the fiber.DevTimeDependency interface. func (s *redisStore) Start(ctx context.Context) error { // start the dependency c, err := tcredis.Run(ctx, "redis:latest") if err != nil { return err } s.ctr = c return nil }
93-97:⚠️ Potential issueFix incorrect interface reference in the String method comment.
The String method comment also references "fiber.RuntimeDependency" instead of "fiber.DevTimeDependency".
// String returns a human-readable representation of the dependency's state. -// It implements the fiber.RuntimeDependency interface. +// It implements the fiber.DevTimeDependency interface. func (s *redisStore) String() string { return "redis-store" }
99-103:⚠️ Potential issueFix incorrect interface reference in the Terminate method comment.
The Terminate method comment also references "fiber.RuntimeDependency" instead of "fiber.DevTimeDependency".
-// Terminate stops and removes the dependency. It implements the fiber.RuntimeDependency interface. +// Terminate stops and removes the dependency. It implements the fiber.DevTimeDependency interface. func (s *redisStore) Terminate(ctx context.Context) error { // stop the dependency return s.ctr.Terminate(ctx) }
170-180:⚠️ Potential issueFix incorrect interface reference in the second example.
The Start method comment in the second example also references "fiber.RuntimeDependency" incorrectly.
-// Start initializes and starts the dependency. It implements the fiber.RuntimeDependency interface. +// Start initializes and starts the dependency. It implements the fiber.DevTimeDependency interface. func (s *redisStore) Start(ctx context.Context) error { // start the dependency c, err := tcredis.Run(ctx, "redis:latest") if err != nil { return err } s.ctr = c return nil }
182-186:⚠️ Potential issueFix incorrect interface reference in the String method comment.
The String method comment in the second example also needs to be corrected.
// String returns a human-readable representation of the dependency's state. -// It implements the fiber.RuntimeDependency interface. +// It implements the fiber.DevTimeDependency interface. func (s *redisStore) String() string { return "redis-store" }
188-192:⚠️ Potential issueFix incorrect interface reference in the Terminate method comment.
The Terminate method comment in the second example also needs to be corrected.
-// Terminate stops and removes the dependency. It implements the fiber.RuntimeDependency interface. +// Terminate stops and removes the dependency. It implements the fiber.DevTimeDependency interface. func (s *redisStore) Terminate(ctx context.Context) error { // stop the dependency return s.ctr.Terminate(ctx) }
🧹 Nitpick comments (5)
devtime_dependencies_benchmark_test.go (2)
16-21: Consider handling context cancellation in the benchmark dependency.While this is a benchmark implementation, the mock should ideally respect context cancellation to accurately benchmark real-world behavior.
func (m *benchmarkDependency) Start(ctx context.Context) error { + select { + case <-ctx.Done(): + return ctx.Err() + default: + } if m.startDelay > 0 { time.Sleep(m.startDelay) } return nil }
27-32: Consider handling context cancellation in the Terminate method.Similar to the Start method, the Terminate implementation should respect context cancellation.
func (m *benchmarkDependency) Terminate(ctx context.Context) error { + select { + case <-ctx.Done(): + return ctx.Err() + default: + } if m.terminateDelay > 0 { time.Sleep(m.terminateDelay) } return nil }devtime_dependencies_test.go (1)
13-19: Optimize struct field alignment and size.The
mockDependencystruct has a suboptimal field alignment that could be improved to reduce its memory footprint.// mockDependency implements DevTimeDependency interface for testing type mockDependency struct { name string + started bool + terminated bool startError error terminateError error - started bool - terminated bool }🧰 Tools
🪛 GitHub Check: lint
[failure] 13-13:
fieldalignment: struct with 48 pointer bytes could be 40 (govet)devtime_dependencies.go (1)
58-62: Consider different error handling on context cancellation.When a context is canceled during shutdown, the code currently appends the error to the error list and continues with other dependencies. This differs from the start method's approach where context cancellation causes an immediate return. It might be beneficial to have consistent behavior.
Consider this alternative approach for handling context cancellation during shutdown:
func (app *App) shutdownDevTimeDependencies(ctx context.Context) error { if app.hasDevTimeDependencies() { var errs []error for _, dep := range app.configured.DevTimeDependencies { if err := ctx.Err(); err != nil { - // Context is canceled, do a best effort to terminate the dependencies. - errs = append(errs, fmt.Errorf("context canceled: %w", err)) - continue + // Context is canceled, return an error immediately, consistent with startDevTimeDependencies + return fmt.Errorf("context canceled while terminating dependencies: %w", err) } err := dep.Terminate(ctx) if err != nil { if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { return fmt.Errorf("dependency %s terminate: %w", dep.String(), err) } errs = append(errs, fmt.Errorf("terminate dependency %s: %w", dep.String(), err)) } } return errors.Join(errs...) } return nil }Alternatively, explain in a comment why the behavior is different between start and shutdown:
if err := ctx.Err(); err != nil { - // Context is canceled, do a best effort to terminate the dependencies. + // Context is canceled, but we continue with a best effort to terminate all dependencies + // unlike startDevTimeDependencies which returns immediately, because it's more important + // to attempt to clean up all resources even if the context is canceled. errs = append(errs, fmt.Errorf("context canceled: %w", err)) continue }docs/api/devtime_dependencies.md (1)
227-229: Consider using dependency injection middleware instead of state.Using the app's state for dependency injection works, but consider suggesting a middleware-based approach that provides a more structured way to inject dependencies into the request handlers.
// Create a middleware to inject Redis client func WithRedis(client *redis.Client) fiber.Handler { return func(c fiber.Ctx) error { c.Locals("redis", client) return c.Next() } } // In your routes setup: app.Use(WithRedis(rdb)) // Then in your handlers: app.Post("/user/create", func(c fiber.Ctx) error { // Get Redis client from context rdb := c.Locals("redis").(*redis.Client) // Use the client... })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
devtime_dependencies.go(1 hunks)devtime_dependencies_benchmark_test.go(1 hunks)devtime_dependencies_test.go(1 hunks)docs/api/devtime_dependencies.md(1 hunks)docs/whats_new.md(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/whats_new.md
🧰 Additional context used
🧬 Code Graph Analysis (1)
devtime_dependencies_benchmark_test.go (1)
devtime_dependencies.go (1)
DevTimeDependency(10-20)
🪛 GitHub Check: lint
devtime_dependencies_benchmark_test.go
[failure] 162-162:
Error return value of app.shutdownDevTimeDependencies is not checked (errcheck)
[failure] 161-161:
Error return value of app.startDevTimeDependencies is not checked (errcheck)
[failure] 150-150:
test helper function should start from b.Helper() (thelper)
[failure] 129-129:
Error return value of app.startDevTimeDependencies is not checked (errcheck)
[failure] 119-119:
test helper function should start from b.Helper() (thelper)
[failure] 87-87:
Error return value of app.shutdownDevTimeDependencies is not checked (errcheck)
[failure] 77-77:
test helper function should start from b.Helper() (thelper)
[failure] 45-45:
Error return value of app.startDevTimeDependencies is not checked (errcheck)
[failure] 35-35:
test helper function should start from b.Helper() (thelper)
devtime_dependencies_test.go
[failure] 13-13:
fieldalignment: struct with 48 pointer bytes could be 40 (govet)
🪛 GitHub Actions: golangci-lint
devtime_dependencies_benchmark_test.go
[error] 35-35: golangci-lint: test helper function should start from b.Helper() (thelper)
🪛 GitHub Check: codecov/patch
devtime_dependencies.go
[warning] 42-43: devtime_dependencies.go#L42-L43
Added lines #L42 - L43 were not covered by tests
[warning] 67-68: devtime_dependencies.go#L67-L68
Added lines #L67 - L68 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: unit (1.24.x, macos-13)
- GitHub Check: unit (1.24.x, windows-latest)
- GitHub Check: unit (1.23.x, windows-latest)
- GitHub Check: unit (1.23.x, macos-13)
- GitHub Check: repeated
- GitHub Check: Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
devtime_dependencies_benchmark_test.go (4)
34-49: Check error returns from startDevTimeDependencies.The error returned by
app.startDevTimeDependenciesis being discarded, which is flagged by the linter. Error checking is important even in benchmarks to ensure the benchmark is measuring what it's supposed to.Apply this fix as mentioned in previous reviews:
for i := 0; i < b.N; i++ { ctx := context.Background() - _ = app.startDevTimeDependencies(ctx) + if err := app.startDevTimeDependencies(ctx); err != nil { + b.Fatal("Failed to start dependencies:", err) + } }🧰 Tools
🪛 GitHub Check: lint
[failure] 47-47:
Error return value ofapp.startDevTimeDependenciesis not checked (errcheck)
78-93: Check error returns from shutdownDevTimeDependencies.Similar to the start benchmark, the error returned by
app.shutdownDevTimeDependenciesis being discarded.Apply this fix as mentioned in previous reviews:
for i := 0; i < b.N; i++ { ctx := context.Background() - _ = app.shutdownDevTimeDependencies(ctx) + if err := app.shutdownDevTimeDependencies(ctx); err != nil { + b.Fatal("Failed to shutdown dependencies:", err) + } }🧰 Tools
🪛 GitHub Check: lint
[failure] 91-91:
Error return value ofapp.shutdownDevTimeDependenciesis not checked (errcheck)
122-138: Check and validate errors from context cancellation benchmark.The error from
app.startDevTimeDependenciesis discarded, but with an immediate timeout (1 nanosecond), we expect an error to occur due to context cancellation. The benchmark should validate this behavior.Apply this fix as suggested in previous reviews:
for i := 0; i < b.N; i++ { ctx, cancel := context.WithTimeout(context.Background(), timeout) - _ = app.startDevTimeDependencies(ctx) + err := app.startDevTimeDependencies(ctx) + // We expect an error here due to the short timeout + if err == nil && timeout < time.Microsecond { + b.Fatal("Expected error due to context cancellation but got none") + } cancel() }🧰 Tools
🪛 GitHub Check: lint
[failure] 135-135:
Error return value ofapp.startDevTimeDependenciesis not checked (errcheck)
155-172: Check errors in memory benchmark.The memory benchmark ignores errors from both
startDevTimeDependenciesandshutdownDevTimeDependencies, which should be checked to ensure the benchmark is valid.Apply this fix as suggested in previous reviews:
for i := 0; i < b.N; i++ { ctx := context.Background() - _ = app.startDevTimeDependencies(ctx) - _ = app.shutdownDevTimeDependencies(ctx) + if err := app.startDevTimeDependencies(ctx); err != nil { + b.Fatal("Failed to start dependencies:", err) + } + if err := app.shutdownDevTimeDependencies(ctx); err != nil { + b.Fatal("Failed to shutdown dependencies:", err) + } }🧰 Tools
🪛 GitHub Check: lint
[failure] 170-170:
Error return value ofapp.shutdownDevTimeDependenciesis not checked (errcheck)
[failure] 169-169:
Error return value ofapp.startDevTimeDependenciesis not checked (errcheck)
🧹 Nitpick comments (2)
devtime_dependencies_benchmark_test.go (2)
10-14: Optimize struct field layout for better memory efficiency.The
benchmarkDependencystruct has a suboptimal memory layout. Go's struct field alignment affects memory usage and performance, especially in performance-sensitive code like benchmarks.Reordering the fields can reduce the struct size from 24 to 8 bytes:
type benchmarkDependency struct { - startDelay time.Duration - terminateDelay time.Duration name string + startDelay time.Duration + terminateDelay time.Duration }🧰 Tools
🪛 GitHub Check: lint
[failure] 10-10:
fieldalignment: struct with 24 pointer bytes could be 8 (govet)🪛 GitHub Actions: golangci-lint
[error] 10-10: golangci-lint: fieldalignment: struct with 24 pointer bytes could be 8 (govet)
140-152: Consider adding more diverse timeout values for context cancellation tests.Currently, the benchmark only tests with an immediate cancellation (1 nanosecond). Consider adding tests with varying timeouts to better understand the performance characteristics under different cancellation scenarios.
Add a test case with a longer timeout that still triggers cancellation:
b.Run("multiple-dependencies/delayed-cancellation", func(b *testing.B) { benchmarkFn(b, []DevTimeDependency{ &benchmarkDependency{name: "dep1", startDelay: 10 * time.Millisecond}, &benchmarkDependency{name: "dep2", startDelay: 20 * time.Millisecond}, &benchmarkDependency{name: "dep3", startDelay: 30 * time.Millisecond}, }, 15*time.Millisecond) // Cancels after first dependency starts but before others })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
devtime_dependencies_benchmark_test.go(1 hunks)devtime_dependencies_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- devtime_dependencies_test.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
devtime_dependencies_benchmark_test.go (1)
devtime_dependencies.go (1)
DevTimeDependency(10-20)
🪛 GitHub Check: lint
devtime_dependencies_benchmark_test.go
[failure] 170-170:
Error return value of app.shutdownDevTimeDependencies is not checked (errcheck)
[failure] 169-169:
Error return value of app.startDevTimeDependencies is not checked (errcheck)
[failure] 135-135:
Error return value of app.startDevTimeDependencies is not checked (errcheck)
[failure] 91-91:
Error return value of app.shutdownDevTimeDependencies is not checked (errcheck)
[failure] 47-47:
Error return value of app.startDevTimeDependencies is not checked (errcheck)
[failure] 10-10:
fieldalignment: struct with 24 pointer bytes could be 8 (govet)
🪛 GitHub Actions: golangci-lint
devtime_dependencies_benchmark_test.go
[error] 10-10: golangci-lint: fieldalignment: struct with 24 pointer bytes could be 8 (govet)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: unit (1.24.x, macos-latest)
- GitHub Check: unit (1.24.x, windows-latest)
- GitHub Check: unit (1.24.x, macos-13)
- GitHub Check: unit (1.23.x, macos-latest)
- GitHub Check: unit (1.23.x, macos-13)
- GitHub Check: unit (1.23.x, windows-latest)
- GitHub Check: Compare
- GitHub Check: repeated
- GitHub Check: Analyse
🔇 Additional comments (3)
devtime_dependencies_benchmark_test.go (3)
16-21: LGTM: Simple and effective context-ignoring implementation.The Start method correctly simulates delays while ignoring the context parameter, which is appropriate for these simple benchmarks.
23-25: LGTM: Straightforward String implementation.This method correctly returns the name of the benchmark dependency, fulfilling the interface requirement.
27-32: LGTM: Terminate implementation matches Start pattern.The Terminate method appropriately mirrors the Start method's implementation with delay simulation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
devtime_dependencies_prod.go (4)
1-2: Remove redundant build tagSince Go 1.17, the older style build tag format
// +build !devis redundant when you're already using the modern//go:build !devdirective.//go:build !dev - // +build !dev🧰 Tools
🪛 GitHub Check: lint
[failure] 2-2:
redundant-build-tag: The build tag "// +build" is redundant since Go 1.17 and can be removed (revive)
19-21: Rename unused receiver variableThe receiver variable
appis not used in this method. Consider renaming it to_to indicate it's intentionally unused.- func (app *App) hasDevTimeDependencies() bool { + func (_ *App) hasDevTimeDependencies() bool { return false }🧰 Tools
🪛 GitHub Check: lint
[failure] 19-19:
unused-receiver: method receiver 'app' is not referenced in method's body, consider removing or renaming it as _ (revive)
24-26: Receiver variable is already properly named for unused parameterThis function correctly uses
_for the unused context parameter, but the receiver variableappis still not used. Consider renaming it as well.- func (app *App) startDevTimeDependencies(_ context.Context) error { + func (_ *App) startDevTimeDependencies(_ context.Context) error { return nil }🧰 Tools
🪛 GitHub Check: lint
[failure] 24-24:
unused-receiver: method receiver 'app' is not referenced in method's body, consider removing or renaming it as _ (revive)
29-31: Rename unused receiver variableThe receiver variable
appis not used in this method. Consider renaming it to_to indicate it's intentionally unused.- func (app *App) shutdownDevTimeDependencies(_ context.Context) error { + func (_ *App) shutdownDevTimeDependencies(_ context.Context) error { return nil }🧰 Tools
🪛 GitHub Check: lint
[failure] 29-29:
unused-receiver: method receiver 'app' is not referenced in method's body, consider removing or renaming it as _ (revive)devtime_dependencies_dev.go (4)
26-29: Fix comment style for methodGo method comments should begin with the method name. This helps generate proper documentation with
godoc.- // hasDevTimeDependencies Checks if there are any dependency for the current application. + // hasDevTimeDependencies checks if there are any dependencies for the current application.Also, there's a minor grammatical error in the comment (should be "dependencies" plural rather than "dependency").
31-54: Logic for starting dependencies looks goodYou've implemented proper error handling and context cancellation for starting dependencies. The approach of immediately returning on context cancellation ensures that users can quickly respond to cancellation events.
One small grammatical improvement:
- // startDevTimeDependencies Handles the start process of dependencies for the current application. + // startDevTimeDependencies handles the start process of dependencies for the current application.
56-79: Different error handling strategies between start and shutdownI noticed that the error handling strategy differs between
startDevTimeDependenciesandshutdownDevTimeDependencies:
- In
startDevTimeDependencies, if context is canceled, you return immediately- In
shutdownDevTimeDependencies, if context is canceled, you continue with best-effort terminationThis difference is likely intentional to ensure all dependencies attempt termination even if the context is canceled, but it might be worth adding a comment explaining this design decision for future maintainers.
- // shutdownDevTimeDependencies Handles the shutdown process of dependencies for the current application. - // Iterates over all dependencies and tries to terminate them, returning an error if any error occurs. + // shutdownDevTimeDependencies handles the shutdown process of dependencies for the current application. + // Iterates over all dependencies and tries to terminate them, returning an error if any error occurs. + // Unlike startDevTimeDependencies, this method continues with best-effort termination even if the context + // is canceled to ensure as many dependencies as possible are properly terminated.
81-91: Logging assumes all dependencies successfully startedThe logging implementation assumes all dependencies are in "OK" state (line 88). Consider modifying the logging to handle cases where dependencies might have failed to start, or make it clear this only logs started dependencies.
You could consider a more robust approach that shows the status of each dependency:
func (app *App) logDevTimeDependencies(out io.Writer, colors Colors) { if app.hasDevTimeDependencies() { fmt.Fprintf(out, "%sINFO%s Dev-time dependencies: \t%s%d%s\n", colors.Green, colors.Reset, colors.Blue, len(app.configured.DevTimeDependencies), colors.Reset) for _, dep := range app.configured.DevTimeDependencies { - fmt.Fprintf(out, "%sINFO%s 🥡 %s[ OK ] %s%s\n", colors.Green, colors.Reset, colors.Blue, dep.String(), colors.Reset) + // Assuming you track status of dependencies in the app or add a Status() method to the interface + status := "[ OK ]" + statusColor := colors.Blue + // if depFailed(dep) { // This would require tracking dependency start status + // status = "[FAIL]" + // statusColor = colors.Red + // } + fmt.Fprintf(out, "%sINFO%s 🥡 %s%s %s%s\n", colors.Green, colors.Reset, statusColor, status, dep.String(), colors.Reset) } } }Alternatively, fix the comment to clarify this only logs started dependencies:
- // logDevTimeDependencies logs information about development-time dependencies + // logDevTimeDependencies logs information about successfully started development-time dependencies
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
devtime_dependencies_dev.go(1 hunks)devtime_dependencies_prod.go(1 hunks)listen.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- listen.go
🧰 Additional context used
🪛 GitHub Check: lint
devtime_dependencies_prod.go
[failure] 29-29:
unused-receiver: method receiver 'app' is not referenced in method's body, consider removing or renaming it as _ (revive)
[failure] 24-24:
unused-receiver: method receiver 'app' is not referenced in method's body, consider removing or renaming it as _ (revive)
[failure] 19-19:
unused-receiver: method receiver 'app' is not referenced in method's body, consider removing or renaming it as _ (revive)
[failure] 2-2:
redundant-build-tag: The build tag "// +build" is redundant since Go 1.17 and can be removed (revive)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Compare
- GitHub Check: Analyse
- GitHub Check: repeated
🔇 Additional comments (3)
devtime_dependencies_prod.go (2)
11-16: Interface definition looks goodThe
DevTimeDependencyinterface is well-defined with the three essential methods for managing development-time dependencies:Start,String, andTerminate.
34-34: LGTMThe no-op implementation for
logDevTimeDependencieslooks good. You've correctly defined the function with unused parameters properly marked with underscores.devtime_dependencies_dev.go (1)
13-24: Interface definition with documentation looks goodThe
DevTimeDependencyinterface is well-documented with clear descriptions for each method. This will help developers implement their own development-time dependencies.
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new abstraction for managing development-time dependencies (“Services”) in Fiber v3, inspired by Spring Boot’s dev-services. The changes include a new Service interface and its implementations in services.go, comprehensive tests in services_test.go and benchmarks in services_benchmark_test.go, as well as updates to documentation and integration into app startup and shutdown procedures.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| services_test.go | Added extensive tests for service start, shutdown, context handling. |
| services_benchmark_test.go | Benchmarks for service startup, shutdown, and context cancellation. |
| services.go | Implementation of the Service interface and lifecycle management. |
| listen.go | Integration of service logging into the startup message routine. |
| docs/whats_new.md | Documentation update featuring the new Services feature. |
| docs/api/services.md | API documentation for Services with usage examples and details. |
| docs/api/constants.md | Minor update to sidebar positioning for constants documentation. |
| app.go | Integration of automatic service startup and shutdown in app lifecycle. |
listen.go
Outdated
| @@ -383,6 +383,12 @@ func (app *App) startupMessage(addr string, isTLS bool, pids string, cfg ListenC | |||
| fmt.Fprintf(out, "%sINFO%s Application name: \t\t%s%s%s\n", colors.Green, colors.Reset, colors.Blue, app.config.AppName, colors.Reset) | |||
| } | |||
|
|
|||
| // TODO: Make this startup timeout log configurable | |||
There was a problem hiding this comment.
Consider exposing the startup timeout for service logging as a configurable parameter instead of hardcoding 30 seconds. This would improve flexibility for users who may need a different timeout value.
Copilot uses AI. Check for mistakes.
app.go
Outdated
| // TODO: support for custom context | ||
| ctx := context.Background() |
There was a problem hiding this comment.
[nitpick] Consider allowing customization of the context used for service startup and shutdown rather than always using context.Background. This would enable users to pass cancellation signals or custom deadlines as needed.
| // TODO: support for custom context | |
| ctx := context.Background() | |
| // Allow customization of context for service startup and shutdown | |
| ctx := app.config.Context | |
| if ctx == nil { | |
| ctx = context.Background() | |
| } |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
listen.go (1)
386-391: Good integration of service status logging into startup process.The implementation correctly creates a timeout context with proper cleanup via
defer cancel()and passes it to the new service logging function. This ensures service status is displayed during application startup without blocking indefinitely.Consider making the 30-second timeout configurable through the app configuration, as noted in your TODO comment.
docs/whats_new.md (1)
792-875: Comprehensive documentation for the new Services feature.The documentation clearly explains the purpose of the Services feature and provides a complete example implementation of the Service interface. This helps developers understand how to leverage this feature in their applications.
In the service implementation example, consider returning
c.imgdirectly in the String() method rather than having an unnecessary return statement that just returns a variable.func (s *myService) String() string { - return s.img + return s.img }services.go (2)
60-76: Terminate services in reverse order of their startDependencies are usually started A ➜ B ➜ C and should be shut down C ➜ B ➜ A to respect potential ordering constraints. Reverse the slice when iterating:
-for _, dep := range app.configured.Services { +for i := len(app.configured.Services) - 1; i >= 0; i-- { + dep := app.configured.Services[i]
81-98: Minor logging polish
strings.ToUpper(state)is called twice.- A successful state might deserve a green colour for better visual scanning.
- } else { - stateColor = colors.Blue - state = strings.ToUpper(state) + } else { + stateColor = colors.Green + state = strings.ToUpper(state) } - fmt.Fprintf(out, "%sINFO%s 🥡 %s[ %s ] %s%s\n", colors.Green, colors.Reset, stateColor, strings.ToUpper(state), dep.String(), colors.Reset) + fmt.Fprintf(out, "%sINFO%s 🥡 %s[ %s ] %s%s\n", colors.Green, colors.Reset, stateColor, state, dep.String(), colors.Reset)[note: purely cosmetic]
services_test.go (2)
240-261: Simplify the already-canceled context testThe test for already-canceled context in shutdownServices calls
startServicesbefore testingshutdownServices, which is unnecessary for this specific test case.Consider simplifying this test to focus directly on the shutdown behavior with a canceled context:
t.Run("already-canceled", func(t *testing.T) { app := &App{ configured: Config{ Services: []Service{ &mockService{name: "dep1"}, }, }, } - err := app.startServices(context.Background()) - require.NoError(t, err) - // Create a context that is already canceled ctx, cancel := context.WithCancel(context.Background()) cancel() err = app.shutdownServices(ctx) require.Error(t, err) require.ErrorIs(t, err, context.Canceled) require.Contains(t, err.Error(), "service dep1 terminate") })
356-400: Good coverage of service logging functionalityThe test for
logServicesverifies correct output formatting including color handling and proper state representation.Consider making the test more robust against changes by:
- Avoiding hardcoded service count in the expected output
- Using the actual values from the Colors struct rather than repeating the color codes
expecteds := []string{ - fmt.Sprintf("%sINFO%s Services: \t%s%d%s\n", colors.Green, colors.Reset, colors.Blue, len(app.configured.Services), colors.Reset), + fmt.Sprintf("%sINFO%s Services: \t%s%d%s\n", colors.Green, colors.Reset, colors.Blue, len(app.configured.Services), colors.Reset), }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app.go(2 hunks)docs/api/constants.md(1 hunks)docs/api/services.md(1 hunks)docs/whats_new.md(2 hunks)listen.go(1 hunks)services.go(1 hunks)services_benchmark_test.go(1 hunks)services_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
app.go (3)
services.go (1)
Service(12-25)log/fiberlog.go (1)
Warnf(54-56)hooks.go (1)
Hooks(21-35)
services.go (3)
state.go (1)
State(9-11)app.go (1)
App(90-134)color.go (1)
Colors(8-53)
🪛 GitHub Check: codecov/patch
app.go
[warning] 614-618: app.go#L614-L618
Added lines #L614 - L618 were not covered by tests
[warning] 620-624: app.go#L620-L624
Added lines #L620 - L624 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: unit (1.24.x, windows-latest)
- GitHub Check: Compare
- GitHub Check: unit (1.23.x, windows-latest)
- GitHub Check: repeated
🔇 Additional comments (8)
docs/api/constants.md (1)
5-5: Sidebar position updated for documentation organization.The sidebar position for constants.md is updated from 9 to 10, presumably to accommodate the new services.md documentation file in the proper sequence.
app.go (1)
414-417: Well-documented Services field addition to Config.The new field has clear documentation explaining its purpose and default value. This maintains the same documentation standard as other config fields.
services_test.go (6)
15-104: Well-designed mock implementation for testingThe
mockServiceimplementation is comprehensive and covers various test scenarios including error conditions, state tracking, delays, and proper context handling. This will allow for thorough testing of the Service interface.
106-121: Good test structure for hasServicesThe test cases for the
hasServicesmethod are well-organized with a helper function and clear test cases covering both scenarios (with and without services).
123-143: Effective helper function for reducing test duplicationThe
testServiceOperationhelper eliminates code duplication in service testing, making the tests more maintainable.
145-214: Comprehensive test coverage for startServicesThe tests cover all important scenarios: no services, successful start, failed start, and context cancellation (both already canceled and during operation).
287-313: Excellent test coverage for context error handlingThe test cases specifically verify proper handling of Context.Canceled and Context.DeadlineExceeded errors during service termination, which is critical for proper shutdown behavior.
315-354: Good test for multiple service error aggregationThese tests ensure that when multiple services fail, all errors are properly reported in the aggregated error, which will provide better diagnostics for users.
app.go
Outdated
Comment on lines
611
to
626
// If the app is configured to use services,
// register a post shutdown hook to shutdown them after the server is closed.
if app.hasServices() {
// TODO: support for custom context
ctx := context.Background()
if err := app.startServices(ctx); err != nil {
log.Warnf("failed to start services: %v", err)
}
app.Hooks().OnPostShutdown(func(_ error) error {
if err := app.shutdownServices(ctx); err != nil {
log.Warnf("failed to shutdown services: %v", err)
}
return nil
})
}
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Service lifecycle management implementation needs context configurability.
The implementation correctly handles service startup during app initialization and graceful shutdown via hooks. The warning logs on failures are appropriate for non-critical development dependencies.
Consider making the dependency context configurable instead of hardcoding context.Background() so users can pass custom contexts for dependency start and shutdown, enhancing flexibility.
- // TODO: support for custom context
- ctx := context.Background()
+ // Use the provided context or default to context.Background()
+ ctx := context.Background()
+ if app.config.ServiceContext != nil {
+ ctx = app.config.ServiceContext
+ }
Additionally, consider adding test coverage for these blocks as static analysis shows they're not currently covered.
🏁 Script executed:
#!/bin/bash
# Find test coverage for the services-related code
rg -A 3 -B 3 "hasServices|startServices|shutdownServices" *_test.go
Length of output: 6114
I’d like to confirm whether App already exposes a configurable context field before suggesting the exact diff. Let’s inspect the App struct and see if something like config.ServiceContext (or any context field) exists:
🏁 Script executed:
#!/bin/bash
# Show the App struct to check for an existing config/context field
rg -n "type App struct" -A 10 -B 0
# Look for any “ServiceContext” identifiers in the repo
rg -n "ServiceContext" -n .
Length of output: 1218
Enable configurable service context
The service startup/shutdown logic currently hardcodes context.Background(). Since there’s no existing field or option on App to override this, let’s introduce one and wire it through both startup and shutdown:
- In
app.go, extend the App struct:
type App struct {
// existing fields…
hooks *Hooks
+ // service context provider (defaults to Background)
+ serviceCtx func() context.Context
}
- Update the constructor (e.g.
NewApp) to initialize the provider and add an option:
func NewApp(opts ...AppOption) *App {
a := &App{
- hooks: /*…*/,
+ hooks: /*…*/,
+ serviceCtx: func() context.Context { return context.Background() },
}
for _, opt := range opts {
opt(a)
}
return a
}
+// WithServiceContext lets callers supply a custom Context for service lifecycle.
+func WithServiceContext(provider func() context.Context) AppOption {
+ return func(a *App) {
+ a.serviceCtx = provider
+ }
+}
- In the shutdown hook block (~lines 611–626), replace
context.Background() with the provider:
- ctx := context.Background()
+ ctx := app.serviceCtx()
if err := app.startServices(ctx); err != nil {
log.Warnf("failed to start services: %v", err)
}
app.Hooks().OnPostShutdown(func(_ error) error {
- if err := app.shutdownServices(ctx); err != nil {
+ if err := app.shutdownServices(app.serviceCtx()); err != nil {
log.Warnf("failed to shutdown services: %v", err)
}
return nil
})
- Add unit tests (in
services_test.go) verifying that WithServiceContext is honored during both start and shutdown.
This makes the context fully pluggable while preserving the default behavior.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
// If the app is configured to use services,
// register a post shutdown hook to shutdown them after the server is closed.
if app.hasServices() {
// TODO: support for custom context
ctx := context.Background()
if err := app.startServices(ctx); err != nil {
log.Warnf("failed to start services: %v", err)
}
app.Hooks().OnPostShutdown(func(_ error) error {
if err := app.shutdownServices(ctx); err != nil {
log.Warnf("failed to shutdown services: %v", err)
}
return nil
})
}
// If the app is configured to use services,
// register a post shutdown hook to shutdown them after the server is closed.
if app.hasServices() {
// TODO: support for custom context
- ctx := context.Background()
+ ctx := app.serviceCtx()
if err := app.startServices(ctx); err != nil {
log.Warnf("failed to start services: %v", err)
}
app.Hooks().OnPostShutdown(func(_ error) error {
- if err := app.shutdownServices(ctx); err != nil {
+ if err := app.shutdownServices(app.serviceCtx()); err != nil {
log.Warnf("failed to shutdown services: %v", err)
}
return nil
})
}
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 614-618: app.go#L614-L618
Added lines #L614 - L618 were not covered by tests
[warning] 620-624: app.go#L620-L624
Added lines #L620 - L624 were not covered by tests
| // If the app is configured to use services, | ||
| // register a post shutdown hook to shutdown them after the server is closed. | ||
| if app.hasServices() { | ||
| // TODO: support for custom context | ||
| ctx := context.Background() | ||
| if err := app.startServices(ctx); err != nil { | ||
| log.Warnf("failed to start services: %v", err) | ||
| } | ||
|
|
||
| app.Hooks().OnPostShutdown(func(_ error) error { | ||
| if err := app.shutdownServices(ctx); err != nil { | ||
| log.Warnf("failed to shutdown services: %v", err) | ||
| } | ||
| return nil | ||
| }) | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Service lifecycle management implementation needs context configurability.
The implementation correctly handles service startup during app initialization and graceful shutdown via hooks. The warning logs on failures are appropriate for non-critical development dependencies.
Consider making the dependency context configurable instead of hardcoding context.Background() so users can pass custom contexts for dependency start and shutdown, enhancing flexibility.
- // TODO: support for custom context
- ctx := context.Background()
+ // Use the provided context or default to context.Background()
+ ctx := context.Background()
+ if app.config.ServiceContext != nil {
+ ctx = app.config.ServiceContext
+ }Additionally, consider adding test coverage for these blocks as static analysis shows they're not currently covered.
🏁 Script executed:
#!/bin/bash
# Find test coverage for the services-related code
rg -A 3 -B 3 "hasServices|startServices|shutdownServices" *_test.goLength of output: 6114
I’d like to confirm whether App already exposes a configurable context field before suggesting the exact diff. Let’s inspect the App struct and see if something like config.ServiceContext (or any context field) exists:
🏁 Script executed:
#!/bin/bash
# Show the App struct to check for an existing config/context field
rg -n "type App struct" -A 10 -B 0
# Look for any “ServiceContext” identifiers in the repo
rg -n "ServiceContext" -n .Length of output: 1218
Enable configurable service context
The service startup/shutdown logic currently hardcodes context.Background(). Since there’s no existing field or option on App to override this, let’s introduce one and wire it through both startup and shutdown:
- In
app.go, extend theAppstruct:
type App struct {
// existing fields…
hooks *Hooks
+ // service context provider (defaults to Background)
+ serviceCtx func() context.Context
}- Update the constructor (e.g.
NewApp) to initialize the provider and add an option:
func NewApp(opts ...AppOption) *App {
a := &App{
- hooks: /*…*/,
+ hooks: /*…*/,
+ serviceCtx: func() context.Context { return context.Background() },
}
for _, opt := range opts {
opt(a)
}
return a
}
+// WithServiceContext lets callers supply a custom Context for service lifecycle.
+func WithServiceContext(provider func() context.Context) AppOption {
+ return func(a *App) {
+ a.serviceCtx = provider
+ }
+}- In the shutdown hook block (~lines 611–626), replace
context.Background()with the provider:
- ctx := context.Background()
+ ctx := app.serviceCtx()
if err := app.startServices(ctx); err != nil {
log.Warnf("failed to start services: %v", err)
}
app.Hooks().OnPostShutdown(func(_ error) error {
- if err := app.shutdownServices(ctx); err != nil {
+ if err := app.shutdownServices(app.serviceCtx()); err != nil {
log.Warnf("failed to shutdown services: %v", err)
}
return nil
})- Add unit tests (in
services_test.go) verifying thatWithServiceContextis honored during both start and shutdown.
This makes the context fully pluggable while preserving the default behavior.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // If the app is configured to use services, | |
| // register a post shutdown hook to shutdown them after the server is closed. | |
| if app.hasServices() { | |
| // TODO: support for custom context | |
| ctx := context.Background() | |
| if err := app.startServices(ctx); err != nil { | |
| log.Warnf("failed to start services: %v", err) | |
| } | |
| app.Hooks().OnPostShutdown(func(_ error) error { | |
| if err := app.shutdownServices(ctx); err != nil { | |
| log.Warnf("failed to shutdown services: %v", err) | |
| } | |
| return nil | |
| }) | |
| } | |
| // If the app is configured to use services, | |
| // register a post shutdown hook to shutdown them after the server is closed. | |
| if app.hasServices() { | |
| // TODO: support for custom context | |
| - ctx := context.Background() | |
| + ctx := app.serviceCtx() | |
| if err := app.startServices(ctx); err != nil { | |
| log.Warnf("failed to start services: %v", err) | |
| } | |
| app.Hooks().OnPostShutdown(func(_ error) error { | |
| - if err := app.shutdownServices(ctx); err != nil { | |
| + if err := app.shutdownServices(app.serviceCtx()); err != nil { | |
| log.Warnf("failed to shutdown services: %v", err) | |
| } | |
| return nil | |
| }) | |
| } |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 614-618: app.go#L614-L618
Added lines #L614 - L618 were not covered by tests
[warning] 620-624: app.go#L620-L624
Added lines #L620 - L624 were not covered by tests
| app := &App{ | ||
| configured: Config{ | ||
| Services: services, | ||
| }, | ||
| } | ||
|
|
||
| b.ResetTimer() | ||
| for i := 0; i < b.N; i++ { | ||
| ctx := context.Background() | ||
| if err := app.startServices(ctx); err != nil { | ||
| b.Fatal("Expected no error but got", err) | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Repeatedly starting the same service instances skews the benchmark results
app and its services slice are created once and then reused for every loop iteration.
After the first Start call all mockServices are already in the started state, so subsequent iterations do almost no work and massively under-report real start-up cost. In addition, real implementations may treat a second Start as an error and the benchmark would silently ignore that.
Refactor so that each iteration sees fresh instances or at least shuts them down before the next run:
- b.ResetTimer()
- for i := 0; i < b.N; i++ {
- ctx := context.Background()
- if err := app.startServices(ctx); err != nil {
- b.Fatal("Expected no error but got", err)
- }
- }
+ for i := 0; i < b.N; i++ {
+ app := &App{configured: Config{Services: cloneServices(services)}} // helper that returns new instances
+ b.StartTimer()
+ if err := app.startServices(context.Background()); err != nil {
+ b.Fatal("start error:", err)
+ }
+ b.StopTimer()
+ _ = app.shutdownServices(context.Background()) // keep the environment clean
+ }(If you only want to measure Start, wrap only the startServices call between StartTimer/StopTimer.)
Committable suggestion skipped: line range outside the PR's diff.
services.go
Outdated
| func (app *App) startServices(ctx context.Context) error { | ||
| if app.hasServices() { | ||
| var errs []error | ||
| for _, dep := range app.configured.Services { | ||
| if err := ctx.Err(); err != nil { | ||
| // Context is canceled, return an error the soonest possible, so that | ||
| // the user can see the context cancellation error and act on it. | ||
| return fmt.Errorf("context canceled while starting services: %w", err) | ||
| } | ||
|
|
||
| err := dep.Start(ctx) | ||
| if err != nil { | ||
| if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { | ||
| return fmt.Errorf("service %s start: %w", dep.String(), err) | ||
| } | ||
| errs = append(errs, fmt.Errorf("service %s start: %w", dep.String(), err)) | ||
| } | ||
| } | ||
| return errors.Join(errs...) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Started services are left running when a later start fails
If dep.Start fails for service n, the already-started services 0…n-1 remain running, potentially leaking resources.
Consider rolling back by terminating any service that was started successfully before returning the error:
for idx, dep := range app.configured.Services {
...
if err != nil {
- if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) {
- return fmt.Errorf("service %s start: %w", dep.String(), err)
- }
- errs = append(errs, fmt.Errorf("service %s start: %w", dep.String(), err))
+ // best effort rollback of previously started deps
+ for i := idx - 1; i >= 0; i-- {
+ _ = app.configured.Services[i].Terminate(ctx)
+ }
+ return fmt.Errorf("service %s start: %w", dep.String(), err)
}
}Committable suggestion skipped: line range outside the PR's diff.
app.go
Outdated
| // TODO: support for custom context | ||
| ctx := context.Background() | ||
| if err := app.startServices(ctx); err != nil { | ||
| log.Warnf("failed to start services: %v", err) |
There was a problem hiding this comment.
I did not consider panicking here because of the dev/test mode mindset, but it's probably fine: it the user defines services, they are expected to be healthy
Will change it asap
There was a problem hiding this comment.
Mmm, if we panic in the middle of the startup of the services, would the postShutdownHook be called? If not, then we can leak services in the running state. I'm going to change this to start/add-to-shutdown dependency by dependency
docs/api/services.md
Outdated
|
|
||
| ``` | ||
|
|
||
| ### Example: Adding a service with State Management |
There was a problem hiding this comment.
Not needed, but it would be cool if the example used the gofiber/storage driver + testcontainers. It would make the example so much shorter since the driver already does most of this
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (9)
docs/api/services.md (4)
64-65: 🛠️ Refactor suggestionFix interface method documentation style
According to the retrieved learnings from this project, interface types should not be used as method receivers in documentation examples.
Based on project conventions, revise method signatures to either:
-func (d *Service) Terminate(ctx context.Context) error +// Use one of these alternatives: +Terminate(ctx context.Context) error +// OR +func (s *concreteImplementation) Terminate(ctx context.Context) error
39-41: 🛠️ Refactor suggestionFix interface method documentation style
According to the retrieved learnings from this project, interface types should not be used as method receivers in documentation examples.
Based on project conventions, revise method signatures to either:
-func (d *Service) Start(ctx context.Context) error +// Use one of these alternatives: +Start(ctx context.Context) error +// OR +func (s *concreteImplementation) Start(ctx context.Context) error
56-57: 🛠️ Refactor suggestionFix interface method documentation style
According to the retrieved learnings from this project, interface types should not be used as method receivers in documentation examples.
Based on project conventions, revise method signatures to either:
-func (d *Service) State(ctx context.Context) (string, error) +// Use one of these alternatives: +State(ctx context.Context) (string, error) +// OR +func (s *concreteImplementation) State(ctx context.Context) (string, error)
48-49: 🛠️ Refactor suggestionFix interface method documentation style
According to the retrieved learnings from this project, interface types should not be used as method receivers in documentation examples.
Based on project conventions, revise method signatures to either:
-func (d *Service) String() string +// Use one of these alternatives: +String() string +// OR +func (s *concreteImplementation) String() stringservices_benchmark_test.go (3)
55-99:⚠️ Potential issueBenchmark doesn't start services before shutting them down
The shutdown benchmark calls
shutdownServices()on services that were never started. This doesn't reflect real-world usage and may skew performance metrics.Refactor the benchmark to start services before shutting them down:
- app := &App{ - configured: Config{ - Services: services, - }, - } - - b.ResetTimer() - for i := 0; i < b.N; i++ { - ctx := context.Background() - if err := app.shutdownServices(ctx); err != nil { - b.Fatal("Expected no error but got", err) - } - } + ctx := context.Background() + + b.ResetTimer() + for i := 0; i < b.N; i++ { + // Create a fresh App instance for each iteration + app := &App{ + configured: Config{ + Services: createFreshServices(services), // Helper to clone services + }, + } + + // Setup: Start services (not part of benchmark timing) + b.StopTimer() + if err := app.startServices(ctx); err != nil { + b.Fatal("Failed to start services:", err) + } + b.StartTimer() + + // Benchmark the shutdown operation + if err := app.shutdownServices(ctx); err != nil { + b.Fatal("Expected no error but got", err) + } + }
9-53:⚠️ Potential issueBenchmark reuses started services between iterations
The benchmark repeatedly calls
startServices()on the sameAppinstance without resetting service state or callingshutdownServices()between iterations. After the first iteration, services are already in the "started" state, which skews results and doesn't represent real-world usage.Refactor the benchmark to ensure each iteration measures a full service start sequence:
- app := &App{ - configured: Config{ - Services: services, - }, - } - - b.ResetTimer() - for i := 0; i < b.N; i++ { - ctx := context.Background() - if err := app.startServices(ctx); err != nil { - b.Fatal("Expected no error but got", err) - } - } + b.ResetTimer() + for i := 0; i < b.N; i++ { + // Create a fresh App instance for each iteration + app := &App{ + configured: Config{ + Services: createFreshServices(services), // Helper to clone services + }, + } + + ctx := context.Background() + if err := app.startServices(ctx); err != nil { + b.Fatal("Expected no error but got", err) + } + + // Clean up after each iteration + _ = app.shutdownServices(ctx) + }Alternatively, use
b.StopTimer()andb.StartTimer()to exclude cleanup operations from the measurement.
116-117: 🛠️ Refactor suggestionImprove error condition check in context cancellation test
The current condition checks for errors only when timeout < 1 second, but with testing delays of 100ms and service delays of 10-30ms, the condition may not behave as expected.
Consider a more precise condition that's tied to the actual service delay:
- if err == nil && timeout < time.Second { + // We expect an error when timeout is shorter than the service's delay + if err == nil && timeout < services[0].(*mockService).startDelay {This will more accurately reflect the expected behavior based on the specific test parameters.
services.go (2)
27-30: Grammar in function commentThe function comment says "if there are any services" but should be "if there are any services for the current application".
-// hasServices Checks if there are any services for the current application. +// hasServices checks if there are any services configured for the current application.
52-80:⚠️ Potential issueStarted services are left running when a later start fails
If
dep.Startfails for service n, the already-started services 0…n-1 remain running, potentially leaking resources.
Consider rolling back by terminating any service that was started successfully before returning the error.func (app *App) startServices(ctx context.Context) error { if app.hasServices() { var errs []error for _, dep := range app.configured.Services { if err := ctx.Err(); err != nil { // Context is canceled, return an error the soonest possible, so that // the user can see the context cancellation error and act on it. return fmt.Errorf("context canceled while starting service %s: %w", dep.String(), err) } err := dep.Start(ctx) if err == nil { // mark the service as started app.startedServices = append(app.startedServices, dep) continue } if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { + // best effort rollback of previously started services + for _, startedDep := range app.startedServices { + _ = startedDep.Terminate(ctx) + } + app.startedServices = nil return fmt.Errorf("service %s start: %w", dep.String(), err) } errs = append(errs, fmt.Errorf("service %s start: %w", dep.String(), err)) } + if len(errs) > 0 { + // best effort rollback of previously started services + for _, startedDep := range app.startedServices { + _ = startedDep.Terminate(ctx) + } + app.startedServices = nil + } return errors.Join(errs...) } return nil }
🧹 Nitpick comments (4)
app.go (1)
630-635: Improve error handling for service shutdownErrors during service shutdown are logged but not propagated or handled in any other way. This could lead to silent failures during application termination.
Consider extending the shutdown hooks mechanism to better handle errors:
- if err := app.shutdownServices(app.servicesShutdownCtx()); err != nil { - log.Errorf("failed to shutdown services: %v", err) - } - return nil + err := app.shutdownServices(app.servicesShutdownCtx()) + if err != nil { + log.Errorf("failed to shutdown services: %v", err) + // If desired, return the error to allow hook chain to handle it + // return err + } + return nilAlso add test coverage for this error path.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 630-634: app.go#L630-L634
Added lines #L630 - L634 were not covered by testsservices_test.go (2)
190-190: Use require.Empty instead of require.Len for zero-length checksFor better readability and more helpful test failure messages, use
require.Emptywhen checking if a collection has no elements.- require.Len(t, app.startedServices, 0) + require.Empty(t, app.startedServices)🧰 Tools
🪛 GitHub Check: lint
[failure] 190-190:
empty: use require.Empty (testifylint)
210-210: Use require.Empty instead of require.Len for zero-length checksFor better readability and more helpful test failure messages, use
require.Emptywhen checking if a collection has no elements.- require.Len(t, app.startedServices, 0) + require.Empty(t, app.startedServices)🧰 Tools
🪛 GitHub Check: lint
[failure] 210-210:
empty: use require.Empty (testifylint)services.go (1)
105-125: Good service status logging with color supportThe logging implementation provides clear visual status information with appropriate use of colors to highlight different states. The emoji adds a nice touch to the output.
Consider extracting the service emoji to a constant for consistency and easier customization:
+const ( + // serviceEmoji is the emoji used for service status in logs + serviceEmoji = "🥡" +) func (app *App) logServices(ctx context.Context, out io.Writer, colors Colors) { // ... - fmt.Fprintf(out, "%sINFO%s 🥡 %s[ %s ] %s%s\n", colors.Green, colors.Reset, stateColor, strings.ToUpper(state), dep.String(), colors.Reset) + fmt.Fprintf(out, "%sINFO%s %s %s[ %s ] %s%s\n", colors.Green, colors.Reset, serviceEmoji, stateColor, strings.ToUpper(state), dep.String(), colors.Reset) // ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app.go(3 hunks)docs/api/services.md(1 hunks)docs/whats_new.md(2 hunks)services.go(1 hunks)services_benchmark_test.go(1 hunks)services_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/whats_new.md
🧰 Additional context used
🧠 Learnings (1)
docs/api/services.md (1)
Learnt from: mdelapenya
PR: gofiber/fiber#3434
File: docs/api/services.md:39-43
Timestamp: 2025-05-07T13:07:33.873Z
Learning: When documenting Go interface methods in the Fiber project, avoid showing method signatures with the interface type as the receiver (e.g., `func (d *Service) Method()`) since interfaces cannot be used as receivers in Go. Instead, show just the method signature without a receiver or use a placeholder implementation name.
🧬 Code Graph Analysis (1)
services.go (3)
state.go (1)
State(9-11)app.go (1)
App(90-136)color.go (1)
Colors(8-53)
🪛 GitHub Check: lint
services_test.go
[failure] 210-210:
empty: use require.Empty (testifylint)
[failure] 190-190:
empty: use require.Empty (testifylint)
app.go
[failure] 429-429:
found a struct that contains a context.Context field (containedctx)
[failure] 424-424:
found a struct that contains a context.Context field (containedctx)
🪛 GitHub Check: codecov/patch
services.go
[warning] 34-37: services.go#L34-L37
Added lines #L34 - L37 were not covered by tests
[warning] 39-39: services.go#L39
Added line #L39 was not covered by tests
[warning] 44-47: services.go#L44-L47
Added lines #L44 - L47 were not covered by tests
[warning] 49-49: services.go#L49
Added line #L49 was not covered by tests
app.go
[warning] 626-627: app.go#L626-L627
Added lines #L626 - L627 were not covered by tests
[warning] 630-634: app.go#L630-L634
Added lines #L630 - L634 were not covered by tests
🪛 GitHub Actions: golangci-lint
app.go
[error] 90-90: golangci-lint fieldalignment: struct with 1424 pointer bytes could be 1376 (govet)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: unit (1.23.x, macos-13)
- GitHub Check: unit (1.24.x, ubuntu-latest)
- GitHub Check: unit (1.24.x, macos-13)
- GitHub Check: unit (1.24.x, windows-latest)
- GitHub Check: unit (1.23.x, windows-latest)
- GitHub Check: repeated
- GitHub Check: Compare
🔇 Additional comments (11)
app.go (1)
626-628:❓ Verification inconclusive
Add test coverage for service startup and panic paths
The static analysis indicates these lines aren't covered by tests, which means the panic path is untested. This is a critical part of the application lifecycle that should be fully verified.
🏁 Script executed:
#!/bin/bash # Check if there are any tests that cover the service panic path rg -n "app\.startServices.*panic" *_test.go # Check for any tests related to service startup in app.go rg -n "New.*Services" *_test.goLength of output: 649
Verify test coverage for service startup and panic path
I ran several searches against the test suite but found no coverage for
app.startServicesor its panic path:
- No occurrences of
startServicesin any*_test.go- No panic assertions (e.g.
panic,Panics) in the testsPlease add explicit tests to cover both the happy-path and failure (panic) scenarios in
app.startServices. For example, inservices_test.go:func TestStartServices_PanicsOnError(t *testing.T) { // Arrange: inject a failing service into app.services app := NewApp(/* ... */) app.services = []Service{&failingService{}} // Act & Assert require.Panics(t, func() { _ = app.startServices(context.Background()) }) }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 626-627: app.go#L626-L627
Added lines #L626 - L627 were not covered by testsservices_test.go (3)
111-126: Well-structured test suite for the service functionalityThe test suite provides good coverage of the service functionality, including both positive and negative scenarios.
20-81: Well-implemented mock service for comprehensive testingThe
mockServiceimplementation effectively simulates real-world service behavior including:
- Context-aware operations
- Configurable delays
- Error conditions
- State tracking
This provides a robust foundation for testing the service lifecycle management.
174-212: Thorough context cancellation testingThe tests for context cancellation scenarios are comprehensive, covering both already-canceled contexts and contexts that expire during operation. This is crucial for validating the system's resilience to external cancellation signals.
🧰 Tools
🪛 GitHub Check: lint
[failure] 210-210:
empty: use require.Empty (testifylint)
[failure] 190-190:
empty: use require.Empty (testifylint)docs/api/services.md (2)
73-175: Well-structured and comprehensive examplesThe examples effectively demonstrate:
- Service implementation with the Redis container
- Integration with Fiber's configuration
- Context management for service lifecycle
- Error handling
This provides a clear guide for users implementing their own services.
153-157:⚠️ Potential issueAdd error handling for connection string failure
In the example, if getting the connection string fails, the function logs the error but continues execution, which will lead to nil pointer dereference when the Redis client is initialized.
Add proper error handling to exit the function on connection string failure:
connString, err := redisSrv.ctr.ConnectionString(ctx) if err != nil { log.Printf("Could not get connection string: %v", err) - return + return // Missing this return statement }Likely an incorrect or invalid review comment.
services_benchmark_test.go (1)
221-261: Well-designed memory allocation benchmarkThis benchmark effectively measures memory allocations for the full service lifecycle. It properly includes both start and shutdown operations in each iteration and reports allocations.
services.go (4)
11-25: Well-designed Service interfaceThe Service interface has a clear and focused design with four essential lifecycle methods. The documentation for each method is informative and explains the purpose well.
82-103: Appropriate error handling during service shutdownThe implementation correctly continues to terminate all services even when errors occur or the context is canceled, which is appropriate for a cleanup operation.
1-9: LGTM: Package and imports are appropriateThe package declaration and imports look good. The imports are minimal and appropriate for the functionality being implemented.
55-78:❌ Incorrect review comment
Consider potential race conditions when modifying startedServices
The
startedServicesslice is modified instartServiceswithout any synchronization. If this method could be called concurrently, consider adding mutex protection for this field.
🏁 Script executed:
#!/bin/bash # Check if startServices could be called concurrently rg -A 5 "startServices|app.startServices" --type go # Look for any concurrent use patterns rg "go .*startServices" --type goLength of output: 3602
No concurrent calls to startServices detected
The
startServicesmethod is only ever invoked synchronously—inapp.goduring startup and in your tests/benchmarks—without anygo app.startServicesusage. Therefore there’s no concurrent mutation ofapp.startedServices, and adding a mutex isn’t necessary.• app.go: called once on startup in the main goroutine
• services_test.go / services_benchmark_test.go: invoked sequentially in testsLikely an incorrect or invalid review comment.
| if err == nil && timeout < time.Second { | ||
| b.Fatal("Expected error due to context cancellation but got none") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error condition check in context cancellation test
Similar to the previous comment, this condition may not behave as expected with the current test parameters.
Consider a more precise condition:
- if err == nil && timeout < time.Second {
+ // We expect an error when timeout is shorter than the service's delay
+ if err == nil && timeout < services[0].(*mockService).terminateDelay {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if err == nil && timeout < time.Second { | |
| b.Fatal("Expected error due to context cancellation but got none") | |
| // We expect an error when timeout is shorter than the service's delay | |
| if err == nil && timeout < services[0].(*mockService).terminateDelay { | |
| b.Fatal("Expected error due to context cancellation but got none") |
services.go
Outdated
| // servicesStartupCtx Returns the context for the services startup. | ||
| // If the ServicesStartupCtx is not set, it returns a new background context. | ||
| func (app *App) servicesStartupCtx() context.Context { | ||
| if app.config.ServicesStartupCtx != nil { | ||
| return app.config.ServicesStartupCtx | ||
| } | ||
|
|
||
| return context.Background() | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Missing test coverage for context methods
The servicesStartupCtx and servicesShutdownCtx methods are not covered by tests according to code coverage analysis. Consider adding tests that verify both the default (background context) and custom context scenarios.
Also applies to: 42-50
🏁 Script executed:
#!/bin/bash
# Verify test coverage for context methods
rg -A 2 "Test.*services(Startup|Shutdown)Ctx" --type goLength of output: 57
🏁 Script executed:
#!/bin/bash
# Search for any references to servicesStartupCtx in test files
rg -n "servicesStartupCtx" -g "*_test.go"
# Search for any references to servicesShutdownCtx in test files
rg -n "servicesShutdownCtx" -g "*_test.go"
# Broad search for StartupCtx and ShutdownCtx in test files
rg -n "StartupCtx" -g "*_test.go"
rg -n "ShutdownCtx" -g "*_test.go"Length of output: 153
Add missing tests for servicesStartupCtx and servicesShutdownCtx
Both context helper methods in services.go lack any test coverage. Please add table-driven tests covering:
- servicesStartupCtx (lines 32–40):
- when app.config.ServicesStartupCtx is non-nil (returns that context)
- when it’s nil (returns context.Background())
- servicesShutdownCtx (lines 42–50):
- when app.config.ServicesShutdownCtx is non-nil
- when it’s nil
Place these in a new or existing *_test.go and assert that the returned context matches the expected one.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 34-37: services.go#L34-L37
Added lines #L34 - L37 were not covered by tests
[warning] 39-39: services.go#L39
Added line #L39 was not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (6)
app.go (1)
623-628: 🛠️ Refactor suggestionConsider graceful degradation instead of panic for service startup failures.
For development and test-time dependencies, a panic on failure could be disruptive to developers. A more graceful approach might be to log the error and continue, letting the application function with reduced capabilities.
if app.hasServices() { - if err := app.startServices(app.servicesStartupCtx()); err != nil { - panic(err) - } + if err := app.startServices(app.servicesStartupCtx()); err != nil { + log.Errorf("failed to start services: %v", err) + // Application continues with failed services + }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 626-627: app.go#L626-L627
Added lines #L626 - L627 were not covered by testsdocs/api/services.md (4)
39-41: 🛠️ Refactor suggestionCorrect the method signature format.
Interfaces cannot be used as method receivers in Go. Based on the retrieved learning, we should avoid showing method signatures with the interface type as the receiver.
-func (d *Service) Start(ctx context.Context) error +Start(ctx context.Context) error
64-65: 🛠️ Refactor suggestionCorrect the method signature format.
This method signature also needs correction.
-func (d *Service) Terminate(ctx context.Context) error +Terminate(ctx context.Context) error
48-49: 🛠️ Refactor suggestionCorrect the method signature format.
Similarly, this method signature also incorrectly uses the interface as receiver.
-func (d *Service) String() string +String() string
56-57: 🛠️ Refactor suggestionCorrect the method signature format.
This method signature also needs correction.
-func (d *Service) State(ctx context.Context) (string, error) +State(ctx context.Context) (string, error)services.go (1)
54-80: 🛠️ Refactor suggestionAdd rollback for started services when a later service fails.
If a service fails to start, any services that were already started will remain running, potentially leaking resources.
Add a rollback mechanism to terminate already-started services when a later service fails:
func (app *App) startServices(ctx context.Context) error { if app.hasServices() { var errs []error for idx, dep := range app.configured.Services { if err := ctx.Err(); err != nil { // Context is canceled, return an error the soonest possible, so that // the user can see the context cancellation error and act on it. return fmt.Errorf("context canceled while starting service %s: %w", dep.String(), err) } err := dep.Start(ctx) if err == nil { // mark the service as started app.startedServices = append(app.startedServices, dep) continue } if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { + // Rollback: terminate any services that were started + for _, startedDep := range app.startedServices { + _ = startedDep.Terminate(ctx) // Best effort + } + app.startedServices = nil return fmt.Errorf("service %s start: %w", dep.String(), err) } errs = append(errs, fmt.Errorf("service %s start: %w", dep.String(), err)) } + if len(errs) > 0 { + // Rollback: terminate any services that were started + for _, startedDep := range app.startedServices { + _ = startedDep.Terminate(ctx) // Best effort + } + app.startedServices = nil + } return errors.Join(errs...) } return nil }
🧹 Nitpick comments (5)
app.go (1)
134-135: Consider field alignment for better memory efficiency.The
Appstruct has a golangci-lint warning about field alignment. Grouping fields of similar size together can reduce memory consumption.You could optimize the struct layout by grouping the pointer fields together (like
startedServices []Service) and scalar fields together. While not critical for functionality, it's a performance best practice for Go structs.docs/api/services.md (2)
7-8: Simplify the introductory sentence.The current text is a bit redundant with "while developing and testing the application".
-Services provide external services needed to run the application. They are supposed to be used while developing and testing the application. +Services provide external dependencies needed to run the application during development and testing.
158-161: Add explicit service initialization to the example.The example could benefit from showing explicit service initialization to avoid potential confusion.
Consider adding a step to explicitly initialize the service:
// Ensure the service is already started (happens automatically when app is created) if !redisSrv.started { err := redisSrv.Start(ctx) if err != nil { log.Fatalf("Failed to start Redis service: %v", err) } }This would make it clearer to users that services must be properly initialized before accessing their resources.
services.go (1)
32-40: Potential inconsistency in context provider check.The function checks
app.configured.ServicesStartupContextProviderbut then usesapp.config.ServicesStartupContextProvider().func (app *App) servicesStartupCtx() context.Context { - if app.configured.ServicesStartupContextProvider != nil { + if app.config.ServicesStartupContextProvider != nil { return app.config.ServicesStartupContextProvider() } return context.Background() }This ensures you're checking the same struct that you're using for the provider.
services_test.go (1)
138-139: Use require.Empty instead of require.Len with 0.The linter suggests using require.Empty for checking empty slices/maps.
- require.Empty(t, app.startedServices) + require.Empty(t, app.startedServices)This is already correct, just noting the linter's approval.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app.go(3 hunks)docs/api/services.md(1 hunks)services.go(1 hunks)services_test.go(1 hunks)
🧰 Additional context used<
10000
/summary>
🧠 Learnings (1)
docs/api/services.md (1)
Learnt from: mdelapenya
PR: gofiber/fiber#3434
File: docs/api/services.md:39-43
Timestamp: 2025-05-07T13:07:33.873Z
Learning: When documenting Go interface methods in the Fiber project, avoid showing method signatures with the interface type as the receiver (e.g., `func (d *Service) Method()`) since interfaces cannot be used as receivers in Go. Instead, show just the method signature without a receiver or use a placeholder implementation name.
🧬 Code Graph Analysis (2)
services.go (3)
state.go (1)
State (9-11)
app.go (1)
App (90-136)
color.go (1)
Colors (8-53)
app.go (3)
services.go (1)
Service (12-25)
hooks.go (1)
Hooks (21-35)
log/fiberlog.go (1)
Errorf (49-51)
🪛 GitHub Check: lint
services_test.go
[failure] 210-210:
empty: use require.Empty (testifylint)
[failure] 190-190:
empty: use require.Empty (testifylint)
🪛 GitHub Check: codecov/patch
app.go
[warning] 626-627: app.go#L626-L627
Added lines #L626 - L627 were not covered by tests
[warning] 630-634: app.go#L630-L634
Added lines #L630 - L634 were not covered by tests
🪛 GitHub Actions: golangci-lint
app.go
[error] 90-90: golangci-lint: fieldalignment: struct with 1392 pointer bytes could be 1344 (govet)
🪛 markdownlint-cli2 (0.17.2)
docs/api/services.md
135-135: Hard tabs
Column: 1
(MD010, no-hard-tabs)
136-136: Hard tabs
Column: 1
(MD010, no-hard-tabs)
137-137: Hard tabs
Column: 1
(MD010, no-hard-tabs)
138-138: Hard tabs
Column: 1
(MD010, no-hard-tabs)
139-139: Hard tabs
Column: 1
(MD010, no-hard-tabs)
140-140: Hard tabs
Column: 1
(MD010, no-hard-tabs)
141-141: Hard tabs
Column: 1
(MD010, no-hard-tabs)
143-143: Hard tabs
Column: 1
(MD010, no-hard-tabs)
144-144: Hard tabs
Column: 1
(MD010, no-hard-tabs)
145-145: Hard tabs
Column: 1
(MD010, no-hard-tabs)
146-146: Hard tabs
Column: 1
(MD010, no-hard-tabs)
147-147: Hard tabs
Column: 1
(MD010, no-hard-tabs)
148-148: Hard tabs
Column: 1
(MD010, no-hard-tabs)
149-149: Hard tabs
Column: 1
(MD010, no-hard-tabs)
150-150: Hard tabs
Column: 1
(MD010, no-hard-tabs)
256-256: Hard tabs
Column: 1
(MD010, no-hard-tabs)
257-257: Hard tabs
Column: 1
(MD010, no-hard-tabs)
258-258: Hard tabs
Column: 1
(MD010, no-hard-tabs)
259-259: Hard tabs
Column: 1
(MD010, no-hard-tabs)
260-260: Hard tabs
Column: 1
(MD010, no-hard-tabs)
261-261: Hard tabs
Column: 1
(MD010, no-hard-tabs)
262-262: Hard tabs
Column: 1
(MD010, no-hard-tabs)
264-264: Hard tabs
Column: 1
(MD010, no-hard-tabs)
265-265: Hard tabs
Column: 1
(MD010, no-hard-tabs)
266-266: Hard tabs
Column: 1
(MD010, no-hard-tabs)
267-267: Hard tabs
Column: 1
(MD010, no-hard-tabs)
268-268: Hard tabs
Column: 1
(MD010, no-hard-tabs)
269-269: Hard tabs
Column: 1
(MD010, no-hard-tabs)
270-270: Hard tabs
Column: 1
(MD010, no-hard-tabs)
271-271: Hard tabs
Column: 1
(MD010, no-hard-tabs)
🪛 GitHub Check: markdownlint
docs/api/services.md
[failure] 145-145: Hard tabs
docs/api/services.md:145:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
[failure] 144-144: Hard tabs
docs/api/services.md:144:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
[failure] 143-143: Hard tabs
docs/api/services.md:143:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
[failure] 141-141: Hard tabs
docs/api/services.md:141:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
[failure] 140-140: Hard tabs
docs/api/services.md:140:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
[failure] 139-139: Hard tabs
docs/api/services.md:139:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
[failure] 138-138: Hard tabs
docs/api/services.md:138:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
[failure] 137-137: Hard tabs
docs/api/services.md:137:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
[failure] 136-136: Hard tabs
docs/api/services.md:136:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
[failure] 135-135: Hard tabs
docs/api/services.md:135:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
🪛 GitHub Actions: markdownlint
docs/api/services.md
[error] 135-135: markdownlint MD010/no-hard-tabs: Hard tabs found at column 1.
🧠 Learnings (1)
docs/api/services.md (1)
Learnt from: mdelapenya
PR: gofiber/fiber#3434
File: docs/api/services.md:39-43
Timestamp: 2025-05-07T13:07:33.873Z
Learning: When documenting Go interface methods in the Fiber project, avoid showing method signatures with the interface type as the receiver (e.g., `func (d *Service) Method()`) since interfaces cannot be used as receivers in Go. Instead, show just the method signature without a receiver or use a placeholder implementation name.
🧬 Code Graph Analysis (2)
services.go (3)
state.go (1)
State(9-11)app.go (1)
App(90-136)color.go (1)
Colors(8-53)
app.go (3)
services.go (1)
Service(12-25)hooks.go (1)
Hooks(21-35)log/fiberlog.go (1)
Errorf(49-51)
🪛 GitHub Check: lint
services_test.go
[failure] 210-210:
empty: use require.Empty (testifylint)
[failure] 190-190:
empty: use require.Empty (testifylint)
🪛 GitHub Check: codecov/patch
app.go
[warning] 626-627: app.go#L626-L627
Added lines #L626 - L627 were not covered by tests
[warning] 630-634: app.go#L630-L634
Added lines #L630 - L634 were not covered by tests
🪛 GitHub Actions: golangci-lint
app.go
[error] 90-90: golangci-lint: fieldalignment: struct with 1392 pointer bytes could be 1344 (govet)
🪛 markdownlint-cli2 (0.17.2)
docs/api/services.md
135-135: Hard tabs
Column: 1
(MD010, no-hard-tabs)
136-136: Hard tabs
Column: 1
(MD010, no-hard-tabs)
137-137: Hard tabs
Column: 1
(MD010, no-hard-tabs)
138-138: Hard tabs
Column: 1
(MD010, no-hard-tabs)
139-139: Hard tabs
Column: 1
(MD010, no-hard-tabs)
140-140: Hard tabs
Column: 1
(MD010, no-hard-tabs)
141-141: Hard tabs
Column: 1
(MD010, no-hard-tabs)
143-143: Hard tabs
Column: 1
(MD010, no-hard-tabs)
144-144: Hard tabs
Column: 1
(MD010, no-hard-tabs)
145-145: Hard tabs
Column: 1
(MD010, no-hard-tabs)
146-146: Hard tabs
Column: 1
(MD010, no-hard-tabs)
147-147: Hard tabs
Column: 1
(MD010, no-hard-tabs)
148-148: Hard tabs
Column: 1
(MD010, no-hard-tabs)
149-149: Hard tabs
Column: 1
(MD010, no-hard-tabs)
150-150: Hard tabs
Column: 1
(MD010, no-hard-tabs)
256-256: Hard tabs
Column: 1
(MD010, no-hard-tabs)
257-257: Hard tabs
Column: 1
(MD010, no-hard-tabs)
258-258: Hard tabs
Column: 1
(MD010, no-hard-tabs)
259-259: Hard tabs
Column: 1
(MD010, no-hard-tabs)
260-260: Hard tabs
Column: 1
(MD010, no-hard-tabs)
261-261: Hard tabs
Column: 1
(MD010, no-hard-tabs)
262-262: Hard tabs
Column: 1
(MD010, no-hard-tabs)
264-264: Hard tabs
Column: 1
(MD010, no-hard-tabs)
265-265: Hard tabs
Column: 1
(MD010, no-hard-tabs)
266-266: Hard tabs
Column: 1
(MD010, no-hard-tabs)
267-267: Hard tabs
Column: 1
(MD010, no-hard-tabs)
268-268: Hard tabs
Column: 1
(MD010, no-hard-tabs)
269-269: Hard tabs
Column: 1
(MD010, no-hard-tabs)
270-270: Hard tabs
Column: 1
(MD010, no-hard-tabs)
271-271: Hard tabs
Column: 1
(MD010, no-hard-tabs)
🪛 GitHub Check: markdownlint
docs/api/services.md
[failure] 145-145: Hard tabs
docs/api/services.md:145:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
[failure] 144-144: Hard tabs
docs/api/services.md:144:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
[failure] 143-143: Hard tabs
docs/api/services.md:143:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
[failure] 141-141: Hard tabs
docs/api/services.md:141:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
[failure] 140-140: Hard tabs
docs/api/services.md:140:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
[failure] 139-139: Hard tabs
docs/api/services.md:139:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
[failure] 138-138: Hard tabs
docs/api/services.md:138:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
[failure] 137-137: Hard tabs
docs/api/services.md:137:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
[failure] 136-136: Hard tabs
docs/api/services.md:136:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
[failure] 135-135: Hard tabs
docs/api/services.md:135:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md010.md
🪛 GitHub Actions: markdownlint
docs/api/services.md
[error] 135-135: markdownlint MD010/no-hard-tabs: Hard tabs found at column 1.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: unit (1.24.x, macos-13)
- GitHub Check: unit (1.23.x, windows-latest)
- GitHub Check: unit (1.24.x, macos-latest)
- GitHub Check: unit (1.24.x, windows-latest)
- GitHub Check: Compare
- GitHub Check: repeated
🔇 Additional comments (8)
app.go (1)
416-430: Design choice: Using function providers instead of contexts directly is good practice.Using context provider functions rather than direct context objects in the Config struct aligns with Go best practices. Contexts should be passed as function parameters rather than stored in structs.
services.go (3)
11-25: Well-designed interface with clear method purposes.The Service interface is well-designed with clear methods for the service lifecycle. The documentation comments are informative.
84-103: Well-implemented shutdown with best-effort error handling.The shutdown logic correctly continues attempting to terminate all services even if one fails or the context is canceled. This is a good practice for cleanup operations.
105-125: Good service state logging implementation with color support.The logging function correctly handles error cases and provides a well-formatted output with color coding based on service state.
services_test.go (4)
15-30: Well-designed mock implementation for testing.The mockService struct provides a flexible mock implementation with configurable errors, delays, and state tracking, which is excellent for thorough testing.
174-213: Excellent context cancellation testing.The tests for context cancellation in startServices are well-implemented, covering both already-canceled contexts and cancellation during execution.
🧰 Tools
🪛 GitHub Check: lint
[failure] 210-210:
empty: use require.Empty (testifylint)
[failure] 190-190:
empty: use require.Empty (testifylint)
252-287: Thorough shutdown testing with context handling.The shutdown tests comprehensively cover error cases and context cancellation patterns, ensuring robust service termination.
335-356: Good coverage of context provider functionality.The tests for service context providers verify both default behavior (background context) and custom contexts, ensuring the flexibility works as expected.
| app.Hooks().OnPostShutdown(func(_ error) error { | ||
| if err := app.shutdownServices(app.servicesShutdownCtx()); err != nil { | ||
| log.Errorf("failed to shutdown services: %v", err) | ||
| } | ||
| return nil | ||
| }) | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Add test coverage for the service shutdown hook.
The static analysis indicates that the post-shutdown hook is not covered by tests. This is a critical part of the service lifecycle management.
Consider adding tests that verify the post-shutdown hook is called and services are properly terminated. This could be done by extending the existing tests in services_test.go.
🏁 Script executed:
#!/bin/bash
# Check if there are any tests that cover hooks with services shutdown
rg "onPostShutdown.*shutdown.*service" --type goLength of output: 50
#!/bin/bash
# Search for shutdownServices usage in test files
rg "shutdownServices" --type go -n -g "*_test.go"
# Search for OnPostShutdown hook in test files
rg "OnPostShutdown" --type go -n -g "*_test.go"This will confirm whether any existing tests cover the post-shutdown hook before we proceed.
Add test coverage for the PostShutdown hook
We found no existing tests invoking OnPostShutdown or calling shutdownServices. Since this hook is critical to cleanly shutting down all services, please add dedicated tests for both success and error paths. For example, in services_test.go (or a new app_test.go):
- Mock a service whose
Shutdownmethod records whether it was called. - Register that mock with your app and trigger the shutdown hook.
- Verify that:
•shutdownServicesis invoked exactly once.
• WhenShutdownreturns an error, it’s logged.
Suggested test signatures:
func TestPostShutdownHook_ShutdownServices_Success(t *testing.T) { … }
func TestPostShutdownHook_ShutdownServices_Error(t *testing.T) { … }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 630-634: app.go#L630-L634
Added lines #L630 - L634 were not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
docs/api/services.md (3)
39-41:⚠️ Potential issueFix interface method signature documentation.
Interface types can't be used as method receivers in Go.
Based on earlier feedback, replace the method signature with one of these alternatives:
- Just show the method name with parameters:
Start(ctx context.Context) error- Use a placeholder implementation name:
func (s SomeImplementation) Start(ctx context.Context) error< 10000 /li>
48-49:⚠️ Potential issueFix interface method signature documentation.
The method signatures for String, State, and Terminate also incorrectly use the interface type as a receiver.
Use the same correction approach as with the Start method. Replace each with either:
- Just the method name with parameters (e.g.,
String() string)- A placeholder implementation name (e.g.,
func (s SomeImplementation) String() string)Also applies to: 56-57, 64-65
134-150:⚠️ Potential issueFix indentation using spaces instead of tabs.
The markdown linter is flagging hard tabs in these code blocks which is causing pipeline failures.
Replace the hard tabs with spaces for indentation to fix the linting errors. This appears to be an issue throughout both code examples in the markdown file.
Also applies to: 255-271
🧹 Nitpick comments (2)
docs/api/services.md (2)
156-176: Add explicit service initialization check.The example relies on the service being automatically started by the app initialization. For clarity and robustness, consider adding a verification step.
Add a check to ensure the Redis container is started before trying to use it:
// Obtain the connection string from the service. + // The service is automatically started during app initialization, + // but we should verify it's ready before proceeding. + if redisSrv.ctr == nil { + log.Fatalf("Redis service not properly initialized") + } connString, err := redisSrv.ctr.ConnectionString(ctx)
277-282: Add explicit service initialization check to second example.The second example has the same potential issue with service initialization.
Add the same verification check as suggested for the first example.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app.go(4 hunks)docs/api/services.md(1 hunks)services_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- services_test.go
🧰 Additional context used
🧠 Learnings (2)
docs/api/services.md (1)
Learnt from: mdelapenya
PR: gofiber/fiber#3434
File: docs/api/services.md:39-43
Timestamp: 2025-05-07T13:07:33.873Z
Learning: When documenting Go interface methods in the Fiber project, avoid showing method signatures with the interface type as the receiver (e.g., `func (d *Service) Method()`) since interfaces cannot be used as receivers in Go. Instead, show just the method signature without a receiver or use a placeholder implementation name.
app.go (1)
Learnt from: mdelapenya
PR: gofiber/fiber#3434
File: app.go:623-636
Timestamp: 2025-05-08T08:14:37.291Z
Learning: In the gofiber/fiber framework, service startup failures should panic rather than allowing the application to continue running with degraded functionality, as this is the agreed-upon design decision.
🧬 Code Graph Analysis (1)
app.go (3)
services.go (1)
Service(12-25)hooks.go (1)
Hooks(21-35)log/fiberlog.go (1)
Errorf(49-51)
🪛 GitHub Check: codecov/patch
app.go
[warning] 626-627: app.go#L626-L627
Added lines #L626 - L627 were not covered by tests
[warning] 630-634: app.go#L630-L634
Added lines #L630 - L634 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: unit (1.23.x, windows-latest)
- GitHub Check: unit (1.24.x, windows-latest)
- GitHub Check: unit (1.23.x, macos-13)
- GitHub Check: Compare
- GitHub Check: repeated
🔇 Additional comments (10)
app.go (5)
122-122: Good addition of service state tracking.The
startedServicesfield is a well-designed addition to track which services have been started, enabling proper cleanup during application shutdown.
416-419: Well-documented service configuration field.The
Servicesfield is appropriately documented as optional with a clear default value. This allows users to easily configure external dependencies.
421-429: Good approach for customizable service contexts.Using function providers instead of raw contexts in the config is a good design pattern. This approach follows Go's context best practices by avoiding storing context objects directly in structs, while still giving users flexibility to customize behavior.
623-628: Correctly implemented service startup with panic.The implementation correctly detects if services are configured and attempts to start them during application initialization. The panic on service failure aligns with the team's agreed-upon design decision.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 626-627: app.go#L626-L627
Added lines #L626 - L627 were not covered by tests
630-636: Add test coverage for the shutdown hook.The post-shutdown hook registration for service termination is well-implemented, but lacks test coverage according to static analysis.
Consider adding a test that explicitly verifies the post-shutdown hook is properly called and that the services are terminated. You can mock a service, register it with the app, and verify the shutdown sequence.
#!/bin/bash # Check if there are tests that cover the post-shutdown hook with services rg "OnPostShutdown.*service" --type go -g "*_test.go"🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 630-634: app.go#L630-L634
Added lines #L630 - L634 were not covered by testsdocs/api/services.md (5)
7-8: Clear introduction to the Services concept.This introduction clearly explains the purpose of the Services feature, establishing it as a development and testing tool.
15-31: Well-defined Service interface.The interface definition is clear and comprehensive with good documentation for each method.
87-125: Excellent service implementation example.This Redis service implementation is comprehensive and demonstrates all required interface methods correctly. The implementation shows good error handling and proper use of contexts.
284-288: Excellent use ofredisStorefrom Fiber's storage module.Using the storage module integration simplifies the example and demonstrates how to combine the Services feature with existing Fiber components.
289-317: Well-implemented CRUD example.The user creation and retrieval handlers clearly demonstrate how the service can be used within a real application context. The error handling is thorough and follows best practices.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
services.go (1)
52-81:⚠️ Potential issueError handling inconsistency and missing rollback.
The service startup error handling has two issues:
There's an inconsistency in error handling: context errors at service start directly return, while other errors are collected. This suggests different treatment but the end result is the same - startup failure.
As noted in a previous review, when a service fails to start, already started services are not terminated. This could lead to resource leaks.
Consider simplifying the error handling and adding rollback functionality:
func (app *App) startServices(ctx context.Context) error { if !app.hasServices() { return nil } - var errs []error for idx, dep := range app.configured.Services { if err := ctx.Err(); err != nil { // Context is canceled, return an error the soonest possible, so that // the user can see the context cancellation error and act on it. return fmt.Errorf("context canceled while starting service %s: %w", dep.String(), err) } err := dep.Start(ctx) if err == nil { // mark the service as started app.startedServices = append(app.startedServices, dep) continue } - if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { - return fmt.Errorf("service %s start: %w", dep.String(), err) - } - - errs = append(errs, fmt.Errorf("service %s start: %w", dep.String(), err)) + // Rollback: terminate any services that were started successfully + for i := len(app.startedServices) - 1; i >= 0; i-- { + _ = app.startedServices[i].Terminate(ctx) + } + app.startedServices = nil + return fmt.Errorf("service %s start: %w", dep.String(), err) } - return errors.Join(errs...) + return nil }This approach simplifies error handling and ensures proper cleanup when a service fails to start.
🧹 Nitpick comments (2)
services.go (2)
27-30: Function name might be misleading.Consider renaming
hasServices()tohasConfiguredServices()to clarify that it's checking the configured services, not the started ones. This would be more consistent with the method's implementation and use cases.
83-105: Consider updating startedServices slice after termination.The
shutdownServicesmethod doesn't update theapp.startedServicesslice after terminating services. Consider clearing this slice after shutdown to maintain accurate state.func (app *App) shutdownServices(ctx context.Context) error { if len(app.startedServices) == 0 { return nil } var errs []error for _, dep := range app.startedServices { if err := ctx.Err(); err != nil { // Context is canceled, do a best effort to terminate the services. errs = append(errs, fmt.Errorf("service %s terminate: %w", dep.String(), err)) continue } err := dep.Terminate(ctx) if err != nil { // Best effort to terminate the services. errs = append(errs, fmt.Errorf("service %s terminate: %w", dep.String(), err)) } } + // Clear the started services slice after shutdown + app.startedServices = nil return errors.Join(errs...) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
services.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: unit (1.24.x, windows-latest)
- GitHub Check: unit (1.23.x, windows-latest)
- GitHub Check: repeated
- GitHub Check: Compare
🔇 Additional comments (3)
services.go (3)
1-9: LGTM! Good package structure and imports.The imports are well-organized and include only what's necessary for the implementation.
11-25: Well-designed Service interface.The Service interface provides a clean, minimal contract for service lifecycle management with good documentation. The four methods (Start, String, State, Terminate) cover the essential operations needed for managing external dependencies.
107-129: LGTM! Good logging implementation with state checks and color formatting.The logging implementation is thorough, displaying the service count and status for each service with appropriate color coding. The error handling for state retrieval is well-implemented.
| // servicesStartupCtx Returns the context for the services startup. | ||
| // If the ServicesStartupContextProvider is not set, it returns a new background context. | ||
| func (app *App) servicesStartupCtx() context.Context { | ||
| if app.configured.ServicesStartupContextProvider != nil { | ||
| return app.config.ServicesStartupContextProvider() | ||
| } | ||
|
|
||
| return context.Background() | ||
| } |
There was a problem hiding this comment.
Inconsistency in context field reference.
There's an inconsistency in how the context provider is checked vs accessed:
func (app *App) servicesStartupCtx() context.Context {
- if app.configured.ServicesStartupContextProvider != nil {
+ if app.config.ServicesStartupContextProvider != nil {
return app.config.ServicesStartupContextProvider()
}
return context.Background()
}This code appears to be checking app.configured.ServicesStartupContextProvider but then using app.config.ServicesStartupContextProvider(). One of these is likely incorrect.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // servicesStartupCtx Returns the context for the services startup. | |
| // If the ServicesStartupContextProvider is not set, it returns a new background context. | |
| func (app *App) servicesStartupCtx() context.Context { | |
| if app.configured.ServicesStartupContextProvider != nil { | |
| return app.config.ServicesStartupContextProvider() | |
| } | |
| return context.Background() | |
| } | |
| // servicesStartupCtx Returns the context for the services startup. | |
| // If the ServicesStartupContextProvider is not set, it returns a new background context. | |
| func (app *App) servicesStartupCtx() context.Context { | |
| if app.config.ServicesStartupContextProvider != nil { | |
| return app.config.ServicesStartupContextProvider() | |
| } | |
| return context.Background() | |
| } |
| @@ -603,6 +620,21 @@ func New(config ...Config) *App { | |||
| // Override colors | |||
| app.config.ColorScheme = defaultColors(app.config.ColorScheme) | |||
|
|
|||
| // If the app is configured to use services, | |||
| // register a post shutdown hook to shutdown them after the server is closed. | |||
| if app.hasServices() { | |||
There was a problem hiding this comment.
question: do you think that, for avoiding users running undesired services in production, we add here a way to check if running in production? Something like the State example code: https://docs.gofiber.io/next/api/state#example-environmentspecific-configuration
// Determine environment.
environment := os.Getenv("ENV")
if environment == "" {
environment = "development"
}
app.State().Set("environment", environment)
// Set environment-specific configurations.
if environment == "development" {
app.State().Set("apiUrl", "http://localhost:8080/api")
app.State().Set("debug", true)
} else {
app.State().Set("apiUrl", "https://api.production.com")
app.State().Set("debug", false)
}```
Description
This pull request introduces a new feature called Services to Fiber v3, which allows developers to quickly start and manage development/test-time dependencies for their applications. This feature, inspired in the Spring Boot Java framework, eliminates the need to manually provision services like databases, caches, or message brokers during development, streamlining the development workflow and ensuring consistent development or testing environments across team members.
The implementation provides a clean interface for managing dependencies through the
Serviceinterface, which handles the lifecycle of development dependencies including starting and terminating them, and checking its state. This feature is particularly valuable for teams working on microservices or applications with multiple dependencies, as it simplifies the development setup process and reduces configuration overhead.Changes introduced
New Features
Serviceinterface for managing development-time dependenciesCode Changes
services.gofile with core implementationDocumentation
Added comprehensive documentation for the new feature
Included usage examples and best practices (a redis container started through testcontainers, including global State usage)
Benchmarks: Describe any performance benchmarks and improvements related to the changes.
Documentation Update: Detail the updates made to the documentation and links to the changed files.
Changelog/What's New: Include a summary of the additions for the upcoming release notes.
Migration Guide: If necessary, provide a guide or steps for users to migrate their existing code to accommodate these changes.
API Alignment with
ExpressSpring Boot: Explain how the changes align with theExpressSpring Boot API.API Longevity: Discuss the steps taken to ensure that the new or updated APIs are consistent and not prone to breaking changes.
Examples: Provide examples demonstrating the new features or changes in action.
Type of change
Please delete options that are not relevant.
Checklist
Before you submit your pull request, please make sure you meet these requirements:
Express.jsSpringBoot framework for new functionalities, making them similar in usage./docs/directory for Fiber's documentation.Commit formatting
Please use emojis in commit messages for an easy way to identify the purpose or intention of a commit. Check out the emoji cheatsheet here: CONTRIBUTING.md