Fix a potential memory leak in schedulePeriodically #2642
Conversation
| @@ -119,11 +120,17 @@ public void call() { | |||
| if (!mas.isUnsubscribed()) { | |||
| action.call(); | |||
| long nextTick = startInNanos + (++count * periodInNanos); | |||
| mas.set(schedule(this, nextTick - TimeUnit.MILLISECONDS.toNanos(now()), TimeUnit.NANOSECONDS)); | |||
| SerialSubscription s = new SerialSubscription(); | |||
There was a problem hiding this comment.
I think there is no need to change this here: the initial SerialSubscription can safely overwritted by the result of the schedule as it is not reentrant.
There was a problem hiding this comment.
Do you mean if period is large, the new Subscription created in the next action won't be able to replace the one here, and if period is small, the leak won't be a problem?
Just to make sure we are in the same page.
There was a problem hiding this comment.
No. I mean once the action is executed on the worker, each invocation of call is guaranteed to be serial, therefore, there is no need to have another level of SerialSubscription indirection.
|
It is functionally mergeable, but you could have used |
Fix a potential memory leak in schedulePeriodically
There is a potential memory leak in
schedulePeriodicallythat may keep a reference toactionafterunsubscribe.Because
mas.setis called afterschedule, it may replace a new Subscription (created inrecursiveAction) with the old one. Therefore,unsubscribewon't be able to unsubscribe the new Subscription and will keep the reference toactionuntil the period time elapses.This PR fixed it by calling
mas.setbeforeschedule.