`methodcaller` is not thread-safe (or re-entrant) #127065

colesbury · 2024-11-20T17:03:30Z

Bug report

EDIT: edited to clarify that the issue is in the C implementation of operator.methodcaller.

Originally reported by @ngoldbaum in crate-py/rpds#101

Reproducer

from operator import methodcaller

from concurrent.futures import ThreadPoolExecutor

class HashTrieMap():
    def keys(self):
        return None
    def values(self):
        return None
    def items(self):
        return None

num_workers=1000

views = [methodcaller(p) for p in ["keys", "values", "items"]]

def work(view):
    m, d = HashTrieMap(), {}
    view(m)
    view(d)

iterations = 10

for _ in range(iterations):
    
    executor = ThreadPoolExecutor(max_workers=num_workers)

    for view in views:
        futures = [executor.submit(work, view) for _ in range(num_workers)]
        results = [future.result() for future in futures]

Once every 5-10 runs, the program prints:

TypeError: descriptor 'keys' for 'dict' objects doesn't apply to a 'HashTrieMap' object

The problem is that operator.methodcaller is not thread-safe because it modifies the vectorcall_args, which is shared across calls:

cpython/Modules/_operator.c

Lines 1646 to 1666 in 0af4ec3

    
           static PyObject * 
        
           methodcaller_vectorcall( 
        
                   methodcallerobject *mc, PyObject *const *args, size_t nargsf, PyObject* kwnames) 
        
           { 
        
               if (!_PyArg_CheckPositional("methodcaller", PyVectorcall_NARGS(nargsf), 1, 1) 
        
                   || !_PyArg_NoKwnames("methodcaller", kwnames)) { 
        
                   return NULL; 
        
               } 
        
               if (mc->vectorcall_args == NULL) { 
        
                   if (_methodcaller_initialize_vectorcall(mc) < 0) { 
        
                       return NULL; 
        
                   } 
        
               } 
        
               assert(mc->vectorcall_args != 0); 
        
               mc->vectorcall_args[0] = args[0]; 
        
               return PyObject_VectorcallMethod( 
        
                       mc->name, mc->vectorcall_args, 
        
                       (PyTuple_GET_SIZE(mc->xargs)) | PY_VECTORCALL_ARGUMENTS_OFFSET, 
        
                       mc->vectorcall_kwnames); 
        
           }

I think this is generally unsafe, not just for free threading. The vectorcall args array needs to be valid for the duration of the call, and it's possible for methodcaller to be called reentrantly or by another thread while the call is still ongoing.

Linked PRs

The text was updated successfully, but these errors were encountered:

colesbury · 2024-11-20T17:42:42Z

Hmm, actually it looks like a thread safety issue in methodcaller.

ngoldbaum · 2024-11-20T17:46:18Z

Yeah, I noticed early it wasn't triggering if I called the descriptors directly without going through methodcaller.

The `methodcaller` C vectorcall implementation uses an arguments array that is shared across calls. The first argument is modified on every invocation. This isn't thread-safe in the free threading build. I think it's also not safe in general, but for now just disable it in the free threading build.

colesbury · 2024-11-21T16:33:35Z

I think the optimized vectorcall implementation from #107201 is not reentrant or thread-safe (even with the GIL) because the shared vectorcall_args array is modified during each invocation:

cpython/Modules/_operator.c

Line 1661 in 3926842

mc->vectorcall_args[0] = args[0];

Most (or all) of our current vectorcall implementations extract out args[0] early, but that's not guaranteed and the vectorcall protocol is a public API.

For now, I'm planning to just disable the optimization in the free threading build (#127109), but I don't see an easy way of making it thread-safe, and I think we should consider reverting #107201 in 3.13 and main.

cc @eendebakpt @corona10 @vstinner

…127109) The `methodcaller` C vectorcall implementation uses an arguments array that is shared across calls. The first argument is modified on every invocation. This isn't thread-safe in the free threading build. I think it's also not safe in general, but for now just disable it in the free threading build.

…ild (pythonGH-127109) The `methodcaller` C vectorcall implementation uses an arguments array that is shared across calls. The first argument is modified on every invocation. This isn't thread-safe in the free threading build. I think it's also not safe in general, but for now just disable it in the free threading build. (cherry picked from commit f83ca69) Co-authored-by: Sam Gross <colesbury@gmail.com>

…uild (GH-127109) (GH-127150) The `methodcaller` C vectorcall implementation uses an arguments array that is shared across calls. The first argument is modified on every invocation. This isn't thread-safe in the free threading build. I think it's also not safe in general, but for now just disable it in the free threading build. (cherry picked from commit f83ca69) Co-authored-by: Sam Gross <colesbury@gmail.com>

eendebakpt · 2024-11-23T20:38:30Z

