Fix memory leak in loki.process on config update #7004
Conversation
ptodev
force-pushed
the
ptodev/fix-loki-mem-leak
branch
from
fe67a9c to
9534e4a
Compare
|
Hi @thampiotr, would you mind reviewing the latest commit please? The unit tests kept being flaky, and I realised it's because the |
| @@ -85,17 +85,24 @@ func New(o component.Options, args Arguments) (*Component, error) { | |||
|
|
|||
| // Run implements component.Component. | |||
| func (c *Component) Run(ctx context.Context) error { | |||
| shutdownCh := make(chan struct{}) | |||
| wgOut := &sync.WaitGroup{} | |||
| defer func() { | |||
| c.mut.RLock() | |||
There was a problem hiding this comment.
Is it safe and desired to wgOut.Wait() while holding this lock? Probably out of scope here, but I think we may have a goroutine waiting for c.mut in Update that would still create resources as Run is shutting down.
Happy to leave it for now, as we have other, more frequently occurring problems.
There was a problem hiding this comment.
If I understand correctly, the issue you're referring to can be avoided by adding a check in Update right after c.mut.Lock() - if the context got canceled, the function could return? That sounds fair, but I think wgOut has to stay regardless? It's Update's responsibility to make sure it doesn't do any updates if the component is shutting down.
There was a problem hiding this comment.
That sounds fair, but I think wgOut has to stay regardless?
Leaving it would be part of some solutions to this problem, yes. Not sure it must stay.
There was a problem hiding this comment.
You're right that this situation could result in a leak, but in practice I don't think it'd be a problem. The only time when Run exists is when the process exits. Hence, a leak wouldn't have any impact. I suppose other components might have a similar issues too btw.
github-actions
bot
added
the
frozen-due-to-age
PR Description
A goroutine wasn't being stopped in Update, which resulted in a memory leak.
The same change was also introduced to Alloy.
PR Checklist