Fix leaking goroutine in memorylimiter #9100
Conversation
| ml.closeOnce.Do(func() { | ||
| close(ml.closed) | ||
| }) |
There was a problem hiding this comment.
Don't need this, we guarantee to close only once.
There was a problem hiding this comment.
without this
that test failsThere was a problem hiding this comment.
I personally think that this test doesn't make sense. I would like @open-telemetry/collector-approvers to say their opinion.
There was a problem hiding this comment.
I think we can remove the second Shutdown call from the test. We don't have guarantees that it's allowed.
There was a problem hiding this comment.
Oh, it's Start->Shutdown->Start-> causing the failure... This change breaks that workflow even without ml.closeOnce. I don't think we have other components supporting this workflow. So should be fine to not test it.
There was a problem hiding this comment.
so maybe let's move closed and closeOnce initialization to Start method, the behavior will stay the same
We can move closed channel creation to Start. closeOnce will not be needed anymore.
@dmitryax is calling a processor's Start after calling Shutdown an unsupported action? Would OPAMP ever need to restart a shutdown component?
I'm not sure
If that pattern is not allowed I agree that the test is invalid and we can treat the code in shutdown as only ever being invoked once.
I'm not saying it's not allowed. We just don't explicitly support it. Likely some components will be failing.
Probably better not not break this pattern here with this change and just move closed channel creation to Start.
There was a problem hiding this comment.
Would OPAMP ever need to restart a shutdown component?
If we ever want a hot restart we will add a "Reload" capability. Once the component is shutdown, I see no reason to re-start it.
There was a problem hiding this comment.
ok, I moved closed channel to Start and removed closeOnce
There was a problem hiding this comment.
I moved it to startMonitoring, so the closed channel is created when the goroutine is created. When there are metrics, logs and traces in the pipeline it would be called 3 times and closed would be created 3 times too resulting in data race
There was a problem hiding this comment.
When there are metrics, logs and traces in the pipeline it would be called 3 times and closed would be created 3 times too resulting in data race
Why? They are synchronized via refCounterLock
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9100 +/- ##
==========================================
+ Coverage 91.50% 91.52% +0.02%
==========================================
Files 320 320
Lines 17189 17198 +9
==========================================
+ Hits 15728 15740 +12
+ Misses 1163 1161 -2
+ Partials 298 297 -1 ☔ View full report in Codecov by Sentry. |
|
@bogdandrutu I forgot about the changelog |
|
Has anyone considered enforcing https://github.com/uber-go/goleak in all tests? |
It would be nice to have. I think I saw some leaking goroutines with the open census instrumentation. So we probably need to wait until we migrate from it. I'll create an issue anyway |
Description:
Fixing a bug - described here #9099
Link to tracking Issue:
#9099
Testing:
No tests added.
Unit tests were run
Documentation:
None