-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Goroutine leaks in several packages #5066
Conversation
Signed-off-by: Harshvir Potpose <hpotpose62@gmail.com>
Signed-off-by: Harshvir Potpose <hpotpose62@gmail.com>
Signed-off-by: Harshvir Potpose <hpotpose62@gmail.com>
Signed-off-by: Harshvir Potpose <hpotpose62@gmail.com>
I've ignored |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #5066 +/- ##
========================================
Coverage 95.56% 95.56%
========================================
Files 313 315 +2
Lines 18160 18323 +163
========================================
+ Hits 17355 17511 +156
- Misses 645 651 +6
- Partials 160 161 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Harshvir Potpose <hpotpose62@gmail.com>
Signed-off-by: Harshvir Potpose <hpotpose62@gmail.com>
Signed-off-by: Harshvir Potpose <hpotpose62@gmail.com>
// be stopped when the test finishes, leading to a detected but expected leak. | ||
func IgnoreGlogFlushDaemonLeak() goleak.Option { | ||
return goleak.IgnoreTopFunction("github.com/golang/glog.(*fileSink).flushDaemon") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add package_test invoking this function to appease code coverage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I just realized, we could've done this differently by adding testutils.CheckGoroutineLeaks
function that would be called from the rest of the codebase, instead of having two dependencies everywhere (goleak and testutils).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, we can also do that.
Signed-off-by: Harshvir Potpose <hpotpose62@gmail.com>
// IgnoreGlogFlushDaemonLeak returns a goleak.Option that ignores the flushDaemon function | ||
// from the glog package that can cause false positives in leak detection. | ||
// This is necessary because glog starts a goroutine in the background that may not | ||
// be stopped when the test finishes, leading to a detected but expected leak. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great explanation!
Signed-off-by: Harshvir Potpose <hpotpose62@gmail.com>
Thanks! |
Which problem is this PR solving?
Description of the changes
How was this change tested?
Checklist
jaeger
:make lint test