Fix reschedule issue with same key (#17706)

Dimon4eg · minggo · commit a3cb7de10136 · 2017-05-04T16:24:20.000+08:00
* fix reschedule issue

* correct code formatting

* Fix isScheduled

* improve a bit performance of executing functions in cocos thread

* added test to verify reschedule issue

* re-init timer in schedule if timer exists and alive
diff --git a/cocos/base/CCScheduler.cpp b/cocos/base/CCScheduler.cpp
@@ -80,17 +80,17 @@ Timer::Timer()
 , _interval(0.0f)
 , _aborted(false)
 {
-
 }
 
 void Timer::setupTimerWithInterval(float seconds, unsigned int repeat, float delay)
 {
-	_elapsed = -1;
-	_interval = seconds;
-	_delay = delay;
-	_useDelay = (_delay > 0.0f) ? true : false;
-	_repeat = repeat;
-	_runForever = (_repeat == CC_REPEAT_FOREVER) ? true : false;
+    _elapsed = -1;
+    _interval = seconds;
+    _delay = delay;
+    _useDelay = (_delay > 0.0f) ? true : false;
+    _repeat = repeat;
+    _runForever = (_repeat == CC_REPEAT_FOREVER) ? true : false;
+    _timesExecuted = 0;
 }
 
 void Timer::update(float dt)
@@ -112,12 +112,12 @@ void Timer::update(float dt)
         {
             return;
         }
+        _timesExecuted += 1; // important to increment before call trigger
         trigger(_delay);
         _elapsed = _elapsed - _delay;
-        _timesExecuted += 1;
         _useDelay = false;
         // after delay, the rest time should compare with interval
-        if (!_runForever && _timesExecuted > _repeat)
+        if (isExhausted())
         {    //unschedule timer
             cancel();
             return;
@@ -128,11 +128,11 @@ void Timer::update(float dt)
     float interval = (_interval > 0) ? _interval : _elapsed;
     while ((_elapsed >= interval) && !_aborted)
     {
+        _timesExecuted += 1; // important to increment before call trigger
         trigger(interval);
         _elapsed -= interval;
-        _timesExecuted += 1;
 
-        if (!_runForever && _timesExecuted > _repeat)
+        if (isExhausted())
         {
             cancel();
             break;
@@ -145,6 +145,11 @@ void Timer::update(float dt)
     }
 }
 
+bool Timer::isExhausted() const
+{
+    return !_runForever && _timesExecuted > _repeat;
+}
+
 // TimerTargetSelector
 
 TimerTargetSelector::TimerTargetSelector()
@@ -312,12 +317,12 @@ void Scheduler::schedule(const ccSchedulerFunc& callback, void *target, float in
         {
             TimerTargetCallback *timer = dynamic_cast<TimerTargetCallback*>(element->timers->arr[i]);
 
-            if (timer && key == timer->getKey())
+            if (timer && !timer->isExhausted() && key == timer->getKey())
             {
-                CCLOG("CCScheduler#scheduleSelector. Selector already scheduled. Updating interval from: %.4f to %.4f", timer->getInterval(), interval);
-                timer->setInterval(interval);
+                CCLOG("CCScheduler#schedule. Reiniting timer with interval %.4f, repeat %u, delay %.4f", interval, repeat, delay);
+                timer->setupTimerWithInterval(interval, repeat, delay);
                 return;
-            }        
+            }
         }
         ccArrayEnsureExtraCapacity(element->timers, 1);
     }
@@ -512,22 +517,18 @@ bool Scheduler::isScheduled(const std::string& key, void *target)
     {
         return false;
     }
-    else
+    
+    for (int i = 0; i < element->timers->num; ++i)
     {
-        for (int i = 0; i < element->timers->num; ++i)
+        TimerTargetCallback *timer = dynamic_cast<TimerTargetCallback*>(element->timers->arr[i]);
+        
+        if (timer && !timer->isExhausted() && key == timer->getKey())
         {
-            TimerTargetCallback *timer = dynamic_cast<TimerTargetCallback*>(element->timers->arr[i]);
-            
-            if (timer && key == timer->getKey())
-            {
-                return true;
-            }
+            return true;
         }
-        
-        return false;
     }
     
-    return false;  // should never get here
+    return false;
 }
 
 void Scheduler::removeUpdateFromHash(struct _listEntry *entry)
