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

use goleak to autodetect leaks #2128

Merged
merged 13 commits into from Dec 2, 2022
Merged

use goleak to autodetect leaks #2128

merged 13 commits into from Dec 2, 2022

Conversation

szuecs
Copy link
Member

@szuecs szuecs commented Nov 3, 2022

replaces #2051 by using https://github.com/AlexanderYastrebov/noleak

One problem I see is uber-go/goleak#83 , so not sure if we want to merge this or better add only fixes for findings and drop the use of goleak. What do you think?

We agreed on that we use only package based tests and I decided to give https://github.com/AlexanderYastrebov/noleak a try since it's simpler than goleak and Uber engineers seem not to be responsive in this project.

Signed-off-by: Sandor Szücs sandor.szuecs@zalando.de

@MustafaSaber
Copy link
Member

👍

@AlexanderYastrebov
Copy link
Member

not sure if we want to merge this or better add only fixes for findings and drop the use of goleak

Did it discover any leaks actually?

@szuecs
Copy link
Member Author

szuecs commented Nov 16, 2022

Yes it does. I was also not able to fix all cases, for example go-redis heartbeat.
Beware of the linked issue, it will find also goroutines spawned by tests that are not part of the test that has the validation enabled. Who knows how this works with Parallel tests.

@AlexanderYastrebov
Copy link
Member

One problem I see is uber-go/goleak#83 , so not sure if we want to merge this or better add only fixes for findings and drop the use of goleak. What do you think?

We should definitely include leak fixes!

There is no easy way to fix uber-go/goleak#83 as defer goleak.VerifyNone(t) implementation looks at the snapshot of all goroutines at the test end so it would see goroutines started by other tests.

There is also an older lib https://github.com/fortytw2/leaktest that looks at the diff between goroutine snapshots before test start and after test ends.

Who knows how this works with Parallel tests.

Both libraries provide per-test checks (defer goleak.VerifyNone(t) and defer leaktest.Check(t)()) that would not work well with parallel tests.

go-leak provides goleak.VerifyTestMain(m) to check leaks after package tests (leaktest does not, see fortytw2/leaktest#4) that should work for parallel tests.

I think we should use goleak.VerifyTestMain(m) per package and not pollute tests with defer goleak.VerifyNone(t).
There is also GODEBUG="tracebackancestors=N" flag that simplifies leak debugging, see https://pkg.go.dev/runtime

PS: I was curious how it works and to understand re-implemented it here https://github.com/AlexanderYastrebov/noleak
It compares snapshots before and after test (like leaktest) and also supports TestMain (like go-leak)

@szuecs szuecs force-pushed the test/go-leak branch 2 times, most recently from 8e16270 to 719270d Compare November 30, 2022 14:42
test that redistest does not leak goroutines
fix: found goroutine leaks
fix: wrap close in sync.once.Do
fix: shallow copy can not copy by deref ptr in case it contains sync.Once
refactor: *sync-Once to sync.Once as shown in stdlib Go example

fix as commented: cleanup nil check
fix as commented: remove t.closed in httpclient
test: add test that NewClient will never return nil
fix as commented: remove time.Sleep
fix as commented: use embedding to get rid of delegations

Signed-off-by: Sandor Szücs <sandor.szuecs@zalando.de>
Signed-off-by: Sandor Szücs <sandor.szuecs@zalando.de>
Signed-off-by: Sandor Szücs <sandor.szuecs@zalando.de>
…very tick run

Signed-off-by: Sandor Szücs <sandor.szuecs@zalando.de>
Signed-off-by: Sandor Szücs <sandor.szuecs@zalando.de>
add todo to refactor receiveFromClient to use ticker instead of time.After

Signed-off-by: Sandor Szücs <sandor.szuecs@zalando.de>
Signed-off-by: Sandor Szücs <sandor.szuecs@zalando.de>
Signed-off-by: Sandor Szücs <sandor.szuecs@zalando.de>
diag_test.go:907: p25 not in range want p25=9ms with epsilon=1ms, got: 7.876215ms
diag_test.go:986: p25 not in range want p25=9ms with epsilon=1ms, got: 7.908821ms

Signed-off-by: Sandor Szücs <sandor.szuecs@zalando.de>
…eneral

Signed-off-by: Sandor Szücs <sandor.szuecs@zalando.de>
…rror in creating the dataclient

Signed-off-by: Sandor Szücs <sandor.szuecs@zalando.de>
… leak checkers

Signed-off-by: Sandor Szücs <sandor.szuecs@zalando.de>
Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
@szuecs
Copy link
Member Author

szuecs commented Dec 2, 2022

👍

1 similar comment
@AlexanderYastrebov
Copy link
Member

👍

@szuecs szuecs merged commit 94faee3 into master Dec 2, 2022
@szuecs szuecs deleted the test/go-leak branch December 2, 2022 17:08
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.

None yet

3 participants