After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 630271 - pygi: always free the invocation_state struct
pygi: always free the invocation_state struct
Product: pygobject
Classification: Bindings
Component: introspection
Other All
: Normal normal
: ---
Assigned To: Nobody's working on this now (help wanted and appreciated)
Python bindings maintainers
Depends on:
Reported: 2010-09-21 16:13 UTC by Damien Caliste
Modified: 2010-11-04 21:58 UTC
See Also:
GNOME target: ---
GNOME version: ---

pygi: always free the invocation_state struct (7.09 KB, patch)
2010-09-21 16:13 UTC, Damien Caliste
committed Details | Review
Remove the tabs from the previous patch (320 bytes, patch)
2010-09-23 16:10 UTC, Damien Caliste
committed Details | Review

Description Damien Caliste 2010-09-21 16:13:16 UTC
  Using PyGI in my code, I notice that each function call costs memory
that is not released after. In my opinion,
the function _wrap_g_function_info_invoke() does not free memory
allocated in the structure "state" in case of working status, while
_free_invocation_state() is indeed call in case of error. I suggest to
call the _free routine before the return line, as done in the attached
Comment 1 Damien Caliste 2010-09-21 16:13:18 UTC
Created attachment 170765 [details] [review]
pygi: always free the invocation_state struct

In pygi-invoke.c, the invocation_state struct is never freed
in case of success. Thus, always call _free_invocation_state()
before leaving.
Modify _free_invocation_state to avoid double free in case of
caller-allocated GValue, once as a released argument in the
_process routine and another time in the _free as the special
case. So move all argument releasing code from the _process
routine to the _free one.
Modify the tests for the callback routines to return an integer
value as specified in the GIR file.

Make check is as successful as before (already existing error
related to GVariant is still there).
Comment 2 johnp 2010-09-23 15:41:15 UTC
Hmm, it would be nice if this patch is correct and we consolidate a bunch of code.  I'm a bit worried about generalizing the GValue freeing code as g_value_unset isn't called.

The tests should be moved out to another patch (and you are free to commit those)

I need to think about the patch a little more so it isn't going into the next release unless someone else can verify it.  Also, I haven't yet applied it but the diff looks like there are some tabs in the code.  I could be wrong but please make sure spaces are used everywhere and there is no whitespace at the end of lines.
Comment 3 Damien Caliste 2010-09-23 16:10:38 UTC
Created attachment 170931 [details] [review]
Remove the tabs from the previous patch

Sorry for the tabs, my fault. The newly attached patch should be free of them.

On your first point, the g_value_unset is indeed called each time thanks to the line 1700 from pygi-argument in function _pygi_argument_release, which is called on each argument with the patch in every cases.

I keep the test correction in this patch because the error does not appear without the patch. I explain:
- the error is generated in the Python part when the callback is called ;
- but the C code does not know about this and the g_function_info_invoke call in line 551 of pygi-invoke returns 1 since the Python called succeed (but set the error flag) ;
- at that stage, a call to Py_Err_Occured() would return TRUE but the C part does not know about this and next call to _process succeed also.
- then without the patch, _invoke succeed and return a not null value and when it returns into the Python interpreter the error is ignore (because of non null returned value). But with the patch, the returned value is cleared by the Py_CLEAR following the Py_Err_Occured test and the Python interpreter successfully raise an error.

By the way, if you think it is better to separate the patch for the test, I can fill up a new bug for it, no problem.
Comment 4 johnp 2010-09-23 16:53:35 UTC
Thanks.  No need to split out the tests.  I didn't realize they were related.  I'll take a closer look soon.  Perhaps I can get someone else to put eyes on this so we can get it out in the release I am preping.  It is a pretty big change but otherwise looks like a nice patch.
Comment 5 johnp 2010-09-23 17:37:36 UTC
I ended up committing this.  It passes make check and make check.valgrind doesn't get any worse.  Looked good overall and I saw the thread on the mailing list.  Thanks.