gh-116738: Make _json module safe in the free-threading build

[eendebakpt]

(updated description)

Writing JSON files (or encoding to a string) is not thread-safe in the sense that when encoding data to json while another thread is mutating the data, the result is not well-defined (this is true for both the normal and free-threading build). But the free-threading build can crash the interpreter while writing JSON because of the usage of methods like PySequence_Fast_GET_ITEM. In this PR we make the free-threading build safe by adding locks in three places in the JSON encoder.

Reading from a JSON file is safe: objects constructed are only known to the executing thread. Encoding data to JSON needs a bit more care: mutable Python objects such as a list or a dict could be modified by another thread during encoding.

• When encoding a list use Py_BEGIN_CRITICAL_SECTION_SEQUENCE_FAST to project against mutation the list
• When encoding a dict, we use a critical section for iteration over exact dicts (PyDict_Next is used there). The non-exact dicts use PyMapping_Items to create a list of tuples. PyMapping_Items itself is assumed to be thread safe, but the resulting list is not a copy and can be mutated.

Update 2025-02-10: refactored to avoid using Py_EXIT_CRITICAL_SECTION_SEQUENCE_FAST

• Issue: Audit all built-in modules for thread safety #116738

• The script below was used to test the free-threading implementation. Similar code was added to the tests.

Test script

import json
from threading import Thread
import time

class JsonThreadingTest:
    
    def __init__(self, number_of_threads=4, number_of_json_dumps=10):
    
        self.data = [ [], [], {}, {}, {}]
        self.json = {str(ii): d for ii, d in enumerate(self.data)}
        self.results =[]
        self.number_of_threads=number_of_threads
        self.number_of_json_dumps =number_of_json_dumps
            
    def modify(self, index):
        while self.continue_thread:
            for d in self.data:
                if isinstance(d, list ):
                    if len(d)>20:
                        d.clear()
                    else:
                        d.append(index)
                else:
                    if len(d)>20:
                        try:
                            d.pop(list(d)[0])
                        except KeyError:
                            pass
                    else:
                        if index%2:                            
                            d[index] = index
                        else:
                            d[bytes(index)] = bytes(index)
                    
    def test(self):
        self.continue_thread = True
        self.modifying_threads = []
        for ii in range(self.number_of_threads):
            t = Thread(target=self.modify, args=[ii])
            self.modifying_threads.append(t)

        self.results.clear()
        for t in self.modifying_threads:
            print(f'start {t}')
            t.start()
            
        for ii in range(self.number_of_json_dumps):
            print(f'dump {ii}')
            time.sleep(0.01)
            
            indent = ii if ii%3==0 else None
            if ii%5==0:
                try:
                    j = json.dumps(self.data, indent=indent, skipkeys=True)
                except TypeError:
                        pass
            else:
                j = json.dumps(self.data, indent=indent)
            self.results.append(j)
        self.continue_thread= False
        
        print([hash(r) for r in self.results])
            


t=JsonThreadingTest(number_of_json_dumps=102, number_of_threads=8)
t0=time.time()
t.test()
dt=time.time()-t0
print(t.results[-1])        
print(f'Done: {dt:.2f}')

• The test script with t=JsonThreadingTest(number_of_json_dumps=102, number_of_threads=8) is a factor 25 faster using free-threading. Nice!

[nineteendo]

You need to include the file that defines that macro.

[nineteendo]

Revert newlines

[eendebakpt]

@nineteendo The implementation of free-threading (e.g. PEP703) is still work in progress, so things may change. But currently the critical sections for Py_BEGIN_CRITICAL_SECTION_SEQUENCE_FAST are in the internal API (see https://github.com/python/cpython/tree/main/Include#the-python-c-api), so they are not part of the public API. If you want to continue supporting the fork with free-threading, I think it is best to ask advice on discourse.

About the race condition: I think there are no guarantees for the result of the json encoder when the data to be encoded is mutated. So yes, race conditions can occur, and depending on how the data is mutated the json output may differ. But this is accepted behaviour. The goal of this PR is intended to prevent the interpreter crashing.

[nineteendo]

I think there are no guarantees for the result of the json encoder when the data to be encoded is mutated.

Shouldn't the empty list always be on a single line? So, without indentation. You can test this by overwriting dict.__len__() and list.__len__():

import io
import json

class BadDict(dict):
    def __len__(self) -> int:
        return 1

class BadList(list):
    def __len__(self) -> int:
        return 1

fp = io.StringIO()
json.dump([BadDict(), BadList()], fp, indent=4)
print(fp.getvalue())

[
    {
        
    },
    
    ]
]

