GNOME Bugzilla – Bug 729718
Dramatically simplify alarm logic to stop timerfd leaks
Last modified: 2014-05-13 14:15:24 UTC
There is a still a timerfd leak going on.
Created attachment 276071 [details] [review] goaalarm: assume clear_scheduled_wakeups will only be called with a valid timer Right now, clear_scheduled_wakeups will only clear the timer source passed in, if there is a timer source scheduled. But the timer source passed in may have nothing to do with the currently scheduled timer, so move that check to the caller of the function.
Created attachment 276072 [details] [review] Don't mangle state of attached timer when closing detached timer Because we defer cleanup of timer sources until idle, we may have one or more detached timers "in flight" waiting to be closed. Right now, the function that closes the timers, also clears the state about the currently attached timer, whic means when an stale timer gets closed, we end up leaking the attached timer. This commit changes the function that closes timers, to only clear state about the currently attached timer, if the timer being closed is the currently attached timer.
Created attachment 276073 [details] [review] goaalarm: nullify attached timer object when it gets closed Right now, after closing a timer source, we leave it's pointer dangling. This commit fixes that.
Review of attachment 276071 [details] [review]: Makes sense.
Review of attachment 276072 [details] [review]: ::: src/goaidentity/goaalarm.c @@ +91,3 @@ + + if (self->priv->scheduled_wakeup_source == source) + is_attached = TRUE; this needs to set is_attached to FALSE in the else case
Created attachment 276099 [details] [review] goaalarm: assume clear_scheduled_wakeups will only be called with a valid timer Right now, clear_scheduled_wakeups will only clear the timer source passed in, if there is a timer source scheduled. But the timer source passed in may have nothing to do with the currently scheduled timer, so move that check to the caller of the function.
Created attachment 276100 [details] [review] goaalarm: don't mangle state of attached timer when closing detached timer Because we defer cleanup of timer sources until idle, we may have one or more detached timers "in flight" waiting to be closed. Right now, the function that closes the timers, also clears the state about the currently attached timer, whic means when an stale timer gets closed, we end up leaking the attached timer. This commit changes the function that closes timers, to only clear state about the currently attached timer, if the timer being closed is the currently attached timer.
Created attachment 276101 [details] [review] goaalarm: nullify attached timer object when it gets closed Right now, after closing a timer source, we leave it's pointer dangling. This commit fixes that.
Created attachment 276102 [details] [review] goaalarm: create task while still hold lock The on_cancelled handler may be called outside of the lock, if the cancellable is cancelled directly, rather than via set_time. Since it's not called under the lock, it's not "safe" to examine the active timer source. This commit moves construction of the task for the active timer source to set_time, where it's safely under the lock.
Created attachment 276103 [details] [review] goaalarm: don't dispatch handlers for stale timers If a handler is on life support just waiting to be destroyed, we shouldn't keep using it.
I think we need to rethink the approach here. The code is way more complicated than it needs to be and these patches only make it more complicated. Furthermore, I'm not convinced the problem is completely fixed, even with the patches. Let me attach a different kind of fix that should mop this mess up in a better way.
Created attachment 276187 [details] [review] identity: dramatically simplify alarm logic The GoaAlarm code is unwieldy. Primarily this is because it supports resetting the alarm after creation, but can't clean up old state right away due to a bug in GLib (bug 705395). All this complexitly is leading to bugs, and caos. This commit introduces some zen to the situation by making GoaAlarm immutable and much simpler. Now to reset an alarm, the identity code just instantiates a new one and destroys the old one.
will post one more iteration to address a leak fix
Created attachment 276237 [details] [review] identity: dramatically simplify alarm logic The GoaAlarm code is unwieldy. Primarily this is because it supports resetting the alarm after creation, but can't clean up old state right away due to a bug in GLib (bug 705395). All this complexitly is leading to bugs, and caos. This commit introduces some zen to the situation by making GoaAlarm immutable and much simpler. Now to reset an alarm, the identity code just instantiates a new one and destroys the old one.
and one more iteration to make it apply now that bug 729864 has landed
Created attachment 276240 [details] [review] identity: dramatically simplify alarm logic The GoaAlarm code is unwieldy. Primarily this is because it supports resetting the alarm after creation, but can't clean up old state right away due to a bug in GLib (bug 705395). All this complexitly is leading to bugs, and caos. This commit introduces some zen to the situation by making GoaAlarm immutable and much simpler. Now to reset an alarm, the identity code just instantiates a new one and destroys the old one.
Review of attachment 276240 [details] [review]: Yes, looks good. Thanks for the patch, Ray!
Attachment 276240 [details] pushed as b2f1b8a - identity: dramatically simplify alarm logic