@@ -734,11 +735,11 @@ bool Scheduler::isTargetPaused(void *target)
     }
     
     // We should check update selectors if target does not have custom selectors
-	tHashUpdateEntry *elementUpdate = nullptr;
-	HASH_FIND_PTR(_hashForUpdates, &target, elementUpdate);
-	if ( elementUpdate )
+    tHashUpdateEntry *elementUpdate = nullptr;
+    HASH_FIND_PTR(_hashForUpdates, &target, elementUpdate);
+    if ( elementUpdate )
     {
-		return elementUpdate->entry->paused;
+        return elementUpdate->entry->paused;
     }
     
     return false;  // should never get here
@@ -943,13 +944,12 @@ void Scheduler::update(float dt)
     if( !_functionsToPerform.empty() ) {
         _performMutex.lock();
         // fixed #4123: Save the callback functions, they must be invoked after '_performMutex.unlock()', otherwise if new functions are added in callback, it will cause thread deadlock.
-        auto temp = _functionsToPerform;
-        _functionsToPerform.clear();
+        auto temp = std::move(_functionsToPerform);
         _performMutex.unlock();
-        for( const auto &function : temp ) {
+        
+        for (const auto &function : temp) {
             function();
         }
-        
     }
 }
 
@@ -985,10 +985,10 @@ void Scheduler::schedule(SEL_SCHEDULE selector, Ref *target, float interval, uns
         {
             TimerTargetSelector *timer = dynamic_cast<TimerTargetSelector*>(element->timers->arr[i]);
             
-            if (timer && selector == timer->getSelector())
+            if (timer && !timer->isExhausted() && selector == timer->getSelector())
             {
-                CCLOG("CCScheduler#scheduleSelector. Selector already scheduled. Updating interval from: %.4f to %.4f", timer->getInterval(), interval);
-                timer->setInterval(interval);
+                CCLOG("CCScheduler#schedule. Reiniting timer with interval %.4f, repeat %u, delay %.4f", interval, repeat, delay);
+                timer->setupTimerWithInterval(interval, repeat, delay);
                 return;
             }
         }
@@ -1023,22 +1023,18 @@ bool Scheduler::isScheduled(SEL_SCHEDULE selector, Ref *target)
     {
         return false;
     }
-    else
+
+    for (int i = 0; i < element->timers->num; ++i)
     {
-        for (int i = 0; i < element->timers->num; ++i)
+        TimerTargetSelector *timer = dynamic_cast<TimerTargetSelector*>(element->timers->arr[i]);
+        
+        if (timer && !timer->isExhausted() && selector == timer->getSelector())
         {
-            TimerTargetSelector *timer = dynamic_cast<TimerTargetSelector*>(element->timers->arr[i]);
-            
-            if (timer && selector == timer->getSelector())
-            {
-                return true;
-            }
+            return true;
         }
-        
-        return false;
     }
     
-    return false;  // should never get here
+    return false;
 }
 
 void Scheduler::unschedule(SEL_SCHEDULE selector, Ref *target)
