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

Memory leak on exception handling with yajl2_c backend #97

Closed
mhugo opened this issue Jun 12, 2023 · 6 comments
Closed

Memory leak on exception handling with yajl2_c backend #97

mhugo opened this issue Jun 12, 2023 · 6 comments
Labels
bug Something isn't working

Comments

@mhugo
Copy link

mhugo commented Jun 12, 2023

Describe the bug

I think I found a memory leak with the yajl2_c backend when ijson.items raises an exception, some memory does not seem to be correctly deallocated.

How to reproduce*

Here is a small Python code to reproduce the issue:

import resource
import ijson
from ijson import JSONError


def memusage():
    """memory usage in MB. getrusage defaults to KB on Linux."""
    return str(resource.getrusage(resource.RUSAGE_SELF).ru_maxrss / 1e3)


print("starting memory usage:", memusage(), "MB")

for _ in range(10000):
    try:
        next(ijson.items(b"XXX", "whatever"))
    except JSONError:
        pass

print("memory usage after ijson calls:", memusage(), "MB")

Example output:

$ IJSON_BACKEND=yajl2_c python test_memleak.py 
starting memory usage: 10.376 MB
memory usage after ijson calls: 651.016 MB

$ IJSON_BACKEND=yajl2_cffi python test_memleak.py 
starting memory usage: 19.068 MB
memory usage after ijson calls: 19.068 MB

Expected behavior

The same run, but with less memory consumed :)

Execution information:

  • Python version: 3.7.10
  • ijson version: 3.2.0.post0
  • ijson backend: yajl2_c
  • ijson installation method: pip
  • OS: linux
@mhugo mhugo added the bug Something isn't working label Jun 12, 2023
@rtobar
Copy link

rtobar commented Jun 12, 2023

Thanks @mhugo for the concise and useful report. I just double-checked I can reproduce the issue on my side. I'll have a look now to see if I can quickly find out the culprit, otherwise it might take a bit longer.

rtobar added a commit that referenced this issue Jun 12, 2023
After reading data into a buffer the reading_generator_next function
calls invokes the underlying yajl parsing routine, which might return
successfully or not. There was a memory leak in the latter case, since
the "view" Py_buffer object was not being released before checking for
NULL returns and returning NULL ourselves.

This commit fixes the bug by ensuring we release the "view" object
before any returns, ensuring memory isn't leaked.

This was originally reported in #97.

Signed-off-by: Rodrigo Tobar <rtobar@icrar.org>
@rtobar
Copy link

rtobar commented Jun 12, 2023

@mhugo: found it and potentially fixed it, please give it a try under the issue-97 branch.

It indeed was very specific to the yajl2_c backend, but it actually affected all generator functions (basic_parse, parse, kvitems and items), but not the async or push interfaces.

@mhugo
Copy link
Author

mhugo commented Jun 12, 2023

Hi @rtobar
Thank you very much for your quick answer !

I confirm the problem is solved on the issue-97 branch. I've tested it on my small test and on my application code and I can't see the memory leak anymore.

When do you think would be the next release that will include this fix ?

rtobar added a commit that referenced this issue Jun 12, 2023
After reading data into a buffer the reading_generator_next function
calls invokes the underlying yajl parsing routine, which might return
successfully or not. There was a memory leak in the latter case, since
the "view" Py_buffer object was not being released before checking for
NULL returns and returning NULL ourselves.

This commit fixes the bug by ensuring we release the "view" object
before any returns, ensuring memory isn't leaked.

This was originally reported in #97.

Signed-off-by: Rodrigo Tobar <rtobar@icrar.org>
@rtobar
Copy link

rtobar commented Jun 12, 2023

@mhugo thanks for the confirmation 👍

I've put the fix now in master and will release 3.2.1 after that with the fix (and support for Python 3.12, which was fixed recently as well). I'm closing this issue now though as the fix is already available.

@rtobar rtobar closed this as completed Jun 12, 2023
@rtobar
Copy link

rtobar commented Jun 12, 2023

3.2.1 has now been published

@mhugo
Copy link
Author

mhugo commented Jun 13, 2023

Excellent ! Thank you very much again for your reactivity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants