🩹 Fix: goroutine leakage #3306
🩹 Fix: goroutine leakage #3306
Conversation
JIeJaitt
requested review from
gaby,
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
WalkthroughThis pull request modifies the error channel in the Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant A as App.Test
participant E as Error Channel
C->>A: Invoke Test connection
A->>E: Send error to buffered channel
E-->>A: Return error without blocking
A->>C: Return response or error
sequenceDiagram
participant T as Test Function
participant A as App Instance
participant H as Handler
participant G as Goroutine Monitor
T->>A: Create new app instance
T->>G: Record initial goroutine count
T->>A: Setup GET route with handler H
T->>A: Send multiple requests
A->>H: Handle incoming request in goroutine
T->>G: Record final goroutine count
T->>T: Assert leak comparison based on expected outcome
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3306 +/- ##
==========================================
- Coverage 84.08% 84.06% -0.03%
==========================================
Files 116 116
Lines 11551 11551
==========================================
- Hits 9713 9710 -3
- Misses 1405 1407 +2
- Partials 433 434 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
app_test.go (3)
60-62: Use a consistent naming convention for clarity.
“Test_App_Test_Goroutine_Leak_Compare” is descriptive but a bit verbose. Consider a name like “TestGoroutineLeaks” for ease of reference.
63-69: Consider adding clarifying comments for each field in the test case struct.
Having brief comments for fields likesleepTimeandexpectLeakhelps future maintainers quickly grasp their usage in test logic.
121-129: Provide explicit synchronization or forced GC for more reliable measurements.
Relying solely ontime.Sleepandruntime.NumGoroutine()can be brittle if other goroutines run concurrently (e.g., from parallel tests). Consider more robust signaling (like wait groups) or forcingruntime.GC()to yield more stable results.app.go (1)
997-1009: Verify success path handling around the buffered channel.
Using a buffered channel helps avoid blocking, but note the extra complexity in the deferred function withreturned. Consider simplifying by returning the error immediately if feasible, or renamereturnedto something more specific (likeserveConnCompleted).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app.go(1 hunks)app_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: unit (1.23.x, macos-latest)
- GitHub Check: unit (1.23.x, windows-latest)
- GitHub Check: repeated
- GitHub Check: Compare
- GitHub Check: Analyse
🔇 Additional comments (1)
app_test.go (1)
91-93: Potential parallel test flakiness.
Running both the main loop and each subtest witht.Parallel()can make runtime goroutine counts unpredictable. This might cause intermittent failures in environments with many parallel tests.
| if tc.expectLeak { | ||
| // before fix: If blocking exists, leaked goroutines should be at least equal to request count | ||
| // after fix: If no blocking exists, leaked goroutines should be less than request count | ||
| if leakedGoroutines >= numRequests { | ||
| t.Errorf("[%s] Expected at least %d leaked goroutines, but got %d", | ||
| tc.name, numRequests, leakedGoroutines) | ||
| } | ||
| } else if leakedGoroutines >= numRequests { // If no blocking exists, leaked goroutines should be less than request count |
There was a problem hiding this comment.
Reevaluate the leak checking logic.
The condition and error message seem inverted. If tc.expectLeak == true, requiring “at least N leaked goroutines,” you might want to flag an error when leakedGoroutines < numRequests instead of >= numRequests. Currently, it flags an error if the condition is satisfied, which contradicts the comment.
Suggest correcting as follows (example flip of condition):
-if leakedGoroutines >= numRequests {
- t.Errorf("[%s] Expected at least %d leaked goroutines, but got %d", ...)
+if leakedGoroutines < numRequests {
+ t.Errorf("[%s] Expected at least %d leaked goroutines, but got %d", ...)📝 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 tc.expectLeak { | |
| // before fix: If blocking exists, leaked goroutines should be at least equal to request count | |
| // after fix: If no blocking exists, leaked goroutines should be less than request count | |
| if leakedGoroutines >= numRequests { | |
| t.Errorf("[%s] Expected at least %d leaked goroutines, but got %d", | |
| tc.name, numRequests, leakedGoroutines) | |
| } | |
| } else if leakedGoroutines >= numRequests { // If no blocking exists, leaked goroutines should be less than request count | |
| if tc.expectLeak { | |
| // before fix: If blocking exists, leaked goroutines should be at least equal to request count | |
| // after fix: If no blocking exists, leaked goroutines should be less than request count | |
| if leakedGoroutines < numRequests { | |
| t.Errorf("[%s] Expected at least %d leaked goroutines, but got %d", | |
| tc.name, numRequests, leakedGoroutines) | |
| } | |
| } else if leakedGoroutines >= numRequests { // If no blocking exists, leaked goroutines should be less than request count |
Description
As mentioned in the #3304 , I modified the channel of the Test function to use a buffered channel, and added a unit test to verify goroutine leakage with and without a timeout.
Fixes #3304
Changes introduced
List the new features or adjustments introduced in this pull request. Provide details on benchmarks, documentation updates, changelog entries, and if applicable, the migration guide.
Type of change
Please delete options that are not relevant.
Checklist
Before you submit your pull request, please make sure you meet these requirements:
/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