Bug 71498 - PackageKit is leaking ~ 10 MiB for each created thread
Summary: PackageKit is leaking ~ 10 MiB for each created thread
Status: RESOLVED FIXED
Alias: None
Product: PackageKit
Classification: Unclassified
Component: core (show other bugs)
Version: unspecified
Hardware: All All
: medium normal
Assignee: Richard Hughes
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-11-11 15:09 UTC by Thomas Perl
Modified: 2013-11-11 16:37 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
Proposed patch -- Call g_thread_unref() on the return value (2.74 KB, patch)
2013-11-11 15:12 UTC, Thomas Perl
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Thomas Perl 2013-11-11 15:09:15 UTC
In src/pk-backend-job.c, the call to g_thread_new() returns a GThread * object that has a reference count of 1 that needs to be unreferenced, otherwise the GThread * object will never be freed. This leaks ~ 10 MiB per created thread in packagekitd while packagekit is running.

This can be fixed by doing a g_thread_unref() on the returned value - as long as the thread is running, it will keep its own reference, so calling g_thread_unref() right after creating the thread is safe.

This problem wasn't happening for the code path for GLib < 2.31, as there, the "joinable" parameter to g_thread_create() was FALSE, which means it didn't return a GThread * object. On the other hand, it also didn't return 

Docs:
 - https://developer.gnome.org/glib/2.37/glib-Threads.html#g-thread-new
 - https://developer.gnome.org/glib/2.37/glib-Deprecated-Thread-APIs.html#g-thread-create

Related to this, for the GLib < 2.31 case, the returned value didn't have a reference, so the memory leak didn't happen there. But as we were keeping the pointer to the return value anyway, this was probably a bad idea (from the docs: "If joinable is FALSE then you should probably not touch the return value."). We didn't use the return value, so instead of managing it and its reference, we just keep the GThread * local, have a "has_thread" boolean on the job's private data to make sure it only has one thread and set joinable to TRUE for g_thread_create() as well, and then just g_thread_unref() the thread in all cases.

I'll attach a proposed patch.
Comment 1 Thomas Perl 2013-11-11 15:12:21 UTC
Created attachment 89034 [details] [review]
Proposed patch -- Call g_thread_unref() on the return value

Proposed patch - also changed the "GThread *" to just a gboolean to check if we have created a thread at some point, as the object referenced by the "GThread *" will in general not be accessible after g_thread_unref() is called (it will be removed as soon as the thread exits).
Comment 2 Richard Hughes 2013-11-11 16:37:32 UTC
I've fixed this in a slightly different way:

commit 6e582a38ce6da13fda74cfabb41865eabd57dbf0
Author: Richard Hughes <richard@hughsie.com>
Date:   Mon Nov 11 16:36:37 2013 +0000

    Fix memory leak when using new versions of GLib
    
    Unref the thread as the thread has a reference to itself.
    
    Resolves: https://bugs.freedesktop.org/show_bug.cgi?id=71498

Thanks for hunting this one down!


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.