Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: remove unsafe pkg usage from util.mempool #15428

Merged
merged 2 commits into from
Dec 16, 2024
Merged

Conversation

na--
Copy link
Member

@na-- na-- commented Dec 16, 2024

What this PR does / why we need it:

Before this change, when I was running the tests with -race, the following error occurred:

fatal error: checkptr: unsafe.Slice result straddles multiple allocations

goroutine 60 gp=0xc0001ba8c0 m=5 mp=0xc000100008 [running]:
runtime.throw({0x14afd15?, 0xc0004e3d30?})
	runtime/panic.go:1067 +0x48 fp=0xc0004e3c18 sp=0xc0004e3be8 pc=0x4b0148
runtime.unsafeslicecheckptr(0x130a920, 0xc0002fa000, 0x40)
	runtime/unsafe.go:88 +0x65 fp=0xc0004e3c40 sp=0xc0004e3c18 pc=0x4a5b85
github.com/grafana/loki/v3/pkg/util/mempool.(*slab).get(0xc0002e5400, 0x10)
	github.com/grafana/loki/v3/pkg/util/mempool/pool.go:58 +0x25b fp=0xc0004e3d58 sp=0xc0004e3c40 pc=0x1299e7b
github.com/grafana/loki/v3/pkg/util/mempool.(*MemPool).Get(0xc0001a5f60, 0x10)
	github.com/grafana/loki/v3/pkg/util/mempool/pool.go:103 +0x146 fp=0xc0004e3e28 sp=0xc0004e3d58 pc=0x129a726
github.com/grafana/loki/v3/pkg/util/mempool.TestMemPool.func4(0xc00018d040)
	github.com/grafana/loki/v3/pkg/util/mempool/pool_test.go:63 +0x1f5 fp=0xc0004e3ee0 sp=0xc0004e3e28 pc=0x129b455
testing.tRunner(0xc00018d040, 0x14d9bd0)

And while the test can be fixed, I didn't see a reason to use unsafe in this package at all

@na-- na-- requested a review from a team as a code owner December 16, 2024 13:09
pkg/util/mempool/pool.go Outdated Show resolved Hide resolved
@na-- na-- merged commit e6d82b9 into main Dec 16, 2024
58 checks passed
@na-- na-- deleted the ned/remove-unsafe-code branch December 16, 2024 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants