[internal/common/ttlmap] Fix leak caused by time.Tick #32044
Conversation
crobert-1
force-pushed
the
goleak_ttlmap
branch
from
9d53069 to
8b63562
Compare
github-actions
bot
requested review from
arminru,
dmitryax,
dyladan and
evan-bradley
| } | ||
|
|
||
| // New creates a TTLMap. The sweepIntervalSeconds arg indicates how often | ||
| // entries are checked for expiration. The maxAgeSeconds arg indicates how long | ||
| // entries can persist before getting evicted. Call Start() on the returned | ||
| // TTLMap to begin periodic sweeps which check for expiration and evict entries | ||
| // as needed. | ||
| func New(sweepIntervalSeconds int64, maxAgeSeconds int64) *TTLMap { | ||
| // done is the channel that will be used to signal to the timer to stop its work. | ||
| func New(sweepIntervalSeconds int64, maxAgeSeconds int64, done chan struct{}) *TTLMap { |
There was a problem hiding this comment.
Apologies for the size of this PR now. I originally wanted to simply create the channel inside of New, but an existing test exporter/signalfxexporter/TestConfigGetMetricTranslator does an Assert.Equal on objects that contain the TTLMap as a private member. The assertion fails as the pointers are not equal. I've attempted a few workarounds (cmp and cmpopts, yaml or JSON marshalling). Nothing works.
If the preference is to simply not have a done channel and use the timer and the timer.Stop() method instead, I can modify this PR to pass that in. Either way, the signature of New will need to be updated to accomodate.
There was a problem hiding this comment.
An alternative that may be less impacting would be to pass the done channel to Start. I don't think passing in the Ticker would work as it gets started at creation. I don't think it would be a great design to pass a running Ticker to these methods.
There was a problem hiding this comment.
the TTLMap struct might also create its own done channel. Wouldn't that be best?
There was a problem hiding this comment.
That would be best, but the usage and testing is where the challenge comes in. The TestConfigGetMetricTranslator is testing to ensure the config is created properly relative to the metric translator itself in the signalfx exporter. When the TTLMap creates its own channel the equality comparison fails.
The other options are remove the test, or add a public equality checking method to the metrics translator.
|
@crobert-1 Sorry for the delay, could you remove the Dynatrace exporter changes? It has been removed from the repo. |
Not a problem, thanks for pointing this out! I've removed references 👍 |
Description:
Documentation shows the existing usage of
time.Tickwill leak a goroutine on shutdown. This updates theinternal/common/ttlmaptick functionality to use a ticker with a context that has a cancel function. This allows the ticker to be properly shut down.I don't believe this requires a changelog because no user-facing components were updated to actually call the shutdown function. It makes more sense to me to add those after this gets merged to keep this PR more concise.
This also adds
goleakto the package to help ensure goroutines aren't being leaked.Link to tracking Issue:
Related to #30438
Testing:
Existing tests are still passing, goleak check was failing before change, succeeds after.