Skip to content

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

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

Open
wants to merge 36 commits into
base: main
Choose a base branch
from

Conversation

eendebakpt
Copy link
Contributor

@eendebakpt eendebakpt commented May 22, 2024

(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

  • 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
Copy link
Contributor

You need to include the file that defines that macro.

Copy link
Contributor

@nineteendo nineteendo left a comment

Choose a reason for hiding this comment

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

Revert newlines

Co-authored-by: Nice Zombies <nineteendo19d0@gmail.com>
@eendebakpt eendebakpt changed the title Draft: gh-116738: Make _json module thread-safe #117530 gh-116738: Make _json module thread-safe #117530 May 31, 2024
@eendebakpt eendebakpt changed the title gh-116738: Make _json module thread-safe #117530 gh-116738: Make _json module thread-safe May 31, 2024
@eendebakpt eendebakpt changed the title gh-116738: Make _json module thread-safe gh-116738: Make _json module safe in the free-threading build Aug 14, 2024
@nineteendo
Copy link
Contributor

nineteendo commented Aug 20, 2024

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

@eendebakpt eendebakpt marked this pull request as draft February 10, 2025 10:48
@eendebakpt eendebakpt marked this pull request as ready for review February 10, 2025 11:05
@eendebakpt
Copy link
Contributor Author

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

@kumaraditya303 kumaraditya303 requested review from kumaraditya303 and removed request for nineteendo August 8, 2025 05:44
Py_ssize_t indent_level, PyObject *indent_cache, PyObject *separator)
{
for (Py_ssize_t i = 0; i < PySequence_Fast_GET_SIZE(s_fast); i++) {
PyObject *obj = PySequence_Fast_GET_ITEM(s_fast, i);
Copy link
Contributor

Choose a reason for hiding this comment

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

Using borrowed reference is not safe here because if the critical section on sequence get's suspended then other thread can decref or free the object.

Copy link
Contributor

Choose a reason for hiding this comment

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

@eendebakpt Can you update the PR to use strong references?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kumaraditya303 Yes I will (probably with a macro to continue using borrowed ones in the normal build).

If I understand the mechanism correctly, then also the _encoder_iterate_dict_lock_held needs to be changed.
There PyDict_Next is used, which also returns borrowed references. (note: the docs for PyDict_Next mention that Py_BEGIN_CRITICAL_SECTION should be used in the free-theaded build, but make no mention of borrowed references, perhaps an additional note to the documentation is in order).

I tried creating a minimal example to add to the tests, but have not succeeded so far. Test script included below. Suggestions to get a minimal example are welcome.

Test script
import gc
import random
import time    
import sys
from threading import Barrier, Thread
from itertools import count
import json

def single_reference_int():
    # Return int that has a single reference (with very high probability)
    return random.randint(2**80, 2**82)


class EvilMapping(dict):
    
    cnt = count()
    
    def __init__(self, data):
        mapping = {next(self.cnt): single_reference_int()} # generate a mapping with no outside references
        super().__init__(mapping)
        self.data = data

    def keys(self):
        return list(self.mapping)
    
    def items(self):
        # this is called in encoder_listencode_dict which is called from encoder_listencode_obj
        # try to get this thread to suspect on the outer lock
        
        self.data.clear() # this will remove self from the list, leaving no references to self (except cyclic references)
        gc.collect() # the EvilMapping without refcounts (e.g. this one) should be cleared
        repr(self) # do something with self
        return super().items()
    
    def __repr__(self):
        return f'{self.__class__.__name__} {super().__repr__()}'
    
run= True
    
def worker(barrier, data, index):
            global run
            barrier.wait()
            while run:               
                # worker clears the list to generate borrowed references with refcount 0
                #print(f'worker {index} {len(lst)=}')
                data.append(single_reference_int())
                data.append(EvilMapping(data)) # inject more evil mappings
                if len(data) > 10:
                    data.clear()
            #print(f'worker {index} done')
            print(f'worked {index=} {run=}')


# we want a list where encoding one of the elements clears elements from the list that have refcount 1
data= []
data.append(EvilMapping(data))
data.append(EvilMapping(data))
print(f'{data=}')
j=json.dumps(data)
print(j)
print(f'{data=}')

#%%
print('----- go! -------')
data= []
data.append(EvilMapping(data))

number_of_threads = 2
number_of_json_encodings=3

worker_threads = []
barrier = Barrier(number_of_threads)
run = True
for index in range(number_of_threads):
    worker_threads.append(
        Thread(target=worker, args=[barrier, data, index])
    )
for t in worker_threads:
    t.start()
for ii in range(number_of_json_encodings):
    print(f'dump {ii}')
    data.extend( [EvilMapping(data)]*10)
    json.dumps(data)

run = False

for t in worker_threads:
    t.join()

Copy link
Contributor

Choose a reason for hiding this comment

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

There PyDict_Next is used, which also returns borrowed references. (note: the docs for PyDict_Next mention that Py_BEGIN_CRITICAL_SECTION should be used in the free-theaded build, but make no mention of borrowed references, perhaps an additional note to the documentation is in order).

Yes, it should say to use strong references. This is only a problem if somehow the critical section of dict gets suspended and in that case another thread can free the the borrowed object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created #138098 to update the docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants