[config/configtls] Enable goleak check #9220
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9220 +/- ##
==========================================
- Coverage 91.87% 91.85% -0.02%
==========================================
Files 360 360
Lines 16725 16736 +11
==========================================
+ Hits 15366 15373 +7
- Misses 1021 1024 +3
- Partials 338 339 +1 ☔ View full report in Codecov by Sentry. |
|
Sorry about the go.mod conflict. Once that is fixed, this can be merged. Thank you! |
jpkrohling
added
the
ready-to-merge
config/configtls/configtls.go
Outdated
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if c.ReloadClientCAFile { | ||
| err = reloader.startWatching() | ||
| err = c.reloader.startWatching() |
There was a problem hiding this comment.
Strange logic that we create a reloader even when not starting, maybe split the logic? Separate PR would be preferred for that.
There was a problem hiding this comment.
To make sure I understand, you're saying it's possible to get here even when the collector is not starting, so we should fix this logic to make sure the reloader only starts watching during start, am I following your thought?
bogdandrutu
removed
the
ready-to-merge
|
@jpkrohling removed the ready-to-use since I believe this does not work for multiple calls to get settings, because of that I think is better to design a different API. |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
crobert-1
force-pushed
the
goleak_configtls
branch
from
f00f265
to
f805a97
Compare
| @@ -132,7 +132,7 @@ func (r *clientCAsFileReloader) handleWatcherEvents() { | |||
|
|
|||
| func (r *clientCAsFileReloader) shutdown() error { | |||
| if r.shutdownCH == nil { | |||
| return fmt.Errorf("client CAs file watcher is not running") | |||
There was a problem hiding this comment.
I don't think we should return an error if the shutdown channel is nil.
// This method must be safe to call:
// - without Start() having been called
// - if the component is in a shutdown state already
Maybe I'm misinterpreting what safe to call means, but I don't think we should return an error unless there's an issue shutting down. I don't think we should error if shutdown is successful. (Which I believe is the second point from docs here).
Even though this itself isn't specifically a collector component, I believe docs still apply as it's a part of collector shutdown.
There was a problem hiding this comment.
I think your change makes sense. I interpret that comment to mean "calling shutdown on something not started should not error".
crobert-1
force-pushed
the
goleak_configtls
branch
from
04a3d73
to
5cefecb
Compare
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
@jpkrohling and @bogdandrutu: I believe I've addressed the concerns mentioned, another look would be appreciated when you're able! |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
receiver/otlpreceiver/otlp.go
Outdated
| if r.cfg != nil { | ||
| if r.cfg.HTTP != nil && r.cfg.HTTP.TLSSetting != nil { | ||
| err = r.cfg.HTTP.TLSSetting.Shutdown() | ||
| } | ||
|
|
||
| if r.cfg.GRPC != nil && r.cfg.GRPC.TLSSetting != nil { | ||
| err = multierr.Append(err, r.cfg.GRPC.TLSSetting.Shutdown()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Looking at this some more I really don't like that any component that uses configtls has to add this check. I am worried about components not realizing that the configtls.TLSSetting embedded in confighttp.ServerConfig needs a Shutdown call.
I think it is out of scope of this PR, which is only trying to solve a leak added by existing functionality, but I think we should take another look at how config modules should handle goroutines and Shutdown.
crobert-1
force-pushed
the
goleak_configtls
branch
2 times, most recently
from
deb37e5
to
e68541c
Compare
This change enables goleak to check the config/configtls package for memory and goroutine leaks. There was originally a goroutine leak which is fixed by calling the newly added method TLSServerSetting.Shutdown. Make reloader member private Add changelog Add changelog Call shutdown Implement shutdown callback Remove unnecessary Shutdown method Use defer instead of list of shutdown calls Update changelog Move logic to shutdown call Add missing shutdown, fix import ordering Fix references Changes requested by Tyler - Make all functions same receiver type - Call all shutdown functions Use error.join instead of multierr Fix errors.join usage
crobert-1
force-pushed
the
goleak_configtls
branch
from
9683569
to
d3e9273
Compare
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
Q: why do we implement reloading of client CA file with a background goroutine instead of using the same approach as This would avoid a goroutine leak and the unpleasant side-effects of having to call Shutdown on a "config" object. |
Description:
This change enables goleak to check the config/configtls package for memory and goroutine leaks. There was originally a goroutine leak which is fixed by calling the newly added method
TLSServerSetting.Shutdown.The major change of this PR is to make the
*clientCAsFileReloaderobject a member of theTLSServerSettingstruct, as well as adding a publicShutdownmethod toTLSServerSetting. This was the cleanest way I could add a way to shut down the reloader, otherwise it will be a leaked goroutine.Also, I had to change
LoadTLSConfig()to be a pointer type receiver, so that it could modify the given object by reference instead of only having the value copy of theTLSServerSettingobject it was called on.Link to tracking Issue:
#9165
Testing:
Goleak is passing successfully, as well as all existing tests.