I agree this looks like a bug also in the normal build. The issue is hard to trigger in the normal build though, as indeed most vectorcall implementations (including the PyObject_VectorcallMethod used by the methodcaller) get args[0] before doing any work).

One option to make the vectorcall implementation thread-safe (and re-entrant) would be to copy the entire mc->vectorcall_args to a temporary buffer before setting the first element to args[0]. An implementation of this idea is main...eendebakpt:methodcaller_ft.

A quick benchmark (using the script from #107201) shows there is still a significant performance increase:

Current main:

call: Mean +- std dev: 48.2 ns +- 4.1 ns
creation: Mean +- std dev: 58.3 ns +- 3.4 ns
creation+call: Mean +- std dev: 114 ns +- 5 ns
call kwarg: Mean +- std dev: 63.8 ns +- 3.7 ns
creation kwarg: Mean +- std dev: 92.7 ns +- 3.1 ns
creation+call kwarg: Mean +- std dev: 187 ns +- 7 ns

Disable the vectorcall (this is the solution Sam added to the FT build):

call: Mean +- std dev: 78.2 ns +- 3.2 ns
creation: Mean +- std dev: 56.6 ns +- 2.7 ns
creation+call: Mean +- std dev: 136 ns +- 6 ns
call kwarg: Mean +- std dev: 124 ns +- 6 ns
creation kwarg: Mean +- std dev: 92.3 ns +- 4.1 ns
creation+call kwarg: Mean +- std dev: 228 ns +- 10 ns

Copy mc->vectorcall_args to a temporary buffer:

call: Mean +- std dev: 50.9 ns +- 1.6 ns
creation: Mean +- std dev: 56.6 ns +- 4.8 ns
creation+call: Mean +- std dev: 118 ns +- 6 ns
call kwarg: Mean +- std dev: 64.8 ns +- 2.5 ns
creation kwarg: Mean +- std dev: 91.9 ns +- 4.9 ns
creation+call kwarg: Mean +- std dev: 194 ns +- 9 ns

(all benchmarks without PGO and with the normal Python build)

@colesbury Is this idea an option for the free-threading build?
@Yhg1s What would you like to do for the 3.13 branch?

vstinner · 2024-11-25T08:48:55Z

An implementation of this idea is main...eendebakpt:methodcaller_ft.

Can you create a PR? I cannot open the link, it takes too long and GitHub displays an error.

eendebakpt · 2024-12-01T22:41:07Z

I removed the draft status from the PR (thanks to @colesbury for some good suggestions). Both PR #127109 or reverting #107201 are good options I think (a classic trade-off between performance and added complexity, except that in this case the status quo is not good)

colesbury added type-bug An unexpected behavior, bug, or error 3.13 bugs and security fixes topic-free-threading 3.14 new features, bugs and security fixes labels Nov 20, 2024

colesbury changed the title ~~Free threading race condition involving descriptor lookup~~ methodcaller is not thread-safe (or re-entrant) Nov 20, 2024

bedevere-app bot mentioned this issue Nov 21, 2024

gh-127065: Make methodcaller thread-safe in free threading build #127109

Merged

bedevere-app bot mentioned this issue Nov 22, 2024

[3.13] gh-127065: Make methodcaller thread-safe in free threading build (GH-127109) #127150

Merged

bedevere-app bot mentioned this issue Nov 25, 2024

gh-127065: Make methodcaller thread-safe and re-entrant #127245

Open

github-actions bot mentioned this issue Dec 1, 2024

Monthly issue metrics report hugovk/test#88

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`methodcaller` is not thread-safe (or re-entrant) #127065

`methodcaller` is not thread-safe (or re-entrant) #127065

colesbury commented Nov 20, 2024 •

edited by bedevere-app bot

Loading

colesbury commented Nov 20, 2024

ngoldbaum commented Nov 20, 2024

colesbury commented Nov 21, 2024 •

edited

Loading

eendebakpt commented Nov 23, 2024

vstinner commented Nov 25, 2024 •

edited

Loading

eendebakpt commented Dec 1, 2024

methodcaller is not thread-safe (or re-entrant) #127065

methodcaller is not thread-safe (or re-entrant) #127065

Comments

colesbury commented Nov 20, 2024 • edited by bedevere-app bot Loading

Bug report

Linked PRs

colesbury commented Nov 20, 2024

ngoldbaum commented Nov 20, 2024

colesbury commented Nov 21, 2024 • edited Loading

eendebakpt commented Nov 23, 2024

vstinner commented Nov 25, 2024 • edited Loading

eendebakpt commented Dec 1, 2024

`methodcaller` is not thread-safe (or re-entrant) #127065

`methodcaller` is not thread-safe (or re-entrant) #127065

colesbury commented Nov 20, 2024 •

edited by bedevere-app bot

Loading

colesbury commented Nov 21, 2024 •

edited

Loading

vstinner commented Nov 25, 2024 •

edited

Loading