Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

fix/internal/memcmd: fix goroutine leak in linux observer#63206

Merged
ggilmore merged 1 commit into
mainfrom
06-11-fix_internal_memcmd_fix_goroutine_leak_in_linux_observer
Jun 11, 2024
Merged

fix/internal/memcmd: fix goroutine leak in linux observer#63206
ggilmore merged 1 commit into
mainfrom
06-11-fix_internal_memcmd_fix_goroutine_leak_in_linux_observer

Conversation

@ggilmore

@ggilmore ggilmore commented Jun 11, 2024

Copy link
Copy Markdown
Contributor

There was a goroutine leak in the Linux observation logic.

https://github.com/sourcegraph/sourcegraph/blob/add4baa455ef86d60b59f7c0182839a7bb2a9e18/internal/memcmd/observer_linux.go#L122-L141

In observe, it was possible for the goroutine that produces collection events on a channel to block forever. If we exit early from the function, it was still possible for the channel to be full (because the consumer exited early), which causes the producer goroutine to block forever since there is no room in the channel.

This PR fixes this issue by adding a defer statement that ensures that collection channel is drained before we exit - thus fixing the leak.

Test plan

Added goroutine leak dector to linux tests - now see them pass.

Changelog

A goroutine leak in the experimental linux memory observation logic has been fixed.

@cla-bot cla-bot Bot added the cla-signed label Jun 11, 2024

Copy link
Copy Markdown
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @ggilmore and the rest of your teammates on Graphite Graphite

@github-actions github-actions Bot added team/product-platform team/source Tickets under the purview of Source - the one Source to graph it all labels Jun 11, 2024
@ggilmore ggilmore requested review from a team and eseliger June 11, 2024 18:03
@ggilmore ggilmore marked this pull request as ready for review June 11, 2024 18:03

@eseliger eseliger left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nice test tool

@graphite-app

graphite-app Bot commented Jun 11, 2024

Copy link
Copy Markdown

Video gif. A toddler grins wide, raising their hand and giving an exuberant thumbs up at us. Their nose krinkles under their big smile.  (Added via Giphy)

@ggilmore ggilmore force-pushed the 06-11-fix_internal_memcmd_fix_goroutine_leak_in_linux_observer branch from f74052a to a08dde7 Compare June 11, 2024 18:26
@ggilmore ggilmore merged commit 1bf52b7 into main Jun 11, 2024

Copy link
Copy Markdown
Contributor Author

Merge activity

@ggilmore ggilmore deleted the 06-11-fix_internal_memcmd_fix_goroutine_leak_in_linux_observer branch June 11, 2024 18:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla-signed team/product-platform team/source Tickets under the purview of Source - the one Source to graph it all

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants