Skip to content

device: avoid cycle-leaky runtime.SetFinalizer when unnecessary#66

Merged
bradfitz merged 1 commit into
tailscalefrom
bradfitz/finalizer
Jun 4, 2026
Merged

device: avoid cycle-leaky runtime.SetFinalizer when unnecessary#66
bradfitz merged 1 commit into
tailscalefrom
bradfitz/finalizer

Conversation

@bradfitz

@bradfitz bradfitz commented Jun 3, 2026

Copy link
Copy Markdown
Member

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

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>
@bradfitz
bradfitz requested review from a team, lkosewsk and raggi June 3, 2026 16:15

@sfllaw sfllaw left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a blocker, but if there are cases where we need to drain the queue, would it be safer to use runtime.AddCleanup?

Comment thread device/channels.go
Comment on lines +86 to +88
if device.needsInboundQueueFinalizer() {
runtime.SetFinalizer(q, device.flushInboundQueue)
}

@sfllaw sfllaw Jun 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Suggestion (non-blocking): Would it be possible to convert this so it uses runtime.AddCleanup, which might also be safer?

Suggested change
if device.needsInboundQueueFinalizer() {
runtime.SetFinalizer(q, device.flushInboundQueue)
}
if device.needsInboundQueueFinalizer() {
runtime.AddCleanup(q, device.flushInboundQueue, q.c)
}

(continued…)

Comment thread device/channels.go
Comment on lines 98 to 101
func (device *Device) flushInboundQueue(q *autodrainingInboundQueue) {
for {
select {
case elemsContainer := <-q.c:

@sfllaw sfllaw Jun 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⬆️ …continued (non-blocking), I think the flusher only needs a minor refactor?

func (device *Device) flushInboundQueue(c chan *QueueInboundElementsContainer) {
	for {
		select {
		case elemsContainer := <-c:

@bradfitz

bradfitz commented Jun 4, 2026

Copy link
Copy Markdown
Member Author

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.

@bradfitz
bradfitz merged commit 58f7aac into tailscale Jun 4, 2026
14 checks passed
sfllaw added a commit that referenced this pull request Jun 17, 2026
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>
sfllaw added a commit that referenced this pull request Jun 17, 2026
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>
sfllaw added a commit that referenced this pull request Jun 19, 2026
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants