gh-146152: Fix memory leak in _json encoder error paths by okiemute04 · Pull Request #146164 · python/cpython

okiemute04 · 2026-03-19T11:16:38Z

Add PyDict_DelItem calls to remove objects from the markers dict
on all error paths in encoder_listencode_obj. Previously, objects
were only removed on the success path, causing memory leaks when:

default() raises an exception
RecursionError occurs
Nested encoding fails

Issue: _json: stale markers entries on error paths in encoder_listencode_obj #146152

@raminfp

Only clean up markers on the RecursionError path (PATH B), where objects accumulate during stack unwinding. Other error paths are safe because the markers dict is local and will be freed. Based on review feedback from @raminfp and @serhiy-storchaka.

serhiy-storchaka

I am not sure that this works. Please add a test.

Define default() which creates a new object, adds a weak reference to it to the list, and returns that object.
Call JSON encoding, and after catching an exception, call test.support.gc_collect(), then test that all weak references in the list return None.

okiemute04 · 2026-03-20T09:49:03Z

@serhiy-storchaka Thanks for the review! I've added a test to test_recursion.py that creates objects tracked with weak references, triggers a RecursionError, and verifies all objects are freed after garbage collection.

serhiy-storchaka

Thank you for your update, @okiemute04.

The test does not fail with the current code. I am not sure it tests what it should test.

Lib/test/test_json/test_recursion.py

picnixz · 2026-03-22T17:17:38Z

Modules/_json.c

        Py_DECREF(newobj);
        if (rv) {
            _PyErr_FormatNote("when serializing %T object", obj);
+


Please revert this

Revert cosmetic changes. Nothing was properly reverted.

Modules/_json.c

Lib/test/test_json/test_recursion.py

Misc/NEWS.d/next/Library/2026-03-19-04-18-27.gh-issue-146152.Ifrd7O.rst

bedevere-app · 2026-03-22T17:22:24Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

- Add error checking for PyDict_DelItem return value - Remove extra blank lines in _json.c - Fix test import order and assertion message - Update NEWS entry to clearly describe the RecursionError path

…/cpython into fix-json-encoder-leak

okiemute04 · 2026-03-22T22:35:17Z

@picnixz has addressed your feedback and added error checking for PyDict_DelItem return value, removed extra blank lines in _json.c, fixed import order and assertion message format in test, updated NEWS entry to clearly describe the RecursionError path...thanks once again for the guide, really appreciate

picnixz

Please do not add or remove blank lines randomly.

picnixz · 2026-03-22T22:41:21Z

Lib/test/test_json/test_recursion.py

 from test import support
 from test.test_json import PyTest, CTest

-


Leave this blank line.

picnixz · 2026-03-22T22:43:26Z

Lib/test/test_json/test_recursion.py

+        try:
+            self.dumps(obj, default=default)
+        except Exception:
+            pass


Why not use the infinite recursion pattern + catching the RecursionError explicitly?

okay, that's a good suggestion, it makes more sense, I will do that

Lib/test/test_json/test_recursion.py

picnixz · 2026-03-22T22:44:15Z

Modules/_json.c

        Py_DECREF(newobj);
        if (rv) {
            _PyErr_FormatNote("when serializing %T object", obj);
+


Revert cosmetic changes. Nothing was properly reverted.

picnixz · 2026-03-22T22:45:44Z

Modules/_json.c

+            }
            return -1;
        }
+


Revert this blank line.

okiemute04 · 2026-03-22T22:48:20Z

I have made the requested changes, it's good for another review

picnixz · 2026-03-22T22:57:36Z

Do not ask for a review if tests are failing please.

- Use support.infinite_recursion() context manager - Catch RecursionError explicitly instead of all exceptions - Fix import order with two blank lines after from imports - Remove extra blank lines in _json.c - Use Py_DECREF instead of Py_XDECREF

…/cpython into fix-json-encoder-leak # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.

okiemute04 · 2026-03-22T23:26:30Z

@picnixz all addressed!!! Thanks for the feedback

picnixz

I will warn it once. I repeated this many times. But do not change unrelates lines and review what your LLM is giving you. I do not have time to lose with unfiltered LLM output.

Lib/test/test_json/test_recursion.py

Modules/_json.c

Lib/test/test_json/test_recursion.py

picnixz · 2026-03-23T00:08:14Z

Modules/_json.c

+            if (ident != NULL) {
+                int del_rv = PyDict_DelItem(s->markers, ident);
+                Py_DECREF(ident);
+                if (del_rv < 0) {
+                    Py_DECREF(newobj);
+                    return -1;
+                }
+
+            }


Suggested change