Oh well, I managed to output invalid JSON. Some assumptions shouldn't be made.

[eendebakpt]

@nineteendo Interesting example. Note for recent python versions (in particular the current main branch) the json output depends on whether dump or dumps is used:

import io
import json

class BadDict(dict):
    def __len__(self) -> int:
        return 1

class BadList(list):
    def __len__(self) -> int:
        return 1

s=json.dumps([ BadList([])],  indent=4)
print(s)

print('--')
fp = io.StringIO()
json.dump([BadList()], fp, indent=4)
print(fp.getvalue())

has output

[
    []
]
--
[

    ]
]

This is something to do with the _one_shot parameter being passed around and the json.dump not using the C encoder. I investigate why this is.

The good thing is that while your example produces funny results, the interpreter does not crash (I checked the C code to make sure the bad list and bad dict are handled correctly).

[nineteendo]

This is something to do with the _one_shot parameter being passed around and the json.dump not using the C encoder. I investigate why this is.

It doesn't use the C encoder because it uses more memory than streaming to the file, but the Python implementation is 4x as slow. I thought about rewriting the C code to use streaming, but it will probably be slower as that would wrap _PyUnicodeWriter instead of using it directly. I opted to always use the C encoder.

[eendebakpt]

This is something to do with the _one_shot parameter being passed around and the json.dump not using the C encoder. I investigate why this is.

It doesn't use the C encoder because it uses more memory than streaming to the file, but the Python implementation is 4x as slow. I thought about rewriting the C code to use streaming, but it will probably be slower as that would wrap _PyUnicodeWriter instead of using it directly. I opted to always use the C encoder.

You are right. Using the C encoder works (i tried locally and it passes all the tests), but it would indeed use more memory. Would be nice to rewrite the C code so that it can work in streaming mode, but that is for another PR.

[nineteendo]

I think we should create a separate issue. Do we fix the race condition or just sub classes (like float and int)?
Fixing the race condition would create a shallow copy of the base class, while fixing just sub classes would use base class methods.

[eendebakpt]

I think we should create a separate issue. Do we fix the race condition or just sub classes (like float and int)? Fixing the race condition would create a shallow copy of the base class, while fixing just sub classes would use base class methods.

I am a bit lost here. Which race condition do you mean?

[nineteendo]

The python implementation first checks if the list is empty and then iterates over it. Instead of making a shallow copy of the list, checking the length of the copy and iterating over it. A different thread could probably make the list empty between these two statements. (Like the subclass is simulating)

if not lst:
    yield "[]"
    return

time.sleep(10) # allow thread to modify the list
for value in lst:
    ...

My question is: do we fix just the broken subclass or also this?

[eendebakpt]

The python implementation first checks if the list is empty and then iterates over it. Instead of making a shallow copy of the list, checking the length of the copy and iterating over it. A different thread could probably make the list empty between these two statements. (Like the subclass is simulating)

if not lst:
    yield "[]"
    return

time.sleep(10) # allow thread to modify the list
for value in lst:
    ...

My question is: do we fix just the broken subclass or also this?

In my opinion there is nothing to fix: when different threads are mutating the underlying data, we give no guarantees on the output. But we do guarantee we will not crash the python interpreter. The python implementation will not crash (since all individual python statements are safe). In this PR we modify the C implementation so that no crashes can occur. On the C side we want to make sure that if the underlying list is emptied we do not index into deallocated memory (this would crash the interpreter). (note: for the json encoder the C method that is unsafe for the list access is PyList_GET_ITEM)

There are some other PRs addressing safety under the free-threading builds and the feedback there was similar: address the crashes, but don't make guarantees on correct output (at the cost of performance). See
#120496 for example

[nineteendo]

There's a precedent for guarding against a broken int.__repr__() and float.__repr__(), so I've created an issue: #123183.

[eendebakpt]

@colesbury @mpage Would one you be able to review the PR? Thanks