Fix memory leaks and other types of leaks #1244
Conversation
| @@ -115,6 +115,7 @@ static int Service_Stop(udt_ServiceInfo* info) | |||
| { | |||
| PyThreadState *threadState; | |||
| PyObject *result; | |||
| int ret = 0; | |||
There was a problem hiding this comment.
Why these changes? If the stop() method could not be called or some other exception takes place, why are you marking the service as stopped anyway?
There was a problem hiding this comment.
This patch I should have put separately. There is another issue that I think may be related.
| @@ -164,27 +167,33 @@ static int Service_SessionChange(DWORD sessionId, DWORD eventType) | |||
| { | |||
| PyThreadState *threadState; | |||
| PyObject *result; | |||
| char *error_message = NULL; | |||
There was a problem hiding this comment.
Similar comments apply here. These changes also do not seem to be relevant for the concerns initially raised: namely, memory leaks.
There was a problem hiding this comment.
I call this "other types of leaks", because the thread is aquired but not released.
| return LogPythonException("unable to import __main__"); | ||
| if (!module) { | ||
| error_message = "unable to import __main__"; | ||
| goto end; |
There was a problem hiding this comment.
I am not at all a fan of goto. If necessary, you can create a separate method (Service_SetupPythonHelper or some such) which will perform the work. There is no real need, though, since if this fails, the entire executable will fail and the process will end.
There was a problem hiding this comment.
I don't like 'goto' either. I recently added a new function in util and used 'if succeded' but it's not all good either. As I have been reading some python codes and I see that in some situations they surrender to goto, I surrendered too....
|
|
||
| // indicate that the service has stopped | ||
| if (Service_SetStatus(info, SERVICE_STOPPED) < 0) | ||
| return LogWin32Error(GetLastError(), "cannot set service as stopped"); | ||
| ret = LogWin32Error(GetLastError(), "cannot set service as stopped"); | ||
|
|
||
| // now set the control event | ||
| if (!SetEvent(gControlEvent)) |
There was a problem hiding this comment.
The logic here is now different, since it used to exit early. I'm not familiar with Service_SetStatus or SetEvent so perhaps this is benign?
source/bases/Win32Service.c
Outdated
| @@ -468,45 +500,70 @@ static int Service_Install(wchar_t *name, wchar_t *configFileName) | |||
| //----------------------------------------------------------------------------- | |||
| static int Service_Uninstall(wchar_t *name) | |||
There was a problem hiding this comment.
I noticed that reference counts in Service_Install are not handled, but they are handled in Service_Uninstall. Was Service_Install left out intentionally? In any case, my main concern is the reference counts in Service_SetupPython and Service_Run are addressed (which they are), since any memory leak as part of install or uninstall will be very short lived.
marcelotduarte
force-pushed
the
leak1
branch
from
55583cb
to
b6244de
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1244 +/- ##
=======================================
Coverage 51.86% 51.86%
=======================================
Files 79 79
Lines 6177 6177
=======================================
Hits 3204 3204
Misses 2973 2973 ☔ View full report in Codecov by Sentry. |
Fixes #1040
@anthony-tuininga Can you review this for me?