Same memory leaks. Now tested under python 2. by Adriandorr · Pull Request #270 · ultrajson/ultrajson

Adriandorr · 2017-06-19T08:38:57Z

This contains a fixes for a number of memory leaks, mainly related to dictionary keys and values.

Using ujson.dumps() with objects having __json__() method leaked memory for object's JSON representation.

PyObject_CallObject() returns a PyObject*; discarding it leaked memory for the result of output.write().

Merged pull request to fix memory leak.

qsniyg · 2017-11-02T06:49:32Z

I can confirm this works, hope it'll get pulled upstream.

orivej · 2017-12-26T05:27:59Z

python/objToJSON.c

+  if (GET_TC(tc)->itemName) {
+      Py_DECREF(GET_TC(tc)->itemName);
+      GET_TC(tc)->itemName = NULL;
+  }


Accidental extra indentation here.

ping @Adriandorr , this looks like the last remaining problem..?

orivej · 2017-12-26T05:36:24Z

tests/tests.py


+    def test_ujson_has_no_memory_leak(self):
+        import gc
+        import objgraph


The new dependency should be added to .travis.yml.

daicang · 2017-12-27T04:01:10Z

Nice work. This pr fixes memory leaks when dumps() dict with string/unicode/other container-type key & value. And seems it also fixes memory leak when dump to files.

Some examples I can confirm:
dumps() with:

{string: 1} string key
{unicode: 1} unicode key
{1: [1,]} list value
{1: {1:1}} dict value
{1: string} string value
{1: unicode} unicode value

danmaas · 2018-01-06T17:23:28Z

Thank you, this is a helpful fix. Hope it can be merged soon!

mxr · 2018-05-10T20:10:45Z

@Jahaja @orivej have you had a chance to take a look at this PR? Thanks!

nav-agarwal · 2019-04-24T11:39:53Z

@Adriandorr I am not able to reproduce these memory leaks with ujson(1.35) and python(2.7.14). I have used tests added by you in this fix. Reference count before and after was same.

Here is the script that I used:

import ujson
import sys

def test_does_not_leak_dictionary_values():
    import gc
    gc.collect()
    value = ["abc"]
    data = {"1": value}
    ref_count = sys.getrefcount(value)
    ujson.dumps(data)
    print(ref_count, sys.getrefcount(value))

def test_does_not_leak_dictionary_keys():
    import gc
    gc.collect()
    key1 = "1"
    key2 = "1"
    value1 = ["abc"]
    value2 = [1,2,3]
    data = {key1: value1, key2: value2}
    ref_count1 = sys.getrefcount(key1)
    ref_count2 = sys.getrefcount(key2)
    ujson.dumps(data)
    print(ref_count1, sys.getrefcount(key1))
    print(ref_count2, sys.getrefcount(key2))

def test_does_not_leak_dictionary_string_key():
    import gc
    gc.collect()
    key1 = "1"
    value1 = 1
    data = {key1: value1}
    ref_count1 = sys.getrefcount(key1)
    ujson.dumps(data)
    print(ref_count1, sys.getrefcount(key1))

def test_does_not_leak_dictionary_tuple_key():
    import gc
    gc.collect()
    key1 = ("a",)
    value1 = 1
    data = {key1: value1}
    ref_count1 = sys.getrefcount(key1)
    ujson.dumps(data)
    print(ref_count1, sys.getrefcount(key1))

def test_does_not_leak_dictionary_bytes_key():
    import gc
    gc.collect()
    key1 = b"1"
    value1 = 1
    data = {key1: value1}
    ref_count1 = sys.getrefcount(key1)
    ujson.dumps(data)
    print(ref_count1, sys.getrefcount(key1))

def test_does_not_leak_dictionary_None_key():
    import gc
    gc.collect()
    key1 = None
    value1 = 1
    data = {key1: value1}
    ref_count1 = sys.getrefcount(key1)
    ujson.dumps(data)
    print(ref_count1, sys.getrefcount(key1))

test_does_not_leak_dictionary_values()
test_does_not_leak_dictionary_keys()
test_does_not_leak_dictionary_string_key()
test_does_not_leak_dictionary_tuple_key()
test_does_not_leak_dictionary_bytes_key()
test_does_not_leak_dictionary_None_key()

Adriandorr · 2019-04-24T15:22:41Z

Have you tried adding the tests from the pull requests without the fixes?

nav-agarwal · 2019-04-24T18:41:30Z

Yes I have tested it without fixes. I am using released version 1.35 which doesn't have these fixes. Naveen

…

On Wed 24 Apr, 2019, 20:52 Adrian Dörr, ***@***.***> wrote: Have you tried adding the tests from the pull requests without the fixes? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#270 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABFOYQHBKVUXZNQJ43YKAZDPSB3MNANCNFSM4DPXVE5Q> .

Adriandorr · 2019-04-24T18:57:37Z

Strange. It’s been a while since I used this. I think it was with python 3, but might well be wrong.

jayvdb · 2019-05-11T03:54:35Z

python/objToJSON.c

+  if (GET_TC(tc)->itemName) {
+      Py_DECREF(GET_TC(tc)->itemName);
+      GET_TC(tc)->itemName = NULL;
+  }


ping @Adriandorr , this looks like the last remaining problem..?

@Adriandorr

Port of ultrajson/ultrajson#270 All credits to @Adriandorr

richafrank · 2020-02-23T20:37:22Z

python/objToJSON.c


-  if (!(GET_TC(tc)->itemValue = PyObject_GetItem(GET_TC(tc)->dictObj, GET_TC(tc)->itemName)))
-  {
+  if (GET_TC(tc)->itemValue) {


Oh, I hadn't seen this and fixed it differently in #353.

hugovk · 2020-03-01T17:16:42Z

I can reproduce the test failures locally on this branch, it's not always the same failure, and sometimes it passes:

$ python3.6 tests/tests.py
................................................................E...............................................................
======================================================================
ERROR: test_does_not_leak_dictionary_None_key (__main__.UltraJSONTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/tests.py", line 917, in test_does_not_leak_dictionary_None_key
    ujson.dumps(data)
OverflowError: Unterminated UTF-8 sequence when encoding string

----------------------------------------------------------------------
Ran 128 tests in 0.887s

FAILED (errors=1)
$ python3.6 tests/tests.py
................................................................E...............................................................
======================================================================
ERROR: test_does_not_leak_dictionary_None_key (__main__.UltraJSONTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/tests.py", line 917, in test_does_not_leak_dictionary_None_key
    ujson.dumps(data)
OverflowError: Unterminated UTF-8 sequence when encoding string

----------------------------------------------------------------------
Ran 128 tests in 1.156s

FAILED (errors=1)
$ python3.6 tests/tests.py
................................................................E...............................................................
======================================================================
ERROR: test_does_not_leak_dictionary_None_key (__main__.UltraJSONTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/tests.py", line 917, in test_does_not_leak_dictionary_None_key
    ujson.dumps(data)
UnicodeDecodeError: 'utf-8' codec can't decode byte 0xbb in position 9: invalid start byte

----------------------------------------------------------------------
Ran 128 tests in 0.625s

FAILED (errors=1)
$ python3.6 tests/tests.py
................................................................................................................................
----------------------------------------------------------------------
Ran 128 tests in 0.892s

OK
$

They pass when using pytest, it must enforce better test isolation:

$ mv tests/tests.py tests/test_ujson.py
$ python3.6 -m pytest
==================================================== test session starts ====================================================
platform darwin -- Python 3.6.9, pytest-5.3.5, py-1.8.1, pluggy-0.13.1
rootdir: /Users/hugo/github/ultrajson
plugins: cov-2.8.1
collected 128 items

tests/test_ujson.py ................................................................................................. [ 75%]
...............................                                                                                       [100%]

==================================================== 128 passed in 1.46s ====================================================
$ python3.6 -m pytest
==================================================== test session starts ====================================================
platform darwin -- Python 3.6.9, pytest-5.3.5, py-1.8.1, pluggy-0.13.1
rootdir: /Users/hugo/github/ultrajson
plugins: cov-2.8.1
collected 128 items

tests/test_ujson.py ................................................................................................. [ 75%]
...............................                                                                                       [100%]

==================================================== 128 passed in 2.08s ====================================================
$ python3.6 -m pytest
==================================================== test session starts ====================================================
platform darwin -- Python 3.6.9, pytest-5.3.5, py-1.8.1, pluggy-0.13.1
rootdir: /Users/hugo/github/ultrajson
plugins: cov-2.8.1
collected 128 items

tests/test_ujson.py ................................................................................................. [ 75%]
...............................                                                                                       [100%]

==================================================== 128 passed in 1.41s ====================================================
$

I've created PR #363 to switch over to pytest.

hugovk · 2020-03-01T17:38:34Z

Using pytest will fix it for 3.6 on Linux and macOS, but it still fails for 3.5 on Linux:

@Adriandorr Please could you check it?

hugovk

Subsets of this PR have been merged in #257 and #353.

Let's include the rest from here when the tests pass and conflicts are resolved, and anyone please feel free to create a new PR if necessary, it would be good to get this released.

AndreLouisCaron · 2020-06-02T00:59:46Z

@nav-agarwal Thanks for providing those tests! Thanks to them, I was able to confirm our upgrade to json>=2 introduced memory leaks in our application.

For others stumbling here, see code in this gist: https://gist.github.com/AndreLouisCaron/a18ae434e2eaf2866ffa4a24f693b2e8 to run tests against multiple ujson versions.

Note that I have the same results whether I use basepython = python3.8 or basepython = python2.7.

hugovk · 2022-05-30T09:30:09Z

Closing this as it's very much out of date but new PRs always welcome to fix things like this (and there have been quite a few recently). Thanks!

borman and others added 6 commits March 23, 2017 10:19

Release saved raw JSON string.

bc94d64

Using ujson.dumps() with objects having __json__() method leaked memory for object's JSON representation.

Do not discard result of PyObject_CallObject()

4481b8d

PyObject_CallObject() returns a PyObject*; discarding it leaked memory for the result of output.write().

Merge pull request #1 from borman/master

45c90a1

Merged pull request to fix memory leak.

{AD} Fixed memory leak of values in a dictionary.

74f538b

Tested and fixed dictionary key memory leaks.

813b775

{AD} Fixed python 2.

bccef01

daicang mentioned this pull request Dec 26, 2017

Fix a couple of memory leaks. #257

Merged

orivej reviewed Dec 26, 2017

View reviewed changes

danmaas added a commit to spinpunch/ultrajson that referenced this pull request Jan 6, 2018

apply memory leak fixes from ultrajson#270

b4fe669

jayvdb suggested changes May 11, 2019

View reviewed changes

sadovnychyi mentioned this pull request Jun 3, 2019

Memory leaks in ujson explosion/srsly#4

Closed

sadovnychyi added a commit to lawinsider/srsly that referenced this pull request Jun 3, 2019

Fix memory leak and add tests

0d63092

Port of ultrajson/ultrajson#270 All credits to @Adriandorr

sadovnychyi mentioned this pull request Jun 3, 2019

Fix memory leak and add tests explosion/srsly#5

Merged

richafrank reviewed Feb 23, 2020

View reviewed changes

hugovk added 2 commits February 23, 2020 23:11

Merge branch 'master' into master

e758314

Merge branch 'master' into master

eb35d15

hugovk mentioned this pull request Mar 1, 2020

Test with pytest #363

Merged

hugovk added the changelog: Fixed For any bug fixes label Mar 2, 2020

richafrank mentioned this pull request Mar 2, 2020

Fix reference counting bug for dict values #353

Merged

hugovk requested changes Mar 3, 2020

View reviewed changes

hugovk mentioned this pull request Mar 7, 2020

ujson uses 30% memory compared to json, even after load complete #333

Open

suola mentioned this pull request Mar 20, 2020

Decrease dict key reference. #394

Merged

hugovk closed this May 30, 2022

Conversation

Adriandorr commented Jun 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qsniyg commented Nov 2, 2017

Uh oh!

orivej Dec 26, 2017

Choose a reason for hiding this comment

Uh oh!

jayvdb May 11, 2019

Choose a reason for hiding this comment

Uh oh!

orivej Dec 26, 2017

Choose a reason for hiding this comment

Uh oh!

daicang commented Dec 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danmaas commented Jan 6, 2018

Uh oh!

mxr commented May 10, 2018

Uh oh!

nav-agarwal commented Apr 24, 2019 • edited by hugovk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Adriandorr commented Apr 24, 2019

Uh oh!

nav-agarwal commented Apr 24, 2019 via email

Uh oh!

Adriandorr commented Apr 24, 2019

Uh oh!

jayvdb May 11, 2019

Choose a reason for hiding this comment

Uh oh!

richafrank Feb 23, 2020

Choose a reason for hiding this comment

Uh oh!

hugovk commented Mar 1, 2020

Uh oh!

hugovk commented Mar 1, 2020

Uh oh!

hugovk left a comment

Choose a reason for hiding this comment

Uh oh!

AndreLouisCaron commented Jun 2, 2020

Uh oh!

hugovk commented May 30, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

Adriandorr commented Jun 19, 2017 •

edited

Loading

daicang commented Dec 27, 2017 •

edited

Loading

nav-agarwal commented Apr 24, 2019 •

edited by hugovk

Loading