domain: fix memory leak for stats #7864
Conversation
| @@ -1066,7 +1066,7 @@ func CreateSession(store kv.Storage) (Session, error) { | |||
| privilege.BindPrivilegeManager(s, pm) | |||
|
|
|||
| // Add statsUpdateHandle. | |||
| if do.StatsHandle() != nil { | |||
| if do.StatsHandle() != nil && do.StatsUpdating() { | |||
There was a problem hiding this comment.
maybe it's better to add a comment to specify that the data collected by statsCollector is consumed by the background stats worker which periodically update stats using the collected data.
|
@winoros If we use a new goroutine, it may still panic. |
There was a problem hiding this comment.
LGTM.
BTW, seems the existing SessionStatsCollectors would not be freed when the statsWorker panic. I guess it is because existing sessions may still write into this SessionStatsCollector. The problem is that these memory would never get chance to be freed then?
Maybe the session should check if it has SessionStatsCollector allocated while the statsWorker has gone, if so, the session should be responsible for freeing the memory when exiting. Anyway, that should be addressed in another issue I guess.
|
/run-all-tests |
eurekaka
added
status/all tests passed
status/LGT3
What problem does this PR solve?
When the stats-lease is 0 or stats updating worker panics, the memory usage of stats will increase as long as there are new sessions.
What is changed and how it works?
When there is a new session, we will create a session stats collector, and it will be freed by the background stats worker when the session exits. Now only when the stats worker exists, which means the stats lease is not zero and the stats worker is running, we will create a new session stats collector.
Check List
Tests
Code changes
Side effects
Related changes
PTAL @zz-jason @winoros @eurekaka