if (ident != NULL) {

int del_rv = PyDict_DelItem(s->markers, ident);

Py_DECREF(ident);

if (del_rv < 0) {

Py_DECREF(newobj);

return -1;

}

}

if (ident != NULL) {

(void)PyDict_DelItem(s->markers, ident);

}

Actually since we are anyway falling through the return -1 case we can simplify this as follows. Then keep the XDECREF for indent. However explicitly suppress the error code of DelItem.

picnixz · 2026-03-23T00:09:22Z

Modules/_json.c

-            if (PyDict_DelItem(s->markers, ident)) {
-                Py_XDECREF(ident);
+            int del_rv = PyDict_DelItem(s->markers, ident);
+            Py_DECREF(ident);
+            if (del_rv < 0) {
                return -1;
            }
-            Py_XDECREF(ident);


This part does not need modifications..It was already properly doing its job.

picnixz · 2026-03-23T00:10:44Z

Modules/_json.c

+            if (ident != NULL) {
+                int del_rv = PyDict_DelItem(s->markers, ident);
+                Py_DECREF(ident);
+                if (del_rv < 0) {
+                    return -1;
+                }
+            }


This one can benefit from the same optimization as my other comment as we anyway return -1.

okiemute04 · 2026-03-23T01:27:47Z

@picnixz made changes as requested, _json.c Error paths now use (void)PyDict_DelItem; success path unchanged

bedevere-app bot mentioned this pull request Mar 19, 2026

_json: stale markers entries on error paths in encoder_listencode_obj #146152

Open

bedevere-app bot added the awaiting review label Mar 19, 2026

okiemute04 force-pushed the fix-json-encoder-leak branch 3 times, most recently from 3eddfdf to 51c39f9 Compare March 19, 2026 13:53

okiemute04 force-pushed the fix-json-encoder-leak branch from 51c39f9 to 0597100 Compare March 19, 2026 14:00

serhiy-storchaka reviewed Mar 20, 2026

View reviewed changes

okiemute04 added 2 commits March 20, 2026 02:39

Add test for JSON encoder memory leak on recursion error

9ec9348

Add test for JSON encoder memory leak on recursion error

cd9cc6f

serhiy-storchaka reviewed Mar 22, 2026

View reviewed changes

Lib/test/test_json/test_recursion.py Outdated Show resolved Hide resolved

okiemute04 added 2 commits March 22, 2026 06:42

Use support.gc_collect() in JSON encoder memory leak test

5428b5e

Trigger CI rebuild

b68b52b

picnixz requested changes Mar 22, 2026

View reviewed changes

bedevere-app bot removed the awaiting review label Mar 22, 2026

bedevere-app bot added the awaiting changes label Mar 22, 2026

okiemute04 added 5 commits March 22, 2026 15:04

Address review feedback for JSON encoder memory leak fix

cafe9ba

- Add error checking for PyDict_DelItem return value - Remove extra blank lines in _json.c - Fix test import order and assertion message - Update NEWS entry to clearly describe the RecursionError path

Add fresh NEWS entry for JSON encoder memory leak fix

a4be61f

Trigger CI: all review feedback addressed

f7bdb6f

Trigger CI: all review feedback addressed

f09ce9a

Merge branch 'fix-json-encoder-leak' of https://github.com/okiemute04…

5359ecd

…/cpython into fix-json-encoder-leak

Trigger CI: all review comments resolved

753a88e

picnixz requested changes Mar 22, 2026

View reviewed changes

okiemute04 added 2 commits March 22, 2026 16:19

picnixz requested changes Mar 22, 2026

View reviewed changes

Lib/test/test_json/test_recursion.py Show resolved Hide resolved

Lib/test/test_json/test_recursion.py Show resolved Hide resolved

Modules/_json.c Show resolved Hide resolved

Modules/_json.c Show resolved Hide resolved

Fix blank lines

81332c7

picnixz reviewed Mar 23, 2026

View reviewed changes

simplify error path cleanup

a9c895e

		from test import support
		from test.test_json import PyTest, CTest

Uh oh!

Conversation

okiemute04 commented Mar 19, 2026 • edited by bedevere-app bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

okiemute04 commented Mar 20, 2026

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bedevere-app bot commented Mar 22, 2026

Uh oh!

okiemute04 commented Mar 22, 2026

Uh oh!

picnixz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

okiemute04 commented Mar 22, 2026

Uh oh!

picnixz commented Mar 22, 2026

Uh oh!

okiemute04 commented Mar 22, 2026

Uh oh!

picnixz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

okiemute04 commented Mar 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

okiemute04 commented Mar 19, 2026 •

edited by bedevere-app bot

Loading