device: avoid cycle-leaky runtime.SetFinalizer when unnecessary#66
Conversation
In #65, @lkosewsk reproduced a memory leak seen in prod with lots of wireguard-go instances being created and destroyed, where they were still being retained forever due to cycles in the runtime.SetFinalizer reference graph. Really we shouldn't be using runtime.SetFinalizer anywhere. But we still use it on mobile platforms in WaitPool. But those platforms don't have thousands of tsnet.Server instances coming & going, so this is a half fix: avoid the finalizer registration on Linux, etc where the queue doesn't need to be drained and there's no WaitPool accounting. Just let GC handle it, without adding finalizer cycle complexity. Updates tailscale/corp#42776 Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
sfllaw
left a comment
There was a problem hiding this comment.
Not a blocker, but if there are cases where we need to drain the queue, would it be safer to use runtime.AddCleanup?
| if device.needsInboundQueueFinalizer() { | ||
| runtime.SetFinalizer(q, device.flushInboundQueue) | ||
| } |
There was a problem hiding this comment.
💡 Suggestion (non-blocking): Would it be possible to convert this so it uses runtime.AddCleanup, which might also be safer?
| if device.needsInboundQueueFinalizer() { | |
| runtime.SetFinalizer(q, device.flushInboundQueue) | |
| } | |
| if device.needsInboundQueueFinalizer() { | |
| runtime.AddCleanup(q, device.flushInboundQueue, q.c) | |
| } |
| func (device *Device) flushInboundQueue(q *autodrainingInboundQueue) { | ||
| for { | ||
| select { | ||
| case elemsContainer := <-q.c: |
There was a problem hiding this comment.
⬆️ …continued (non-blocking), I think the flusher only needs a minor refactor?
func (device *Device) flushInboundQueue(c chan *QueueInboundElementsContainer) {
for {
select {
case elemsContainer := <-c:|
runtime.AddCleanup would work, but really I want to get rid of finalizers entirely (with either mechanism). I'm neutral if people want to switch the iOS/Android builds over to AddCleanup meanwhile, but I think it's kinda a waste of time if the end goal is to remove them entirely. If the old finalizer mechanism is "working" for now, I'd rather not touch it in its final days and just let it be. |
In PR #66, we tried to address a memory leak by avoiding runtime.SetFinalizer for autodrainingInboundQueueand and autodrainingOutboundQueue unless there was something to do. However, when there is work to be done, these finalizers still leak memory because they’re still holding on to a cyclical reference to q. This applies to any platform that relies on a bounded device.WaitPool, like Android and iOS which both declare PreallocatedBuffersPerPool. This patch converts this logic to runtime.AddCleanup which is designed to avoid this problem. Updates tailscale/corp#42776 Signed-off-by: Simon Law <sfllaw@tailscale.com>
In PR #66, we tried to address a memory leak by avoiding runtime.SetFinalizer for autodrainingInboundQueue and autodrainingOutboundQueue unless there was something to do. However, when there is work to be done, these finalizers still leak memory because they’re still holding on to a cyclical reference to q. This applies to any platform that relies on a bounded device.WaitPool, like Android and iOS which both declare PreallocatedBuffersPerPool. This patch converts this logic to runtime.AddCleanup which is designed to avoid this problem. Updates tailscale/corp#42776 Signed-off-by: Simon Law <sfllaw@tailscale.com>
In PR #66, we tried to address a memory leak by avoiding runtime.SetFinalizer for autodrainingInboundQueue and autodrainingOutboundQueue unless there was something to do. However, when there is work to be done, these finalizers still leak memory because they’re still holding on to a cyclical reference to q. This applies to any platform that relies on a bounded device.WaitPool, like Android and iOS which both declare PreallocatedBuffersPerPool. This patch converts this logic to runtime.AddCleanup which is designed to avoid this problem. Updates tailscale/corp#42776 Signed-off-by: Simon Law <sfllaw@tailscale.com>
In #65, @lkosewsk reproduced a memory leak seen
in prod with lots of wireguard-go instances being created and
destroyed, where they were still being retained forever due to cycles
in the runtime.SetFinalizer reference graph.
Really we shouldn't be using runtime.SetFinalizer anywhere. But we
still use it on mobile platforms in WaitPool. But those platforms
don't have thousands of tsnet.Server instances coming & going, so this
is a half fix: avoid the finalizer registration on Linux, etc where
the queue doesn't need to be drained and there's no WaitPool
accounting. Just let GC handle it, without adding finalizer cycle
complexity.
Updates tailscale/corp#42776