@@ -1060,7 +1056,7 @@ void Scheduler::unschedule(SEL_SCHEDULE selector, Ref *target)
             
             if (timer && selector == timer->getSelector())
             {
-                if (timer == element->currentTimer && (! timer->isAborted()))
+                if (timer == element->currentTimer && !timer->isAborted())
                 {
                     timer->retain();
                     timer->setAborted();
diff --git a/cocos/base/CCScheduler.h b/cocos/base/CCScheduler.h
@@ -50,14 +50,10 @@ class CC_DLL Timer : public Ref
 protected:
     Timer();
 public:
-    /** get interval in seconds */
-    float getInterval() const { return _interval; }
-    /** set interval in seconds */
-    void setInterval(float interval) { _interval = interval; }
-    
     void setupTimerWithInterval(float seconds, unsigned int repeat, float delay);
     void setAborted() { _aborted = true; }
     bool isAborted() const { return _aborted; }
+    bool isExhausted() const;
     
     virtual void trigger(float dt) = 0;
     virtual void cancel() = 0;
@@ -66,7 +62,6 @@ class CC_DLL Timer : public Ref
     void update(float dt);
     
 protected:
-    
     Scheduler* _scheduler; // weak ref
     float _elapsed;
     bool _runForever;
diff --git a/tests/cpp-tests/Classes/SchedulerTest/SchedulerTest.cpp b/tests/cpp-tests/Classes/SchedulerTest/SchedulerTest.cpp
@@ -1,8 +1,10 @@
 #include "SchedulerTest.h"
 #include "../testResource.h"
+#include "ui/UIText.h"
 
 USING_NS_CC;
 USING_NS_CC_EXT;
+using namespace cocos2d::ui;
 
 enum {
     kTagAnimationDance = 1,
@@ -26,6 +28,7 @@ SchedulerTests::SchedulerTests()
     ADD_TEST_CASE(RescheduleSelector);
     ADD_TEST_CASE(SchedulerDelayAndRepeat);
     ADD_TEST_CASE(SchedulerIssue2268);
+    ADD_TEST_CASE(SchedulerIssueWithReschedule);
     ADD_TEST_CASE(ScheduleCallbackTest);
     ADD_TEST_CASE(ScheduleUpdatePriority);
     ADD_TEST_CASE(SchedulerIssue10232);
@@ -983,6 +986,8 @@ std::string TwoSchedulers::subtitle() const
     return "Three schedulers. 2 custom + 1 default. Two different time scales";
 }
 
+// SchedulerIssue2268
+
 class TestNode2 : public Node
 {
 public:
@@ -1040,6 +1045,61 @@ std::string SchedulerIssue2268::subtitle() const
     return "Should not crash";
 }
 
+// SchedulerIssueWithReschedule
+// https://github.com/cocos2d/cocos2d-x/pull/17706
+
+void SchedulerIssueWithReschedule::onEnter()
+{
+	SchedulerTestLayer::onEnter();
+    
+    Size widgetSize = getContentSize();
+    
+    auto status_text = Text::create("Checking..", "fonts/Marker Felt.ttf", 18);
+    status_text->setColor(Color3B(255, 255, 255));
+    status_text->setPosition(Vec2(widgetSize.width / 2.0f, widgetSize.height / 2.0f));
+    addChild(status_text);
+    
+    // schedule(callback, target, interval, repeat, delay, paused, key);
+    auto verified = std::make_shared<bool>();
+    *verified = false;
+
+	_scheduler->schedule([this, verified](float dt){
+        log("SchedulerIssueWithReschedule - first timer");
+        
+        _scheduler->schedule([this, verified](float dt){
+            log("SchedulerIssueWithReschedule - second timer. OK");
+            *verified = true;
+        }, this, 0.1f, 0, 0, false, "test_timer");
+        
+    }, this, 0.1f, 0, 0, false, "test_timer");
+    
+    _scheduler->schedule([verified, status_text](float dt){
+        if (*verified)
+        {
+            log("SchedulerIssueWithReschedule - test OK");
+            status_text->setString("OK");
+            status_text->setColor(Color3B(0, 255, 0));
+        }
+        else
+        {
+            log("SchedulerIssueWithReschedule - test failed!");
+            status_text->setString("Failed");
+            status_text->setColor(Color3B(255, 0, 0));
+        }
+
+    }, this, 0.5f, 0, 0, false, "test_verify_timer");
+}
+
+std::string SchedulerIssueWithReschedule::title() const
+{
+    return "Issue with reschedule";
+}
+
+std::string SchedulerIssueWithReschedule::subtitle() const
+{
+    return "reschedule issue with same key";
+}
+
 // ScheduleCallbackTest
 
 ScheduleCallbackTest::~ScheduleCallbackTest()
diff --git a/tests/cpp-tests/Classes/SchedulerTest/SchedulerTest.h b/tests/cpp-tests/Classes/SchedulerTest/SchedulerTest.h
@@ -284,6 +284,16 @@ class SchedulerIssue2268 : public SchedulerTestLayer
     cocos2d::Node* testNode;
 };
 
+class SchedulerIssueWithReschedule : public SchedulerTestLayer
+{
+public:
+    CREATE_FUNC(SchedulerIssueWithReschedule);
+
+    virtual std::string title() const override;
+    virtual std::string subtitle() const override;
+    void onEnter() override;
+};
+
 class ScheduleCallbackTest : public SchedulerTestLayer
 {
 public:

Original file line number	Diff line number	Diff line change
`@@ -80,17 +80,17 @@ Timer::Timer()`
`80`	`80`	`, _interval(0.0f)`
`81`	`81`	`, _aborted(false)`
`82`	`82`	`{`
`83`		`-`
`84`	`83`	`}`
`85`	`84`
`86`	`85`	`void Timer::setupTimerWithInterval(float seconds, unsigned int repeat, float delay)`
`87`	`86`	`{`
`88`		`- _elapsed = -1;`
`89`		`- _interval = seconds;`
`90`		`- _delay = delay;`
`91`		`- _useDelay = (_delay > 0.0f) ? true : false;`
`92`		`- _repeat = repeat;`
`93`		`- _runForever = (_repeat == CC_REPEAT_FOREVER) ? true : false;`
	`87`	`+ _elapsed = -1;`
	`88`	`+ _interval = seconds;`
	`89`	`+ _delay = delay;`
	`90`	`+ _useDelay = (_delay > 0.0f) ? true : false;`
	`91`	`+ _repeat = repeat;`
	`92`	`+ _runForever = (_repeat == CC_REPEAT_FOREVER) ? true : false;`
	`93`	`+ _timesExecuted = 0;`
`94`	`94`	`}`
`95`	`95`
`96`	`96`	`void Timer::update(float dt)`
`@@ -112,12 +112,12 @@ void Timer::update(float dt)`
`112`	`112`	`{`
`113`	`113`	`return;`
`114`	`114`	`}`
	`115`	`+ _timesExecuted += 1; // important to increment before call trigger`
`115`	`116`	`trigger(_delay);`
`116`	`117`	`_elapsed = _elapsed - _delay;`
`117`		`- _timesExecuted += 1;`
`118`	`118`	`_useDelay = false;`
`119`	`119`	`// after delay, the rest time should compare with interval`
`120`		`- if (!_runForever && _timesExecuted > _repeat)`
	`120`	`+ if (isExhausted())`
`121`	`121`	`{ //unschedule timer`
`122`	`122`	`cancel();`
`123`	`123`	`return;`
`@@ -128,11 +128,11 @@ void Timer::update(float dt)`
`128`	`128`	`float interval = (_interval > 0) ? _interval : _elapsed;`
`129`	`129`	`while ((_elapsed >= interval) && !_aborted)`
`130`	`130`	`{`
	`131`	`+ _timesExecuted += 1; // important to increment before call trigger`
`131`	`132`	`trigger(interval);`
`132`	`133`	`_elapsed -= interval;`
`133`		`- _timesExecuted += 1;`
`134`	`134`
`135`		`- if (!_runForever && _timesExecuted > _repeat)`
	`135`	`+ if (isExhausted())`
`136`	`136`	`{`
`137`	`137`	`cancel();`
`138`	`138`	`break;`
`@@ -145,6 +145,11 @@ void Timer::update(float dt)`
`145`	`145`	`}`
`146`	`146`	`}`
`147`	`147`
	`148`	`+bool Timer::isExhausted() const`
	`149`	`+{`
	`150`	`+ return !_runForever && _timesExecuted > _repeat;`
	`151`	`+}`
	`152`	`+`
`148`	`153`	`// TimerTargetSelector`
`149`	`154`
`150`	`155`	`TimerTargetSelector::TimerTargetSelector()`
`@@ -312,12 +317,12 @@ void Scheduler::schedule(const ccSchedulerFunc& callback, void *target, float in`
`312`	`317`	`{`
`313`	`318`	`TimerTargetCallback timer = dynamic_cast<TimerTargetCallback>(element->timers->arr[i]);`
`314`	`319`
`315`		`- if (timer && key == timer->getKey())`
	`320`	`+ if (timer && !timer->isExhausted() && key == timer->getKey())`
`316`	`321`	`{`
`317`		`- CCLOG("CCScheduler#scheduleSelector. Selector already scheduled. Updating interval from: %.4f to %.4f", timer->getInterval(), interval);`
`318`		`- timer->setInterval(interval);`
	`322`	`+ CCLOG("CCScheduler#schedule. Reiniting timer with interval %.4f, repeat %u, delay %.4f", interval, repeat, delay);`
	`323`	`+ timer->setupTimerWithInterval(interval, repeat, delay);`
`319`	`324`	`return;`
`320`		`- }`
	`325`	`+ }`
`321`	`326`	`}`
`322`	`327`	`ccArrayEnsureExtraCapacity(element->timers, 1);`
`323`	`328`	`}`
`@@ -512,22 +517,18 @@ bool Scheduler::isScheduled(const std::string& key, void *target)`
`512`	`517`	`{`
`513`	`518`	`return false;`
`514`	`519`	`}`
`515`		`- else`
	`520`	`+`
	`521`	`+ for (int i = 0; i < element->timers->num; ++i)`
`516`	`522`	`{`
`517`		`- for (int i = 0; i < element->timers->num; ++i)`
	`523`	`+ TimerTargetCallback timer = dynamic_cast<TimerTargetCallback>(element->timers->arr[i]);`
	`524`	`+`
	`525`	`+ if (timer && !timer->isExhausted() && key == timer->getKey())`
`518`	`526`	`{`
`519`		`- TimerTargetCallback timer = dynamic_cast<TimerTargetCallback>(element->timers->arr[i]);`
`520`		`-`
`521`		`- if (timer && key == timer->getKey())`
`522`		`- {`
`523`		`- return true;`
`524`		`- }`
	`527`	`+ return true;`
`525`	`528`	`}`
`526`		`-`
`527`		`- return false;`
`528`	`529`	`}`
`529`	`530`
`530`		`- return false; // should never get here`
	`531`	`+ return false;`
`531`	`532`	`}`
`532`	`533`
`533`	`534`	`void Scheduler::removeUpdateFromHash(struct _listEntry *entry)`
`@@ -734,11 +735,11 @@ bool Scheduler::isTargetPaused(void *target)`
`734`	`735`	`}`
`735`	`736`
`736`	`737`	`// We should check update selectors if target does not have custom selectors`
`737`		`- tHashUpdateEntry *elementUpdate = nullptr;`
`738`		`- HASH_FIND_PTR(_hashForUpdates, &target, elementUpdate);`
`739`		`- if ( elementUpdate )`
	`738`	`+ tHashUpdateEntry *elementUpdate = nullptr;`
	`739`	`+ HASH_FIND_PTR(_hashForUpdates, &target, elementUpdate);`
	`740`	`+ if ( elementUpdate )`
`740`	`741`	`{`
`741`		`- return elementUpdate->entry->paused;`
	`742`	`+ return elementUpdate->entry->paused;`
`742`	`743`	`}`
`743`	`744`
`744`	`745`	`return false; // should never get here`
`@@ -943,13 +944,12 @@ void Scheduler::update(float dt)`
`943`	`944`	`if( !_functionsToPerform.empty() ) {`
`944`	`945`	`_performMutex.lock();`
`945`	`946`	`// fixed #4123: Save the callback functions, they must be invoked after '_performMutex.unlock()', otherwise if new functions are added in callback, it will cause thread deadlock.`
`946`		`- auto temp = _functionsToPerform;`
`947`		`- _functionsToPerform.clear();`
	`947`	`+ auto temp = std::move(_functionsToPerform);`
`948`	`948`	`_performMutex.unlock();`
`949`		`- for( const auto &function : temp ) {`
	`949`	`+`
	`950`	`+ for (const auto &function : temp) {`
`950`	`951`	`function();`
`951`	`952`	`}`
`952`		`-`
`953`	`953`	`}`
`954`	`954`	`}`
`955`	`955`
`@@ -985,10 +985,10 @@ void Scheduler::schedule(SEL_SCHEDULE selector, Ref *target, float interval, uns`
`985`	`985`	`{`
`986`	`986`	`TimerTargetSelector timer = dynamic_cast<TimerTargetSelector>(element->timers->arr[i]);`
`987`	`987`
`988`		`- if (timer && selector == timer->getSelector())`
	`988`	`+ if (timer && !timer->isExhausted() && selector == timer->getSelector())`
`989`	`989`	`{`
`990`		`- CCLOG("CCScheduler#scheduleSelector. Selector already scheduled. Updating interval from: %.4f to %.4f", timer->getInterval(), interval);`
`991`		`- timer->setInterval(interval);`
	`990`	`+ CCLOG("CCScheduler#schedule. Reiniting timer with interval %.4f, repeat %u, delay %.4f", interval, repeat, delay);`
	`991`	`+ timer->setupTimerWithInterval(interval, repeat, delay);`
`992`	`992`	`return;`
`993`	`993`	`}`
`994`	`994`	`}`
`@@ -1023,22 +1023,18 @@ bool Scheduler::isScheduled(SEL_SCHEDULE selector, Ref *target)`
`1023`	`1023`	`{`
`1024`	`1024`	`return false;`
`1025`	`1025`	`}`
`1026`		`- else`
	`1026`	`+`
	`1027`	`+ for (int i = 0; i < element->timers->num; ++i)`
`1027`	`1028`	`{`
`1028`		`- for (int i = 0; i < element->timers->num; ++i)`
	`1029`	`+ TimerTargetSelector timer = dynamic_cast<TimerTargetSelector>(element->timers->arr[i]);`
	`1030`	`+`
	`1031`	`+ if (timer && !timer->isExhausted() && selector == timer->getSelector())`
`1029`	`1032`	`{`
`1030`		`- TimerTargetSelector timer = dynamic_cast<TimerTargetSelector>(element->timers->arr[i]);`
`1031`		`-`
`1032`		`- if (timer && selector == timer->getSelector())`
`1033`		`- {`
`1034`		`- return true;`
`1035`		`- }`
	`1033`	`+ return true;`
`1036`	`1034`	`}`
`1037`		`-`
`1038`		`- return false;`
`1039`	`1035`	`}`
`1040`	`1036`
`1041`		`- return false; // should never get here`
	`1037`	`+ return false;`
`1042`	`1038`	`}`
`1043`	`1039`
`1044`	`1040`	`void Scheduler::unschedule(SEL_SCHEDULE selector, Ref *target)`
`@@ -1060,7 +1056,7 @@ void Scheduler::unschedule(SEL_SCHEDULE selector, Ref *target)`
`1060`	`1056`
`1061`	`1057`	`if (timer && selector == timer->getSelector())`
`1062`	`1058`	`{`
`1063`		`- if (timer == element->currentTimer && (! timer->isAborted()))`
	`1059`	`+ if (timer == element->currentTimer && !timer->isAborted())`
`1064`	`1060`	`{`
`1065`	`1061`	`timer->retain();`
`1066`	`1062`	`timer->setAborted();`