-
Notifications
You must be signed in to change notification settings - Fork 1.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
[service] Fix memory leaks and enable goleak check in tests #9241
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9241 +/- ##
==========================================
- Coverage 91.64% 91.63% -0.01%
==========================================
Files 406 406
Lines 19001 19007 +6
==========================================
+ Hits 17414 17418 +4
- Misses 1227 1229 +2
Partials 360 360 ☔ View full report in Codecov by Sentry. |
17a159e
to
0c77122
Compare
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
35ab246
to
8bc1cc1
Compare
93f6927
to
71f5312
Compare
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
71f5312
to
7848e80
Compare
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
@crobert-1 What's the status of this PR? Does it need more reviews? Do you need to fix something? (No rush, just trying to clean up our backlog) |
Just waiting for reviews 👍 |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
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.
Can you fix the merge conflicts? I can merge after that
Should be good now, thanks @mx-psi! |
Description:
This change adds
goleak
to check for memory leaks. Originally there were 3 failing tests in theservice
package, so I'll describe changes in relation to resolving each test's failing goleak check.TestServiceTelemetryRestart
: Simplest fix, close the response body to make sure goroutines aren't leaked by reopening a server on the same port. This was just a test issueTestTelemetryInit.UseOTelWithSDKConfiguration
: The meter provider was being started in the initialization process (metrics reference), but never shutdown. The type originally being used (meter.MetricProvider
) was the base interface which didn't provide aShutdown
method. I changed this to use thesdk
interfaces that provide the requiredShutdown
method. The actual functionality of starting the providers was already using and returning thesdk
interface, so the actual underlying type remains the same. Sincemp
is a private member andsdkmetric
and implement the original type, I don't believe changing the type is a breaking change.TestServiceTelemetryCleanupOnError
: This test starts a server using a sub-goroutine, cancels it, starts again in a subroutine, and cancels again in the main goroutine. This test showed the racing behavior of the subroutine runningserver.ListenAndServe
and the main goroutine's functionality of calling close and then starting the server again right away. The solution here is to add async.WaitGroup
variable that can properly block until all servers are closed before returning fromshutdown
. This will allow us to ensure it's safe to proceed knowing the ports are free, and server is fully closed.The first test fix was just a test issue, but 2 and 3 were real bugs. I realize it's a bit hard to read with them all together, but I assumed adding PR dependency notes would be more complicated.
Link to tracking Issue:
#9165
Testing:
All tests are passing as well as goleak check.