GNOME Bugzilla – Bug 630271
pygi: always free the invocation_state struct
Last modified: 2010-11-04 21:58:03 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 patch.
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).
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.
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.
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.
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.