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

route-sync is leaking tickers #140

Closed
st3v opened this Issue Nov 28, 2017 · 2 comments

Comments

Projects
None yet
4 participants
@st3v
Member

st3v commented Nov 28, 2017

Using time.Tick to repeatably create new Tickers causes a slow memory leak in pooler.timeBased.Run.

timer = time.Tick(tb.interval)

Following is a heap profile taken with pprof for a sample that replicates the problematic code (kept it running for a few hours):

(pprof) top5
Showing nodes accounting for 78.56MB, 100% of 78.56MB total
      flat  flat%   sum%        cum   cum%
   75.01MB 95.48% 95.48%    78.56MB   100%  time.NewTicker /usr/local/Cellar/go/1.9.2/libexec/src/time/tick.go
    3.55MB  4.52%   100%     3.55MB  4.52%  time.startTimer /usr/local/Cellar/go/1.9.2/libexec/src/runtime/time.go
         0     0%   100%    78.56MB   100%  main.tick /Users/switzel/go/src/github.com/st3v/tick/main.go
         0     0%   100%    78.56MB   100%  time.Tick /usr/local/Cellar/go/1.9.2/libexec/src/time/tick.go

You can see that we accumulated 75MB of tickers at that point, and it's only climbing.

From the go docs:

Tick is a convenience wrapper for NewTicker providing access to the ticking channel only. While Tick is useful for clients that have no need to shut down the Ticker, be aware that without a way to shut it down the underlying Ticker cannot be recovered by the garbage collector; it "leaks". Unlike NewTicker, Tick will return nil if d <= 0.

So either use time.NewTicker() or better yet, don't re-assign timer at all. You could rewrite the function as follows:

func (tb *timeBased) Run(ctx context.Context, src route.Source, router route.Router) {
    ticker := time.NewTicker(tb.interval)
    defer ticker.Stop()

    tb.tick(src, router)

    for {
        select {
            case <-ctx.Done():
                return
            case <-ticker.C:
                tb.tick(src, router)
            }
    }
}

Site Note: It seems Pooler is doing more than what its name suggests. It looks like it's concerned both with scheduling and actually syncing routes. Would it make sense to separate these concerns? Pooler.Run could just take a function, i.e. Run(ctx context.Context, fn func()).

Fyi @jhvhs, I mentioned this last week.

@cf-gitbot

This comment has been minimized.

cf-gitbot commented Nov 28, 2017

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/153211365

The labels on this github issue will be updated when the story is started.

@Manifaust

This comment has been minimized.

Member

Manifaust commented Dec 5, 2017

We will prioritize this in the next IPM

@cf-gitbot cf-gitbot added scheduled and removed unscheduled labels Dec 5, 2017

tvs added a commit that referenced this issue Dec 5, 2017

Fixed route-sync ticker-based memory leak
Issue #140.

[#153211365]

Signed-off-by: Travis Hall <thall@vmware.com>

@BenChapman BenChapman closed this Dec 11, 2017

@cf-gitbot cf-gitbot removed the accepted label Dec 11, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment