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.

Sign up for GitHub

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

Jump to bottom

server: close the temporary session in HTTP API to avoid memory leak #24339

Merged
merged 13 commits into from
May 13, 2021
Merged

server: close the temporary session in HTTP API to avoid memory leak #24339

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
51708c7
server,session: do not create stats collector in HTTP api to avoid me…
tiancaiamao Apr 28, 2021
9191316
fix CI
tiancaiamao Apr 28, 2021
3629b06
address comment
tiancaiamao May 12, 2021
0d757d0
Merge branch 'master' into stats-leak
tiancaiamao May 12, 2021
818d532
address comment
tiancaiamao May 13, 2021
cf7b6d4
Merge branch 'master' into stats-leak
qw4990 May 13, 2021
981e3c5
Merge branch 'master' into stats-leak
ti-chi-bot May 13, 2021
abdeccd
Merge branch 'master' into stats-leak
ti-chi-bot May 13, 2021
96b7f23
Merge branch 'master' into stats-leak
ti-chi-bot May 13, 2021
36f6992
Merge branch 'master' into stats-leak
ti-chi-bot May 13, 2021
e3f24f1
Merge branch 'master' into stats-leak
ti-chi-bot May 13, 2021
4bea4de
Merge branch 'master' into stats-leak
ti-chi-bot May 13, 2021
ca76788
Merge branch 'master' into stats-leak
ti-chi-bot May 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 10 additions & 7 deletions server/http_handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,7 +368,10 @@ func (t *tikvHandlerTool) getPartition(tableVal table.Table, partitionName strin
}

func (t *tikvHandlerTool) schema() (infoschema.InfoSchema, error) {
session, err := session.CreateSession(t.Store)
// Disable stats collector.
// Stats collector of all the sessions are linked together in the statistics handler which is a global object.
// If we create stats collector here, they may not be recycled by the Go GC and cause memory leak.
session, err := session.CreateSessionWithOpt(t.Store, &session.Opt{DisableStatsCollector: true})
Copy link
Contributor

@eurekaka eurekaka Apr 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the root cause of the memory leak is that there is no session.Close() called for this session, so s.statsCollector.Delete() is not triggered to release the stats collector. Would there be other potential leaks caused by not closing this session?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would there be other potential leaks caused by not closing this session?

I'm not sure. I wonder is it really necessary to create a temporary session here? Maybe that's a better fix, what's your opinion?

Copy link
Contributor

@qw4990 qw4990 Apr 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with @eurekaka .
Calling session.Close() when exiting from these HTTP APIs seems a safer and clearer way to solve this issue.
If we add another global resource into session like statsCollector, then this problem will occur again if using this way.

if err != nil {
return nil, errors.Trace(err)
}
Expand Down Expand Up @@ -726,7 +729,7 @@ func (h settingsHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) {
}
}
if asyncCommit := req.Form.Get("tidb_enable_async_commit"); asyncCommit != "" {
s, err := session.CreateSession(h.Store.(kv.Storage))
s, err := session.CreateSessionWithOpt(h.Store.(kv.Storage), &session.Opt{DisableStatsCollector: true})
if err != nil {
writeError(w, err)
return
Expand All @@ -749,7 +752,7 @@ func (h settingsHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) {
}
}
if onePC := req.Form.Get("tidb_enable_1pc"); onePC != "" {
s, err := session.CreateSession(h.Store.(kv.Storage))
s, err := session.CreateSessionWithOpt(h.Store.(kv.Storage), &session.Opt{DisableStatsCollector: true})
if err != nil {
writeError(w, err)
return
Expand Down Expand Up @@ -892,7 +895,7 @@ func (h flashReplicaHandler) getTiFlashReplicaInfo(tblInfo *model.TableInfo, rep
}

func (h flashReplicaHandler) getDropOrTruncateTableTiflash(currentSchema infoschema.InfoSchema) ([]*tableFlashReplicaInfo, error) {
s, err := session.CreateSession(h.Store.(kv.Storage))
s, err := session.CreateSessionWithOpt(h.Store.(kv.Storage), &session.Opt{DisableStatsCollector: false})
if err != nil {
return nil, errors.Trace(err)
}
Expand Down Expand Up @@ -967,7 +970,7 @@ func (h flashReplicaHandler) handleStatusReport(w http.ResponseWriter, req *http
writeError(w, err)
return
}
s, err := session.CreateSession(h.Store.(kv.Storage))
s, err := session.CreateSessionWithOpt(h.Store.(kv.Storage), &session.Opt{DisableStatsCollector: true})
if err != nil {
writeError(w, err)
return
Expand Down Expand Up @@ -1137,7 +1140,7 @@ func (h ddlHistoryJobHandler) ServeHTTP(w http.ResponseWriter, req *http.Request
}

func (h ddlHistoryJobHandler) getAllHistoryDDL() ([]*model.Job, error) {
s, err := session.CreateSession(h.Store.(kv.Storage))
s, err := session.CreateSessionWithOpt(h.Store.(kv.Storage), &session.Opt{DisableStatsCollector: true})
if err != nil {
return nil, errors.Trace(err)
}
Expand Down Expand Up @@ -1867,7 +1870,7 @@ func (h dbTableHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) {

// ServeHTTP handles request of TiDB metric profile.
func (h profileHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) {
sctx, err := session.CreateSession(h.Store)
sctx, err := session.CreateSessionWithOpt(h.Store, &session.Opt{DisableStatsCollector: true})
if err != nil {
writeError(w, err)
return
Expand Down
9 changes: 7 additions & 2 deletions session/session.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2065,7 +2065,8 @@ func CreateSession4Test(store kv.Storage) (Session, error) {

// Opt describes the option for creating session
type Opt struct {
PreparedPlanCache *kvcache.SimpleLRUCache
PreparedPlanCache *kvcache.SimpleLRUCache
DisableStatsCollector bool
}

// CreateSession4TestWithOpt creates a new session environment for test.
Expand Down Expand Up @@ -2104,9 +2105,13 @@ func CreateSessionWithOpt(store kv.Storage, opt *Opt) (Session, error) {

sessionBindHandle := bindinfo.NewSessionBindHandle(s.parser)
s.SetValue(bindinfo.SessionBindInfoKeyType, sessionBindHandle)
var disableStatsCollector bool
if opt != nil {
disableStatsCollector = opt.DisableStatsCollector
}
// Add stats collector, and it will be freed by background stats worker
// which periodically updates stats using the collected data.
if do.StatsHandle() != nil && do.StatsUpdating() {
if do.StatsHandle() != nil && do.StatsUpdating() && !disableStatsCollector {
s.statsCollector = do.StatsHandle().NewSessionStatsCollector()
if GetIndexUsageSyncLease() > 0 {
s.idxUsageCollector = do.StatsHandle().NewSessionIndexUsageCollector()
Expand Down