Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Possible (tiny) Memory Leak in Win32Service.c #1040

Open
deymundson opened this issue Apr 29, 2021 · 7 comments · May be fixed by #1244
Open

Possible (tiny) Memory Leak in Win32Service.c #1040

deymundson opened this issue Apr 29, 2021 · 7 comments · May be fixed by #1244

Comments

@deymundson
Copy link

I noticed in Win32Service.c that there are several places where a PyObject reference is acquired but then the ref count is never decremented. For example, in Service_SetupPython() there is module = PyImport_ImportModule(...), but then Py_DECREF() is never called. Since PyImport_ImportModule() returns a "new" reference, I would expect to see Py_DECREF() called once the function is done working with module, based on this doc: https://docs.python.org/3/c-api/intro.html#reference-count-details

Is there a particular reason that these reference decrements do not exist? If not, it seems to me that Win32Service.c has a very tiny memory leak; in which case I'm happy to submit a PR with the reference decrements if you wish.

Desktop (please complete the following information):

  • Platform information: Windows
  • OS architecture: amd64
  • cx_Freeze version: 6.6
  • Python version: 3.8
@marcelotduarte
Copy link
Owner

Is there a particular reason that these reference decrements do not exist?

I see no reason. And I don't see anything documented. It must be forgetfulness.

I'm happy to submit a PR

okay.

There is an open issue about Win32Service that I would like you to take a look at and make comments if you wish.
https://github.com/marcelotduarte/cx_Freeze/issues?q=is%3Aissue+is%3Aopen+Win32Service+

@marcelotduarte marcelotduarte linked a pull request Sep 13, 2021 that will close this issue
@marcelotduarte
Copy link
Owner

@deymundson Can you take a look at PR #1224 for me?

@anthony-tuininga
Copy link
Collaborator

For example, in Service_SetupPython() there is module = PyImport_ImportModule(...), but then Py_DECREF() is never called. Since PyImport_ImportModule() returns a "new" reference, I would expect to see Py_DECREF() called once the function is done working with module, based on this doc: https://docs.python.org/3/c-api/intro.html#reference-count-details

That is strictly true, but functionally not important. :-) The module will be used for the duration of the process, so it won't matter what the refcount is! Adding the decref shouldn't harm things, though.

Regarding the other places, have you actually seen a memory leak? Or are you just examining the code?

@marcelotduarte
Copy link
Owner

That is strictly true, but functionally not important.

I'm not sure what I'm talking about, but apparently, this memory stays as used, so after several runs, the memory might run out. I have a case of a system that runs on a machine with limited memory (cloud) and after a day or two, it crashes due to memory exhaustion (The solution is to restart vm every day). I think issue #898 is about that too.

@anthony-tuininga
Copy link
Collaborator

So, with your patch does the memory use remain stable? Maybe I don't know what I am talking about. ;-)

@marcelotduarte
Copy link
Owner

That's not what I meant. Even because I'm not using Win32Service on this system, only the Console. And there must also be other leaks in other modules I use. It works great until memory runs out. That's why I said:
" I'm not sure what I'm talking about, but apparently, this memory stays as used "
And at the moment I have no way to test or tools to test.

@deymundson
Copy link
Author

Regarding the other places, have you actually seen a memory leak? Or are you just examining the code?

This is from examining the code - I'm noting discrepancies between Python's documentation and what I see in Win32Service.c.

For context, I'm currently maintaining some software that uses a fork of Win32Service.c. When the original developer forked the service base some 10 years ago, they added the decrefs that I noted are missing. However, the original developer never documented if those were added because of an observed memory leak.

My goal is to get the software back onto the "canonical" Win32Service instead of the fork; however, I'm reticent to do so without the decrefs in place since, if there was an observed memory leak, I don't want to reintroduce it. And in any case I think they "should" be there. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants