Merge lp:~vanvugt/compiz-core/fix-timer-infinite-loop into lp:compiz-core/0.9.5

[Daniel van Vugt]

Fix infinite loop in CompTimeoutSource::callback which hangs compiz.

The loop was growing the timers list at the same rate it was shrinking, so of course the loop never finished if any timer had minTime <= 0.

I have not seen this happen in the wild, but a quick grep shows it can in theory happen with the obs and regex plugins. Also, src/window.cpp contains comments with respect to bugs in setTimes(0, 0), which could be the same issue.

[Daniel van Vugt (vanvugt)]

To clarify, I _have_ seen the infinite loop happen and compiz hang in my own branch where I called setTimes(0, 0). But I haven't seen it happen in a release.

[Daniel van Vugt (vanvugt)]

This fix also affects regular non-zero timers. If for example you have a system that is very slow at rendering (or just a slow plugin) and takes up the whole frame-time just to render, then the same infinite loop could occur. This fix prevents it from occurring so makes glib event scheduling across the whole compiz process more fair.

[Sam Spilsbury (smspillaz)]

Hey Daniel,

Thank you for the fix to what looks like it was quite a complicated problem! I have been trying to debug some of the inaccuracies with the timers for some time now.

Could you attach a simple plugin to this which shows the old behaviour happening? OR even better, would be to add a unit test in tests/timer/

Cheers,

Sam

review: Needs Information

[Daniel van Vugt (vanvugt)]

A test case without X events happening is a bit tricky. But I think something like creating 2 timers:
(a) interval 0
(b) interval 500
And then waiting 2000ms, verifying that (b) has been called >= 2 times, should work. With the bug, (a) will always stay in front of (b) so (b) is never called.

I'll look at doing this soon.

On a related note, is there a good reason why CompTimer and the associated classes are so complex? I tried rewriting CompTimer with g_timeout_add_full, and deleted the associated timeoutsource/handler classes, and found it performed much better (due to better pre-emption - other events can be handled between timer callbacks) and was much smaller...

[Sam Spilsbury (smspillaz)]

On Sat, Nov 12, 2011 at 12:14 PM, Daniel van Vugt <email address hidden> wrote:

> A test case without X events happening is a bit tricky. But I think
> something like creating 2 timers:
> (a) interval 0
> (b) interval 500
> And then waiting 2000ms, verifying that (b) has been called >= 2 times,
> should work. With the bug, (a) will always stay in front of (b) so (b) is
> never called.
>
> I'll look at doing this soon.
>
> On a related note, is there a good reason why CompTimer and the associated
> classes are so complex? I tried rewriting CompTimer with
> g_timeout_add_full, and deleted the associated timeoutsource/handler
> classes, and found it performed much better (due to better pre-emption -
> other events can be handled between timer callbacks) and was much smaller...
>

Yes, ordering becomes a problem when timers change their timeout times
while the timer is already "started" (the composite plugin was doing this
and it caused redraw issues)

> --
>
> https://code.launchpad.net/~vanvugt/compiz-core/fix-timer-infinite-loop/+merge/81944
> You are reviewing the proposed merge of
> lp:~vanvugt/compiz-core/fix-timer-infinite-loop into lp:compiz-core.
>

--
Sam Spilsbury

[Sam Spilsbury (smspillaz)]

On Fri, Nov 18, 2011 at 7:01 PM, Sam Spilsbury <email address hidden> wrote:

>
>
> On Sat, Nov 12, 2011 at 12:14 PM, Daniel van Vugt <email address hidden>wrote:
>
>> A test case without X events happening is a bit tricky. But I think
>> something like creating 2 timers:
>> (a) interval 0
>> (b) interval 500
>> And then waiting 2000ms, verifying that (b) has been called >= 2 times,
>> should work. With the bug, (a) will always stay in front of (b) so (b) is
>> never called.
>>
>> I'll look at doing this soon.
>>
>> On a related note, is there a good reason why CompTimer and the
>> associated classes are so complex? I tried rewriting CompTimer with
>> g_timeout_add_full, and deleted the associated timeoutsource/handler
>> classes, and found it performed much better (due to better pre-emption -
>> other events can be handled between timer callbacks) and was much smaller...
>>
>
> Yes, ordering becomes a problem when timers change their timeout times
> while the timer is already "started" (the composite plugin was doing this
> and it caused redraw issues).
>

Well, to clarify on this point - GLib doesn't have a concept of "changing"
a timeout time while a timer is already running. You can only remove a
timer and re-add it with the new timeout time. If you had 4 timers with the
same timeout time (eg, zero or something) then this would place the newly
added timer at the back of the queue.

>
>
>> --
>>
>> https://code.launchpad.net/~vanvugt/compiz-core/fix-timer-infinite-loop/+merge/81944
>> You are reviewing the proposed merge of
>> lp:~vanvugt/compiz-core/fix-timer-infinite-loop into lp:compiz-core.
>>
>
>
>
> --
> Sam Spilsbury
>

--
Sam Spilsbury

Unmerged revisions

2894. By Daniel van Vugt on 2011-11-11

Fix infinite loop in CompTimeoutSource::callback which hangs compiz.

The loop was growing the timers list at the same rate it was shrinking,
so of course the loop never finished if any timer had minTime <= 0.

=== modified file 'src/timer.cpp'
--- src/timer.cpp	2011-07-05 19:15:24 +0000
+++ src/timer.cpp	2011-11-11 07:33:29 +0000
@@ -152,21 +152,29 @@
 bool
 CompTimeoutSource::callback ()
 {
-    while (TimeoutHandler::Default ()->timers ().begin () != TimeoutHandler::Default ()->timers ().end () &&
-	   TimeoutHandler::Default ()->timers ().front ()->minLeft () <= 0)
+    TimeoutHandler *handler = TimeoutHandler::Default ();
+    std::list<CompTimer*> requeue, &timers = handler->timers ();
+
+    while (!timers.empty ())
     {
-	CompTimer *t = TimeoutHandler::Default ()->timers ().front ();
-	TimeoutHandler::Default ()->timers ().pop_front ();
-
+	CompTimer *t = timers.front ();
+	if (t->minLeft () > 0)
+	    break;
+	timers.pop_front ();
 	t->setActive (false);
 	if (t->triggerCallback ())
-	{
-	    TimeoutHandler::Default ()->addTimer (t);
-	    t->setActive (true);
-	}
-    }
-
-    return !TimeoutHandler::Default ()->timers ().empty ();
+	    requeue.push_back (t);
+    }
+
+    std::list<CompTimer*>::const_iterator i = requeue.begin ();
+    for (; i != requeue.end (); i++)
+    {
+	CompTimer *t = *i;
+	handler->addTimer (t);
+	t->setActive (true);
+    }
+
+    return !timers.empty ();
 }
 
 PrivateTimer::PrivateTimer () :