[
https://issues.apache.org/jira/browse/THRIFT-5881?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18118175#comment-18118175
]
Jens Geyer commented on THRIFT-5881:
------------------------------------
The leak is confirmed. The fix is to chain up to the parent class's finalize,
not to clear the instance data by hand.
The finalize function the c_glib generator writes for structs and exceptions
(and for the argument and result structs of every service method) frees the
members, but never calls the parent class's finalize. So GObject's own
finalize, {{g_object_finalize()}}, never runs for these objects. That is the
function that calls {{g_datalist_clear()}} on the instance data, so everything
kept there is lost: data attached with {{g_object_set_data_full()}}, and the
data {{g_object_unref()}} itself stores there while it disposes of the object.
Measured with GLib 2.72.4 (Ubuntu 22.04) under valgrind: creating and releasing
a generated struct 1,000 times lost 48,000 bytes in 1,000 blocks, all allocated
inside {{g_object_unref()}}. With the fix, nothing is lost.
h4. Why not {{g_datalist_clear()}} in the generated finalize
* Chaining up is what GObject expects of an overridden finalize. It runs
whatever the finalize functions further up the chain do, now and in future GLib
versions. A hand-written {{g_datalist_clear()}} repeats one step of
{{g_object_finalize()}} and reaches into the instance's internals; anything
else in that chain, including a finalize added to {{ThriftStruct}} later, would
still be skipped.
* It would not cover the library classes, which have the same gap (see below).
h4. The change
The generated code now does what {{G_DEFINE_TYPE}} does for a hand-written
class: it keeps the parent class in a static variable, sets it in class_init
and chains up at the end of finalize:
{code:c}
static gpointer t_test_xtruct_parent_class = NULL;
static void
t_test_xtruct_finalize (GObject *object)
{
/* ... free the members, as before ... */
G_OBJECT_CLASS (t_test_xtruct_parent_class)->finalize (object);
}
static void
t_test_xtruct_class_init (TTestXtructClass * cls)
{
/* ... */
t_test_xtruct_parent_class = g_type_class_peek_parent (cls);
/* ... */
}
{code}
h4. The same gap in the library
Eight hand-written classes override finalize without chaining up:
{{ThriftApplicationException}}, {{ThriftBufferedTransport}},
{{ThriftFDTransport}}, {{ThriftFramedTransport}}, {{ThriftMemoryBuffer}},
{{ThriftServerSocket}}, {{ThriftSocket}} and {{ThriftZlibTransport}}.
{{ThriftSSLSocket}} chains up correctly, but into {{ThriftSocket}}'s finalize,
so its data was lost as well: 72 bytes for a single instance in the same
measurement, none after the fix. The generated clients, handlers and
processors, and the remaining library classes, already chain up or do not
override finalize. The library classes are fixed under this ticket too, so it
now also carries the C glib - Library component.
A new test, {{testfinalize}}, and a new case in {{testzlibtransport}} attach
data with a destroy notification to an object, drop the last reference and
check that the notification ran. They fail without the fix and pass with it.
The change is in [PR #3934|https://github.com/apache/thrift/pull/3934].
_Drafted with AI assistance (Claude Opus 5.5); reviewed and posted by Jens
Geyer._
> [Glib] The {class}_finalize method created by c_glib generator is leaking
> memory
> --------------------------------------------------------------------------------
>
> Key: THRIFT-5881
> URL: https://issues.apache.org/jira/browse/THRIFT-5881
> Project: Thrift
> Issue Type: Bug
> Components: C glib - Compiler, C glib - Library
> Reporter: Kevin Rodgers
> Priority: Major
> Time Spent: 10m
> Remaining Estimate: 0h
>
> In newer versions of glib the {{g_object_unref}} call has been updated to
> freeze the object’s notification queue. This causes memory to be allocated
> and saved in
> [object->qdata|https://github.com/GNOME/glib/blob/c1e8174992182d7a6e6499ea9ab2cf76ad81a3ca/gobject/gobject.c#L4828].This
> memory is freed when the notification queue is thawed, which usually happens
> in the \{class}_finalize method call of the object’s parent class.
> In the thrift generated finalize method there is no call to finalize the
> parent object and the memory that has been allocated to freeze notifications
> is lost. This causes the process size to increase as objects are created and
> destroyed. I have fixed this in my environment by updating
> [t_c_glib_generator.cc|https://github.com/apache/thrift/blob/master/compiler/cpp/src/thrift/generate/t_c_glib_generator.cc#L3306]{color:#172b4d}
> to call g_datalist_clear{color} at the very end of each \{class}_finalize
> method.
> {{Example:}}
> static void
> {{test_object_finalize (GObject *object)}}
> {
> {{ TestObject *tobject = TEST_OBJECT (object);}}
> {{ /* satisfy -Wall in case we don't use tobject */}}
> {{ THRIFT_UNUSED_VAR (tobject);}}
> {{ if (tobject->versions != NULL)}}
> {{ {}}
> {{ g_free(tobject->versions);}}
> {{ tobject->versions = NULL;}}
> }
> {{ g_datalist_clear(&object->qdata); // added to free memory}}
> {{}}}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
