Fixing all goroutine leaks with client closings #1041
Conversation
|
I get lint issues (with your version of golangci-lint) that have nothing to do with any of the new code. So I ignore them? |
karelbilek
force-pushed
the
fix-goroutines
branch
from
dfb7ab0
to
335edec
Compare
|
note that this and #1042 are in "conflict"; one will need to be rebased after the other is merged |
There was a problem hiding this comment.
Nice, I learned something new today - I wanted to try goleak for a while 😼
There are a couple of small changes to make ignored errors explicit. Please apply these suggestions when you rebase and resolve conflicts (and if you see similar cases in other changes - please handle these as well)
Amazing work, thank you!
preset/cockroachdb/preset.go
Outdated
| @@ -87,6 +87,10 @@ func (p *P) setDefaults() { | |||
| func healthcheck(_ context.Context, c *gnomock.Container) error { | |||
| db, err := connect(c, "") | |||
| if err != nil { | |||
| if db != nil { | |||
| db.Close() | |||
There was a problem hiding this comment.
| db.Close() | |
| _ = db.Close() |
preset/mariadb/preset.go
Outdated
| @@ -91,6 +91,10 @@ func (p *P) healthcheck(_ context.Context, c *gnomock.Container) error { | |||
|
|
|||
| db, err := p.connect(addr) | |||
| if err != nil { | |||
| if db != nil { | |||
| db.Close() | |||
There was a problem hiding this comment.
| db.Close() | |
| _ = db.Close() |
preset/mysql/preset.go
Outdated
| @@ -93,6 +93,10 @@ func (p *P) healthcheck(_ context.Context, c *gnomock.Container) error { | |||
|
|
|||
| db, err := p.connect(addr) | |||
| if err != nil { | |||
| if db != nil { | |||
| db.Close() | |||
There was a problem hiding this comment.
| db.Close() | |
| _ = db.Close() |
preset/postgres/preset.go
Outdated
| @@ -93,6 +93,10 @@ func (p *P) Options() []gnomock.Option { | |||
| func (p *P) healthcheck(_ context.Context, c *gnomock.Container) error { | |||
| db, err := connect(c, defaultDatabase) | |||
| if err != nil { | |||
| if db != nil { | |||
| db.Close() | |||
There was a problem hiding this comment.
| db.Close() | |
| _ = db.Close() |
|
I will wait for the lint fix to be merged as that will basically show this
up :)
…On Sun 19. 5. 2024 at 22:04, Yury Fedorov ***@***.***> wrote:
Assigned #1041 <#1041> to
@karelbilek <https://github.com/karelbilek>.
—
Reply to this email directly, view it on GitHub
<#1041 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAZT4MJRRPZ35WBZTC2FBLZDEAVRAVCNFSM6AAAAABH4FZBPKVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJSHA2TSMJUGEYTONQ>
.
You are receiving this because you were assigned.Message ID:
***@***.***>
|
|
Linter PR is merged 😼 |
karelbilek
force-pushed
the
fix-goroutines
branch
2 times, most recently
from
f9205cc
to
bd68ec1
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1041 +/- ##
===========================================
- Coverage 85.87% 28.25% -57.62%
===========================================
Files 50 53 +3
Lines 2350 2587 +237
===========================================
- Hits 2018 731 -1287
- Misses 173 1770 +1597
+ Partials 159 86 -73 ☔ View full report in Codecov by Sentry. |
|
No idea what to do with the code coverage stuff so I ignore it? The added line are covered by tests, I don't know what is codecov smoking. Anyway it's rebased and done no idea where is codecov taking the coverage data from. It seems it's not running the kafka tests at all? No idea |
Also adds uber's goroutine leaks checker to almost all tests The biggest change here is adding context to sidecar pull, which is then cancelled if the container creation fails. This prevents the sidecar goroutine from leaking. The same for making the sidecar channel buffered; the goroutine is not stuck when nobody reads from it. (It is not a memory leak either, GC will remove the channel.) Other changes are just annoying work; putting client.Close() everywhere, db.Close() when Ping() is not successful, client closes in tests/presets, etc. internal/gnomockd/ tests still show goroutine leaks for http transport, but I have no idea what is gnomockd even doing, let alone the tests, so I let that be.
karelbilek
force-pushed
the
fix-goroutines
branch
from
bd68ec1
to
8625d1a
Compare
There was a problem hiding this comment.
Thank you 😼
Yeah, codecov can be ignored for now. We send coverage reports from every test job, and at some point they made a change that first stopped all deliveries (I think my token was suspended? Not sure), and after that upload attempts started getting throttled with heavy rate limits.
I will hopefully get to this at some point and figure out what's wrong with the token, and maybe I'll have to look for another coverage provider, free for open source.
Also adds uber's goroutine leaks checker to almost all tests.
The biggest change here is adding context to sidecar pull, which is then cancelled if the container creation fails. This prevents the sidecar goroutine from leaking. The same for making the sidecar channel buffered; the goroutine is not stuck when nobody reads from it. (It is not a memory leak either, GC will remove the channel.)
Other changes are just annoying work; putting client.Close() everywhere, db.Close() when Ping() is not successful, client closes in tests/presets, etc.
internal/gnomockd/ tests still show goroutine leaks for http transport, but I have no idea what is gnomockd even doing, let alone the tests, so I let that be.
Fixes #1035