GNOME Bugzilla – Bug 711696
Leaking timerfds when we fall back to polling the ccache
Last modified: 2013-11-12 16:04:21 UTC
Looks similar to bug 691142 on the outside, but I think this only manifests itself when we poll the credentials cache because we can not monitor it. When we poll we keep pushing the renewal_time forward [1] which causes renewal_alarm to be reset. That leads us to goa_alarm_set_time [2] where we cancel the old self->priv->cancellable and replace it with a new one. This leads to clear_scheduled_wakeups being run in a idle callback, however before that can run we call schedule_wakeups. This causes the timer.stream to be leaked. I am suspicious of clear_scheduled_wakeups because it again clears self->priv->cancellable and self->priv->context, which in this case are the new ones set by goa_alarm_set_time. Maybe we should move them to dispose? Looks like we tackled a similar problem in bug 708975 Similar to what we did there we need to somehow tag the old stream and source that need to be cleaned in on_cancelled. [1] https://git.gnome.org/browse/gnome-online-accounts/tree/src/goaidentity/goakerberosidentity.c#n867 [2] https://git.gnome.org/browse/gnome-online-accounts/tree/src/goaidentity/goaalarm.c#n606 [3] https://git.gnome.org/browse/gnome-online-accounts/tree/src/goaidentity/goaalarm.c#n134
Created attachment 259293 [details] [review] alarm: Do not clear the wrong objects when setting the time
Created attachment 259294 [details] [review] alarm: The global default main context is always the same
Created attachment 259560 [details] [review] alarm: Use the same GSource pointer for TIMER and TIMEOUT alarms
Created attachment 259561 [details] [review] alarm: Tag the "cancelled" handler with the correct source and stream
Created attachment 259562 [details] [review] alarm: Remove redundant preprocessor conditional
Review of attachment 259562 [details] [review]: ++
Review of attachment 259293 [details] [review]: so this should be squashed with the next attachment right?
Review of attachment 259294 [details] [review]: ::: src/goaidentity/goaalarm.c @@ +645,3 @@ self->priv->time = time; + if (self->priv->context != NULL) shouldn't that be == NULL?
Review of attachment 259560 [details] [review]: ::: src/goaidentity/goaalarm.c @@ +61,3 @@ GoaAlarmType type; + GSource *scheduled_wakeup_source; + GInputStream *stream; should have a comment above this that says somethine like /* NULL unless using timerfd() */ @@ +97,3 @@ gboolean is_closed; + g_clear_pointer (&self->priv->scheduled_wakeup_source, (GDestroyNotify) g_source_destroy); Since we're using the same source variable now for both timer/timeout codepaths it may make sense to consolidate clear_scheduled_time*_wakeups into one function.
Review of attachment 259561 [details] [review]: ::: src/goaidentity/goaalarm.c @@ +258,3 @@ + self = g_task_get_source_object (task); + source = (GSource *) g_object_get_data (G_OBJECT (task), "alarm-scheduled-wakeup-source"); + stream = G_INPUT_STREAM (g_object_get_data (G_OBJECT (task), "alarm-stream")); This is going to spew to the console if timerfd isn't compiled in right? (since alarm-stream will be NULL)
(In reply to comment #7) > Review of attachment 259293 [details] [review]: > > so this should be squashed with the next attachment right? Yes. I kept them separate for the purposes of the review.
(In reply to comment #8) > Review of attachment 259294 [details] [review]: > > ::: src/goaidentity/goaalarm.c > @@ +645,3 @@ > self->priv->time = time; > > + if (self->priv->context != NULL) > > shouldn't that be == NULL? Yes, ofcourse.
(In reply to comment #10) > Review of attachment 259561 [details] [review]: > > ::: src/goaidentity/goaalarm.c > @@ +258,3 @@ > + self = g_task_get_source_object (task); > + source = (GSource *) g_object_get_data (G_OBJECT (task), > "alarm-scheduled-wakeup-source"); > + stream = G_INPUT_STREAM (g_object_get_data (G_OBJECT (task), > "alarm-stream")); > > This is going to spew to the console if timerfd isn't compiled in right? (since > alarm-stream will be NULL) Yes, you are right.
Created attachment 259586 [details] [review] alarm: The global default main context is always the same
Created attachment 259587 [details] [review] alarm: Use the same GSource pointer for TIMER and TIMEOUT alarms
Created attachment 259588 [details] [review] alarm: Tag the "cancelled" handler with the correct source and stream
(In reply to comment #9) > @@ +97,3 @@ > gboolean is_closed; > > + g_clear_pointer (&self->priv->scheduled_wakeup_source, (GDestroyNotify) > g_source_destroy); > > Since we're using the same source variable now for both timer/timeout codepaths > it may make sense to consolidate clear_scheduled_time*_wakeups into one > function. Yes. My quick attempt at this is spewing some CRITICALs to the terminal, and I got to run now. Will attach something tomorrow morning.
Created attachment 259645 [details] [review] alarm: Consolidate clear_scheduled_time*_wakeups into one function
Review of attachment 259588 [details] [review]: This seems okay (modulo minor buglet mentioned below). Now that we can have overlapping streams/sources I wonder if the stream, source, and cancellable should be moved out of the main GoaAlarmPrivate and be stored entirely in task specific data, but that's maybe a cleanup for another day. ::: src/goaidentity/goaalarm.c @@ +255,3 @@ + GSource *source; + GTask *task = G_TASK (user_data); + gpointer *task_data; gpointer not gpointer * @@ +260,3 @@ + source = (GSource *) g_object_get_data (G_OBJECT (task), "alarm-scheduled-wakeup-source"); + task_data = g_object_get_data (G_OBJECT (task), "alarm-stream"); + stream = (task_data == NULL) ? NULL : G_INPUT_STREAM (task_data); this is fine, but i think it would be a little cleaner to pass the type in as object data as well and then only set stream if the type is the kind that has a stream. That would allow you to keep the G_INPUT_STREAM runtime check for the timerfd code-path in case things go wonky and the stream somehow gets nullified. Anyway it doesn't really matter either way.
Review of attachment 259645 [details] [review]: ++
(In reply to comment #19) > Review of attachment 259588 [details] [review]: > > This seems okay (modulo minor buglet mentioned below). Now that we can have > overlapping streams/sources I wonder if the stream, source, and cancellable > should be moved out of the main GoaAlarmPrivate and be stored entirely in task > specific data, but that's maybe a cleanup for another day. > > ::: src/goaidentity/goaalarm.c > @@ +255,3 @@ > + GSource *source; > + GTask *task = G_TASK (user_data); > + gpointer *task_data; > > gpointer not gpointer * Oops! Fixed. > @@ +260,3 @@ > + source = (GSource *) g_object_get_data (G_OBJECT (task), > "alarm-scheduled-wakeup-source"); > + task_data = g_object_get_data (G_OBJECT (task), "alarm-stream"); > + stream = (task_data == NULL) ? NULL : G_INPUT_STREAM (task_data); > > this is fine, but i think it would be a little cleaner to pass the type in as > object data as well and then only set stream if the type is the kind that has a > stream. That would allow you to keep the G_INPUT_STREAM runtime check for the > timerfd code-path in case things go wonky and the stream somehow gets > nullified. Anyway it doesn't really matter either way. I decided not to use the GoaAlarmType because now that we are using the same GSource * for both TIMER and TIMEOUT alarms, we might be able to get away with the type being a binary (scheduled / unscheduled) instead of a tri-state.
Created attachment 259670 [details] [review] alarm: Tag the "cancelled" handler with the correct source and stream
Committed to both master and gnome-3-10. Will backport to gnome-3-8 after some more testing.