fixed timer queue processor goroutine leak in test #5857
Conversation
shijiesheng
requested review from
Shaddoll,
neil-xie,
davidporter-id-au,
Groxx,
agautam478,
jakobht,
3vilhamster,
sankari165,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
Codecov Report
Additional details and impacted files
... and 11 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| @@ -176,7 +176,10 @@ func (t *timerQueueProcessorBase) Start() { | |||
|
|
|||
| func (t *timerQueueProcessorBase) Stop() { | |||
| if !atomic.CompareAndSwapInt32(&t.status, common.DaemonStatusStarted, common.DaemonStatusStopped) { | |||
| return | |||
| // initialized should also be stopped to stop TimerGate | |||
| if !atomic.CompareAndSwapInt32(&t.status, common.DaemonStatusInitialized, common.DaemonStatusStopped) { | |||
There was a problem hiding this comment.
This is a good catch but instead of solving problem here we can fix TimerGate itself. It should have Start/Stop functions and shouldn't create any goroutine in constructor. That would be cleaner solution I think.
There was a problem hiding this comment.
I still think the code is buggy since lifecycle of timer gate is not handled correctly here.
- if you just initialize the timer queue processor without starting it, timer gate needs to be stopped separately
- if you start timer queue processor and stop it, timer gate is closed.
But to make minimum change. I'll handle this in the test case. The remaining concern is whether we do create timer gate without close it in the first scenario. But it should be addressed separately. I'll just add a comment.
|
|
||
| func TestTransferQueueProcessor_RequireStartStop(t *testing.T) { | ||
| // some goroutine leak not from this test | ||
| defer goleak.VerifyNone(t) |
There was a problem hiding this comment.
great idea to verify leaks in these critical components with bunch of underlying goroutines!
Pull Request Test Coverage Report for Build 018eaa23-245f-4679-971e-285ef54448f8Details
💛 - Coveralls |
shijiesheng
force-pushed
the
transfer-task-test
branch
from
ce0ca48 to
211f5ce
Compare
What changed?
Correctly stop timer queue processor on InitializedStateInstead, stop timergate in unit test only and added a comment for improvementWhy?
Previously in initialized state, stopping timer queue processor will not stop the timer gate which has a daemon loop; although this shall never happen in production, it's causing go routine leak in unit tests
How did you test it?
unit test
Potential risks
Release notes
Documentation Changes