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

Fix memory leak on encoding errors when the buffer was resized #549

Merged

Conversation

JustAnotherArchivist
Copy link
Collaborator

JSON_EncodeObject returns NULL when an error occurs, but without freeing the buffer. This leads to a memory leak when the buffer is internally allocated (because the caller's buffer was insufficient or none was provided at all) and any error occurs. Similarly, objToJSON did not clean up the buffer in all error conditions either.

This adds the missing buffer free in JSON_EncodeObject (iff the buffer was allocated internally) and refactors the error handling in objToJSON slightly to also free the buffer when a Python exception occurred without the encoder's errorMsg being set.


I haven't added a test for this so far, and I'm not sure how to do it. The stdlib only has resource.getrusage for this (as far as I could see), and on Linux, this only provides the max RSS (ixrss, idrss, and isrss are always zero per the man page). Since the test suite constantly allocates and frees memory, including some huge objects, that makes it kind of messy (subprocess?). Let me know what you think about that.

A quick manual test using a bytes object to trigger an exception:

import resource
import ujson

ujson.dumps(['a' * 65536, ''])
print(resource.getrusage(resource.RUSAGE_SELF).ru_maxrss)
for i in range(1000):
	try:
		ujson.dumps(['a' * 65536, b''])
	except:
		pass
print(resource.getrusage(resource.RUSAGE_SELF).ru_maxrss)

With main: 10224 → 78068 (kB)
With this branch: 10152 → 10152
(The exact numbers vary slightly, so that difference of 72 kB is meaningless.)

Note that the initial ujson.dumps causes an increase in memory usage (by ~350 kB on my machine). I haven't looked into that at all, but I assume it's static variables etc.?

`JSON_EncodeObject` returns `NULL` when an error occurs, but without freeing the buffer. This leads to a memory leak when the buffer is internally allocated (because the caller's buffer was insufficient or none was provided at all) and any error occurs. Similarly, `objToJSON` did not clean up the buffer in all error conditions either.

This adds the missing buffer free in `JSON_EncodeObject` (iff the buffer was allocated internally) and refactors the error handling in `objToJSON` slightly to also free the buffer when a Python exception occurred without the encoder's `errorMsg` being set.
@codecov-commenter
Copy link

codecov-commenter commented Jun 4, 2022

Codecov Report

Merging #549 (a615d27) into main (4ac30c9) will increase coverage by 0.05%.
The diff coverage is 100.00%.

Current head a615d27 differs from pull request most recent head a646c42. Consider uploading reports for the commit a646c42 to get more accurate results

@@            Coverage Diff             @@
##             main     #549      +/-   ##
==========================================
+ Coverage   91.59%   91.64%   +0.05%     
==========================================
  Files           6        6              
  Lines        1808     1820      +12     
==========================================
+ Hits         1656     1668      +12     
  Misses        152      152              
Impacted Files Coverage Δ
lib/ultrajsonenc.c 85.60% <100.00%> (+0.07%) ⬆️
python/objToJSON.c 90.14% <100.00%> (-0.03%) ⬇️
tests/test_ujson.py 99.62% <100.00%> (+<0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4ac30c9...a646c42. Read the comment docs.

@JustAnotherArchivist
Copy link
Collaborator Author

Based on Codecov, I guess we don't have any tests that cause a realloc and an error? That would be something to add then I suppose, regardless of testing for the memory leak, since it could hide bugs in the error handling as well.

@bwoodsend
Copy link
Collaborator

That little manual test example you've got put in its own file and invoked as a subproccess doesn't sound too unpleasant.

Based on Codecov, I guess we don't have any tests that cause a realloc and an error?

I've tried doing out of memory testing before and I was amazed at how painful it was. If we limit ourselves to just running these tests on Linux then we can use:

ulimit -Sv 200000
python some-test.py

which will run the some-test.py script but malloc() will return NULL if the amount of virtual memory exceeds 200MB. Because ulimit is a shell feature, we'd have to use:

subprocess.run("ulimit -Sv 200000; " + shlex.join([sys.executable, "memory-test.py"]), shell=True)
Example memory test file
import ctypes
import humanize

malloc = ctypes.CDLL("").malloc
malloc.argtypes = [ctypes.c_size_t]
malloc.restype = ctypes.c_void_p

for i in range(30):
    print(i, humanize.naturalsize(1 << i), malloc(1 << i))

@JustAnotherArchivist
Copy link
Collaborator Author

Yeah, memory-anything is really annoying with Python, which isn't really surprising since it's a high-level language.

ulimit would certainly be an option, although it seems like a bit of a crude way of doing it and could still hide small leaks. If we limit ourselves to Unix, we could use getrusage in a subprocess as well. Load ujson, run a test case once (for the initial memory growth from static variables or whatever it is exactly), get maxrss, run the test case multiple times, get maxrss again, sys.exit(1) if the measurements aren't (nearly?) identical. This effectively turns the maximum RSS measurement into an instantaneous one since we wouldn't expect memory usage to shrink during calls to ujson.dumps and ujson.loads.

@bwoodsend
Copy link
Collaborator

I meant to use ulimit specifically to test the otherwise uncovered codepaths where malloc() or realloc() return NULL.

For detecting memory leaks, I've used this in the past. I'm pretty uncertain about the choice between rss and vms - from what I've read, neither are really applicable but vms does at least go up and down on malloc() and free() so I went with that one. getrusage() would do too.

@JustAnotherArchivist
Copy link
Collaborator Author

I meant to use ulimit specifically to test the otherwise uncovered codepaths where malloc() or realloc() return NULL.

Hmm, right. We'd still need to write test cases that cover those paths to trigger it though, at which point we might as well use a more exact test. Or maybe I'm still misunderstanding you.

For detecting memory leaks, I've used this in the past.

Yeah, psutil is probably the 'correct' way of doing it, although I was hoping to not add a new dependency for the tests if it isn't strictly needed.

I'll play around with getrusage this week.

@JustAnotherArchivist JustAnotherArchivist force-pushed the fix-buffer-leak-encoding-errors branch 2 times, most recently from ed270d9 to 0d1b532 Compare June 9, 2022 21:18
@JustAnotherArchivist
Copy link
Collaborator Author

After some experimenting, it turns out that subprocess.run uses forking (at least on Linux under the circumstances I'm using it), which messes up the maxrss field. It would probably be possible to hack around that with shell = True, but I'd rather not. So psutil it is. And I switched to VMS as suggested.

@JustAnotherArchivist
Copy link
Collaborator Author

JustAnotherArchivist commented Jun 9, 2022

Hmm, memory usage grows slightly (under 2 %) in non-debug mode except on macOS... From local testing, this growth happens only on buffer resizes, does not depend on the number of repetitions, but does change with the input size:

# No resizing as it fits into the 64 KiB buffer
$ python3 tests/memory.py 'import functools, ujson; func = functools.partial(ujson.dumps, ["a" * 10000, b""])'; echo $?
VMS before: 20779008, after: 20779008
0

# Resizing, different number of iterations
$ python3 tests/memory.py 'import functools, ujson; func = functools.partial(ujson.dumps, ["a" * 11000, b""])'; echo $?
VMS before: 20779008, after: 21102592
1
$ python3 tests/memory.py 'import functools, ujson; func = functools.partial(ujson.dumps, ["a" * 11000, b""])' 2000; echo $?
VMS before: 20783104, after: 21102592
1
$ python3 tests/memory.py 'import functools, ujson; func = functools.partial(ujson.dumps, ["a" * 11000, b""])' 4000; echo $?
VMS before: 20783104, after: 21102592
1

# Resizing, different size
$ python3 tests/memory.py 'import functools, ujson; func = functools.partial(ujson.dumps, ["a" * 22000, b""])'; echo $?
VMS before: 20779008, after: 21114880
1
$ python3 tests/memory.py 'import functools, ujson; func = functools.partial(ujson.dumps, ["a" * 44000, b""])'; echo $?
VMS before: 20779008, after: 21397504
1

This makes no sense to me, especially since it doesn't happen in debug mode and the differences between debug and non-debug are almost exclusively additional if (buffer not big enough) { fprintf(...); abort(); } checks. The only different one is where the buffer size is exactly the requested amount vs doubling, but that shouldn't induce any leaks...

@JustAnotherArchivist
Copy link
Collaborator Author

JustAnotherArchivist commented Jun 9, 2022

The growth is only in VMS, not in RSS. The growth also disappears if I run func() twice at the start in memory.py's run function instead of once.

# Measuring and printing also RSS
$ python3 tests/memory.py 'import functools, ujson; func = functools.partial(ujson.dumps, ["a" * 11000, b""])'; echo $?
VMS before: 20803584, after: 21155840
RSS before: 14282752, after: 14282752
1

# Two `func()` calls at the start of `run`:
$ python3 tests/memory.py 'import functools, ujson; func = functools.partial(ujson.dumps, ["a" * 11000, b""])'; echo $?
VMS before: 21123072, after: 21123072
RSS before: 14385152, after: 14385152
0

I have no idea what this means...

@bwoodsend
Copy link
Collaborator

bwoodsend commented Jun 9, 2022

Yeah, I think this is normal I'm afraid. rss is just a subset of memory used because it only includes certain types of memory. vms is a very noisy superset of memory used because it includes memory that's shared with other processes. So they're both pretty unreliable. Apparently, uss is the recommended metric now.

@JustAnotherArchivist
Copy link
Collaborator Author

I see. Never heard of USS before, but that sounds very reasonable indeed. Still quite weird under which circumstances this VMS weirdness happens. It's perfectly reproducible, too. ¯\_(ツ)_/¯

USS grows as well but by a much smaller amount (well under 1 %) and regardless of the number of pre-runs and repetitions.

VMS/RSS/USS

All with one pre-run of func(). The exit status is still based on VMS.

# Debug
$ python3 tests/memory.py 'import functools, ujson; func = functools.partial(ujson.dumps, ["a" * 11000, b""])'; echo $?
VMS before: 20807680, after: 20967424
RSS before: 13983744, after: 13983744
USS before: 10473472, after: 10481664
0
$ python3 tests/memory.py 'import functools, ujson; func = functools.partial(ujson.dumps, ["a" * 11000, b""])' 2000; echo $?
VMS before: 20807680, after: 20967424
RSS before: 13901824, after: 13901824
USS before: 10469376, after: 10477568
0
$ python3 tests/memory.py 'import functools, ujson; func = functools.partial(ujson.dumps, ["a" * 11000, b""])' 4000; echo $?
VMS before: 20807680, after: 20967424
RSS before: 13946880, after: 13946880
USS before: 10436608, after: 10444800
0
$ python3 tests/memory.py 'import functools, ujson; func = functools.partial(ujson.dumps, ["a" * 22000, b""])'; echo $?
VMS before: 21008384, after: 21008384
RSS before: 13844480, after: 13844480
USS before: 10489856, after: 10514432
0
$ python3 tests/memory.py 'import functools, ujson; func = functools.partial(ujson.dumps, ["a" * 44000, b""])'; echo $?
VMS before: 20807680, after: 21196800
RSS before: 14200832, after: 14200832
USS before: 10461184, after: 10506240
1

# Prod
$ python3 tests/memory.py 'import functools, ujson; func = functools.partial(ujson.dumps, ["a" * 11000, b""])'; echo $?
VMS before: 20811776, after: 21164032
RSS before: 14417920, after: 14417920
USS before: 10469376, after: 10481664
1
$ python3 tests/memory.py 'import functools, ujson; func = functools.partial(ujson.dumps, ["a" * 11000, b""])' 2000; echo $?
VMS before: 20807680, after: 21164032
RSS before: 14315520, after: 14315520
USS before: 10391552, after: 10399744
1
$ python3 tests/memory.py 'import functools, ujson; func = functools.partial(ujson.dumps, ["a" * 11000, b""])' 4000; echo $?
VMS before: 20807680, after: 21164032
RSS before: 14331904, after: 14331904
USS before: 10493952, after: 10502144
1
$ python3 tests/memory.py 'import functools, ujson; func = functools.partial(ujson.dumps, ["a" * 22000, b""])'; echo $?
VMS before: 20807680, after: 21172224
RSS before: 14286848, after: 14286848
USS before: 10432512, after: 10457088
1
$ python3 tests/memory.py 'import functools, ujson; func = functools.partial(ujson.dumps, ["a" * 44000, b""])'; echo $?
VMS before: 20807680, after: 21458944
RSS before: 14225408, after: 14225408
USS before: 10485760, after: 10530816
1

But yeah, USS seems most reasonable, so I'll switch to that.

@JustAnotherArchivist
Copy link
Collaborator Author

So PyPy fails on testing, various deploy wheel tests fail except on PyPy 3.7 (which we aren't testing directly), the Windows native one due to no psutil even though it's getting installed just a few lines up. Maybe I shouldn't have looked under this particular rock...

tests/test_ujson.py Outdated Show resolved Hide resolved
Co-authored-by: Brénainn Woodsend <bwoodsend@gmail.com>
@bwoodsend
Copy link
Collaborator

I can reproduce the implied memory leak failure locally. Seeing if I can diagnose it now...

@bwoodsend
Copy link
Collaborator

Hmm, the memory increase seems to be constant irregardless of n. If there was a memory leak then the memory increase would be proportional to n so it is jumping at shadows. Not sure how to sort that...

@JustAnotherArchivist
Copy link
Collaborator Author

Does running func() multiple times before the first measurement help? Once should really be sufficient, but if multiple runs 'fix' it, there's no harm in that I think. We'd still catch any actual leaks.

@bwoodsend
Copy link
Collaborator

It doesn't change it at all. What does make it work is if I put the before = psutil.Process().memory_full_info().uss in a little loop so that it runs 10 times.

@bwoodsend
Copy link
Collaborator

In fact, just twice is enough.

diff --git i/tests/memory.py w/tests/memory.py
index 8035bd1..e2f4f71 100644
--- i/tests/memory.py
+++ w/tests/memory.py
@@ -11,7 +11,8 @@ def run(func, n):
         pass
 
     # Take a measurement
-    before = psutil.Process().memory_full_info().uss
+    for i in range(2):
+        before = psutil.Process().memory_full_info().uss
 
     # Run n times
     for i in range(n):

@JustAnotherArchivist
Copy link
Collaborator Author

Hmm, interesting. I also set this up locally now and am getting this:

# CPython 3.10.1
USS before: 10428416, after: 10440704
USS before: 10420224, after: 10432512
USS before: 10297344, after: 10309632

# PyPy 3.8/7.3.7 with no extra calls
USS before: 70766592, after: 71127040
USS before: 70934528, after: 71172096
USS before: 70287360, after: 70770688

# PyPy 3.8/7.3.7 with four extra calls to psutil.Process().memory_full_info() before the first measurement
USS before: 70520832, after: 70811648
USS before: 70615040, after: 70959104
USS before: 70348800, after: 70643712

So a constant +12 KiB with CPython. With PyPy, it varies strongly, but I still get growth with the extra calls, and it doesn't really seem to be smaller. The differences are small enough on my machine that it doesn't fail in either case though.

@bwoodsend
Copy link
Collaborator

Perhaps valgrind is a better way of doing this. It doesn't appear to be very cross platform so again this would probably have to be Linux only but squirting your reproducer code through it does detect the leak (see the 3rd block from below referencing Buffer_Realloc()). The signal to noise ratio isn't great since it also it's also analysing Python itself too but we can kind of get around that just by grepping for source filenames and/or functions that belong to ujson.

> git checkout main
> UJSON_BUILD_NO_STRIP=1 CFLAGS='-g -O0' python setup.py build_ext --inplace
> echo -e 'import ujson\ntry: ujson.dumps(["a" * 65536, b""]);\nexcept: pass' | valgrind --leak-check=yes python
==945== Memcheck, a memory error detector
==945== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==945== Using Valgrind-3.16.1 and LibVEX; rerun with -h for copyright info
==945== Command: python
==945== 
==945== 
==945== HEAP SUMMARY:
==945==     in use at exit: 831,129 bytes in 230 blocks
==945==   total heap usage: 1,560 allocs, 1,330 frees, 2,957,191 bytes allocated
==945== 
==945== 568 bytes in 1 blocks are possibly lost in loss record 12 of 149
==945==    at 0x483877F: malloc (vg_replace_malloc.c:307)
==945==    by 0x4971301: UnknownInlinedFun (obmalloc.c:99)
==945==    by 0x4971301: UnknownInlinedFun (obmalloc.c:572)
==945==    by 0x4971301: UnknownInlinedFun (obmalloc.c:1956)
==945==    by 0x4971301: UnknownInlinedFun (obmalloc.c:1949)
==945==    by 0x4971301: UnknownInlinedFun (obmalloc.c:685)
==945==    by 0x4971301: UnknownInlinedFun (gcmodule.c:2274)
==945==    by 0x4971301: UnknownInlinedFun (gcmodule.c:2301)
==945==    by 0x4971301: _PyObject_GC_NewVar (gcmodule.c:2332)
==945==    by 0x49929FB: UnknownInlinedFun (frameobject.c:794)
==945==    by 0x49929FB: UnknownInlinedFun (frameobject.c:837)
==945==    by 0x49929FB: _PyEval_MakeFrameVector (ceval.c:4790)
==945==    by 0x49A43A9: UnknownInlinedFun (ceval.c:5057)
==945==    by 0x49A43A9: _PyFunction_Vectorcall (call.c:342)
==945==    by 0x499350A: UnknownInlinedFun (abstract.h:114)
==945==    by 0x499350A: UnknownInlinedFun (abstract.h:123)
==945==    by 0x499350A: UnknownInlinedFun (ceval.c:5867)
==945==    by 0x499350A: _PyEval_EvalFrameDefault (ceval.c:4198)
==945==    by 0x49A43D7: UnknownInlinedFun (pycore_ceval.h:46)
==945==    by 0x49A43D7: UnknownInlinedFun (ceval.c:5065)
==945==    by 0x49A43D7: _PyFunction_Vectorcall (call.c:342)
==945==    by 0x499350A: UnknownInlinedFun (abstract.h:114)
==945==    by 0x499350A: UnknownInlinedFun (abstract.h:123)
==945==    by 0x499350A: UnknownInlinedFun (ceval.c:5867)
==945==    by 0x499350A: _PyEval_EvalFrameDefault (ceval.c:4198)
==945==    by 0x49A43D7: UnknownInlinedFun (pycore_ceval.h:46)
==945==    by 0x49A43D7: UnknownInlinedFun (ceval.c:5065)
==945==    by 0x49A43D7: _PyFunction_Vectorcall (call.c:342)
==945==    by 0x4993127: UnknownInlinedFun (abstract.h:114)
==945==    by 0x4993127: UnknownInlinedFun (abstract.h:123)
==945==    by 0x4993127: UnknownInlinedFun (ceval.c:5867)
==945==    by 0x4993127: _PyEval_EvalFrameDefault (ceval.c:4213)
==945==    by 0x49A43D7: UnknownInlinedFun (pycore_ceval.h:46)
==945==    by 0x49A43D7: UnknownInlinedFun (ceval.c:5065)
==945==    by 0x49A43D7: _PyFunction_Vectorcall (call.c:342)
==945==    by 0x4993127: UnknownInlinedFun (abstract.h:114)
==945==    by 0x4993127: UnknownInlinedFun (abstract.h:123)
==945==    by 0x4993127: UnknownInlinedFun (ceval.c:5867)
==945==    by 0x4993127: _PyEval_EvalFrameDefault (ceval.c:4213)
==945==    by 0x49A43D7: UnknownInlinedFun (pycore_ceval.h:46)
==945==    by 0x49A43D7: UnknownInlinedFun (ceval.c:5065)
==945==    by 0x49A43D7: _PyFunction_Vectorcall (call.c:342)
==945== 
==945== 720 bytes in 1 blocks are possibly lost in loss record 64 of 149
==945==    at 0x483877F: malloc (vg_replace_malloc.c:307)
==945==    by 0x49707D8: UnknownInlinedFun (obmalloc.c:99)
==945==    by 0x49707D8: UnknownInlinedFun (obmalloc.c:572)
==945==    by 0x49707D8: UnknownInlinedFun (obmalloc.c:1956)
==945==    by 0x49707D8: _PyObject_Malloc (obmalloc.c:1949)
==945==    by 0x49AA563: UnknownInlinedFun (obmalloc.c:2304)
==945==    by 0x49AA563: UnknownInlinedFun (obmalloc.c:2323)
==945==    by 0x49AA563: PyObject_Realloc (obmalloc.c:703)
==945==    by 0x4A5BEF8: _PyObject_GC_Resize (gcmodule.c:2350)
==945==    by 0x4992D24: UnknownInlinedFun (frameobject.c:809)
==945==    by 0x4992D24: UnknownInlinedFun (frameobject.c:837)
==945==    by 0x4992D24: _PyEval_MakeFrameVector (ceval.c:4790)
==945==    by 0x49A43A9: UnknownInlinedFun (ceval.c:5057)
==945==    by 0x49A43A9: _PyFunction_Vectorcall (call.c:342)
==945==    by 0x4A51303: _PyObject_VectorcallTstate.lto_priv.5 (abstract.h:114)
==945==    by 0x4A5122D: _PyObject_CallFunctionVa (call.c:479)
==945==    by 0x4A54A23: PyObject_CallMethod (call.c:577)
==945==    by 0x57CD46A: PyInit__decimal (_decimal.c:5734)
==945==    by 0x4A78036: UnknownInlinedFun (importdl.c:167)
==945==    by 0x4A78036: UnknownInlinedFun (import.c:2049)
==945==    by 0x4A78036: _imp_create_dynamic (import.c.h:330)
==945==    by 0x49A45D8: cfunction_vectorcall_FASTCALL (methodobject.c:430)
==945== 
==945== 524,288 bytes in 1 blocks are definitely lost in loss record 149 of 149
==945==    at 0x483877F: malloc (vg_replace_malloc.c:307)
==945==    by 0x49706D8: UnknownInlinedFun (obmalloc.c:99)
==945==    by 0x49706D8: UnknownInlinedFun (obmalloc.c:572)
==945==    by 0x49706D8: UnknownInlinedFun (obmalloc.c:1956)
==945==    by 0x49706D8: UnknownInlinedFun (obmalloc.c:1949)
==945==    by 0x49706D8: PyObject_Malloc (obmalloc.c:685)
==945==    by 0x55AA7B5: Buffer_Realloc (ultrajsonenc.c:150)
==945==    by 0x55ABFB5: encode (ultrajsonenc.c:894)
==945==    by 0x55AB8E3: encode (ultrajsonenc.c:757)
==945==    by 0x55AC36C: JSON_EncodeObject (ultrajsonenc.c:985)
==945==    by 0x55AE79D: objToJSON (objToJSON.c:874)
==945==    by 0x49A3F72: cfunction_call (methodobject.c:543)
==945==    by 0x499D509: _PyObject_MakeTpCall (call.c:215)
==945==    by 0x499837E: UnknownInlinedFun (abstract.h:112)
==945==    by 0x499837E: UnknownInlinedFun (abstract.h:99)
==945==    by 0x499837E: UnknownInlinedFun (abstract.h:123)
==945==    by 0x499837E: UnknownInlinedFun (ceval.c:5867)
==945==    by 0x499837E: _PyEval_EvalFrameDefault (ceval.c:4181)
==945==    by 0x4A532C1: UnknownInlinedFun (pycore_ceval.h:46)
==945==    by 0x4A532C1: _PyEval_Vector (ceval.c:5065)
==945==    by 0x4A53221: PyEval_EvalCode (ceval.c:1134)
==945== 
==945== LEAK SUMMARY:
==945==    definitely lost: 524,288 bytes in 1 blocks
==945==    indirectly lost: 0 bytes in 0 blocks
==945==      possibly lost: 1,288 bytes in 2 blocks
==945==    still reachable: 305,553 bytes in 227 blocks
==945==         suppressed: 0 bytes in 0 blocks
==945== Reachable blocks (those to which a pointer was found) are not shown.
==945== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==945== 
==945== For lists of detected and suppressed errors, rerun with: -s
==945== ERROR SUMMARY: 3 errors from 3 contexts (suppressed: 0 from 0)

@JustAnotherArchivist
Copy link
Collaborator Author

I remembered that tracemalloc is a thing. Since all allocations in ujson happen through Python's memory management methods (rather than plain malloc et al.), that works very well here. There isn't even any need to repeat runs or similar; just a single pre-run (for any shared state, caching, whatever) and then a single test run is sufficient to reliably detect allocation leaks. And it's built in, so no headaches with installation and configuration of another tool.

Sadly, this isn't supported by PyPy. But we already skip some memory-related tests on PyPy for the same reason, and since I don't think there is any CPython-specific code in ujson, I'd expect memory leaks to always show up in a CPython-only test. So that might be fine.

@bwoodsend
Copy link
Collaborator

tracemalloc it is then. And yes, let's just skip PyPy.

@JustAnotherArchivist
Copy link
Collaborator Author

For reference, this is how it fails on main (4ac30c9):

$ python3 tests/memory.py 'import functools, ujson; func = functools.partial(ujson.dumps, ["a" * 11000, b""])'; echo $?
<path>/ultrajson/tests/memory.py:32: size=64.5 KiB (+64.5 KiB), count=1 (+1), average=64.5 KiB
1

Or in the test suite:

<snip>
=================================== FAILURES ===================================
___________ test_no_memory_leak_encoding_errors[["a" * 11000, b""]] ____________

input = '["a" * 11000, b""]'

    @pytest.mark.skipif(
        hasattr(sys, "pypy_version_info"), reason="PyPy uses incompatible GC"
    )
    @pytest.mark.parametrize("input", ['["a" * 11000, b""]'])
    def test_no_memory_leak_encoding_errors(input):
>       no_memory_leak(f"functools.partial(ujson.dumps, {input})")

tests/test_ujson.py:1044: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

func_code = 'functools.partial(ujson.dumps, ["a" * 11000, b""])', n = []

    def no_memory_leak(func_code, n=None):
        code = f"import functools, ujson; func = {func_code}"
        path = os.path.join(os.path.dirname(__file__), "memory.py")
        n = [str(n)] if n is not None else []
        p = subprocess.run([sys.executable, path, code] + n)
>       assert p.returncode == 0
E       assert 1 == 0
E        +  where 1 = CompletedProcess(args=['<path>/bin/python3.10', '<path>/ultrajson/tests/memory.py', 'import functools, ujson; func = functools.partial(ujson.dumps, ["a" * 11000, b""])'], returncode=1).returncode

tests/test_ujson.py:1036: AssertionError
----------------------------- Captured stdout call -----------------------------
<path>/ultrajson/tests/memory.py:32: size=64.5 KiB (+64.5 KiB), count=1 (+1), average=64.5 KiB

Copy link
Collaborator

@bwoodsend bwoodsend left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one!

@bwoodsend bwoodsend added the changelog: Fixed For any bug fixes label Jun 13, 2022
@bwoodsend bwoodsend requested a review from hugovk June 13, 2022 21:22
@hugovk hugovk enabled auto-merge June 13, 2022 21:35
hugovk
hugovk approved these changes Jun 13, 2022
Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

(I've set this to automerge when/if Travis completes, we might need to give it a nudge later.)

@JustAnotherArchivist
Copy link
Collaborator Author

Travis CI's PowerPCs said no.

@hugovk
Copy link
Member

hugovk commented Jun 14, 2022

Restarted for 3.10:

image

@hugovk hugovk merged commit d40cf50 into ultrajson:main Jun 14, 2022
14 checks passed
@bwoodsend
Copy link
Collaborator

@hugovk Would you prefer that those travis jobs were on GitHub Actions? Docker buildx would handle the architecture emulation (like in the wheel builder pipeline). Then we can just use the generic python base docker images which come in pretty much every architecture you can think of?

@hugovk
Copy link
Member

hugovk commented Jun 14, 2022

Generally I'd like to ditch Travis completely!

But for normal CI testing, the main question is speed, we want things to run in just a few minutes, and emulation on GHA can be very slow. (Not too big a deal for a release every few weeks and post merge builds.)

Travis takes about 2 minutes each for these, but having jobs (ppc64le) in this case fail to queue up isn't great. Although Travis has been mostly stable recently.

Do you have an idea how long it would run with emulation?

@bwoodsend
Copy link
Collaborator

Hmm, it takes 89 seconds to run the Ubuntu variants or 70 seconds to run the Alpine variants on my machine. Then you needs to add about 20 seconds to setup docker buildx and however long it takes GA to download 200MBs (Ubuntu) or 20MBs (Alpine) worth of docker images. So I'd guess about the same. I think I'll just try it and see...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: Fixed For any bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants