gh-128942: make arraymodule.c free-thread safe #128943

tom-pytel · 2025-01-17T13:51:36Z

This PR is a work in progress but it currently fixes the crashes mentioned in the related issue so I am sending it up now to elicit feedback to make sure I am on the right path. So far the changes have been made functional but as minimal as possible to facilitate review. The critical sections are fairly broad so optimization may follow if functionality is confirmed. The changes are mostly additions of critical sections to publicly visible methods and slots gotten from the two structures listed below (as well as a few internal functions):

static PyMethodDef array_methods[] = {
    ARRAY_ARRAY_APPEND_METHODDEF
    ARRAY_ARRAY_BUFFER_INFO_METHODDEF
    ARRAY_ARRAY_BYTESWAP_METHODDEF
    ARRAY_ARRAY_CLEAR_METHODDEF
    ARRAY_ARRAY___COPY___METHODDEF
    ARRAY_ARRAY_COUNT_METHODDEF
    ARRAY_ARRAY___DEEPCOPY___METHODDEF
    ARRAY_ARRAY_EXTEND_METHODDEF
    ARRAY_ARRAY_FROMFILE_METHODDEF
    ARRAY_ARRAY_FROMLIST_METHODDEF
    ARRAY_ARRAY_FROMBYTES_METHODDEF
    ARRAY_ARRAY_FROMUNICODE_METHODDEF
    ARRAY_ARRAY_INDEX_METHODDEF
    ARRAY_ARRAY_INSERT_METHODDEF
    ARRAY_ARRAY_POP_METHODDEF
    ARRAY_ARRAY___REDUCE_EX___METHODDEF
    ARRAY_ARRAY_REMOVE_METHODDEF
    ARRAY_ARRAY_REVERSE_METHODDEF
    ARRAY_ARRAY_TOFILE_METHODDEF
    ARRAY_ARRAY_TOLIST_METHODDEF
    ARRAY_ARRAY_TOBYTES_METHODDEF
    ARRAY_ARRAY_TOUNICODE_METHODDEF
    ARRAY_ARRAY___SIZEOF___METHODDEF
    {"__class_getitem__", Py_GenericAlias, METH_O|METH_CLASS, PyDoc_STR("See PEP 585")},
    {NULL, NULL}  /* sentinel */
};

static PyType_Slot array_slots[] = {
    {Py_tp_dealloc, array_dealloc},
    {Py_tp_repr, array_repr},
    {Py_tp_getattro, PyObject_GenericGetAttr},
    {Py_tp_doc, (void *)arraytype_doc},
    {Py_tp_richcompare, array_richcompare},
    {Py_tp_iter, array_iter},
    {Py_tp_methods, array_methods},
    {Py_tp_members, array_members},
    {Py_tp_getset, array_getsets},
    {Py_tp_alloc, PyType_GenericAlloc},
    {Py_tp_new, array_new},
    {Py_tp_traverse, array_tp_traverse},

    /* as sequence */
    {Py_sq_length, array_length},
    {Py_sq_concat, array_concat},
    {Py_sq_repeat, array_repeat},
    {Py_sq_item, array_item},
    {Py_sq_ass_item, array_ass_item},
    {Py_sq_contains, array_contains},
    {Py_sq_inplace_concat, array_inplace_concat},
    {Py_sq_inplace_repeat, array_inplace_repeat},

    /* as mapping */
    {Py_mp_length, array_length},
    {Py_mp_subscript, array_subscr},
    {Py_mp_ass_subscript, array_ass_subscr},

    /* as buffer */
    {Py_bf_getbuffer, array_buffer_getbuf},
    {Py_bf_releasebuffer, array_buffer_relbuf},

    {0, NULL},
};

The following public methods/slots have been modified directly:

* array_array_buffer_info   = ARRAY_ARRAY_BUFFER_INFO_METHODDEF
* array_array_byteswap      = ARRAY_ARRAY_BYTESWAP_METHODDEF
* array_array_clear         = ARRAY_ARRAY_CLEAR_METHODDEF
* array_array_count         = ARRAY_ARRAY_COUNT_METHODDEF
* array_array_fromlist      = ARRAY_ARRAY_FROMLIST_METHODDEF  / CRITICAL2
* array_array_fromunicode   = ARRAY_ARRAY_FROMUNICODE_METHODDEF
* array_array_index         = ARRAY_ARRAY_INDEX_METHODDEF
* array_array_pop           = ARRAY_ARRAY_POP_METHODDEF
* array_array_remove        = ARRAY_ARRAY_REMOVE_METHODDEF
* array_array_reverse       = ARRAY_ARRAY_REVERSE_METHODDEF
* array_array_tofile        = ARRAY_ARRAY_TOFILE_METHODDEF
* array_array_tolist        = ARRAY_ARRAY_TOLIST_METHODDEF
* array_array_tobytes       = ARRAY_ARRAY_TOBYTES_METHODDEF
* array_array_tounicode     = ARRAY_ARRAY_TOUNICODE_METHODDEF
* array_array___sizeof__    = ARRAY_ARRAY___SIZEOF___METHODDEF
* array_repr                = Py_tp_repr
* array_richcompare         = Py_tp_richcompare  / CRITICAL2
* array_new                 = Py_tp_new
* array_length              = Py_sq_length
* array_concat              = Py_sq_concat  / CRITICAL2
* array_repeat              = Py_sq_repeat
* array_item                = Py_sq_item
* array_ass_item            = Py_sq_ass_item
* array_contains            = Py_sq_contains
* array_inplace_repeat      = Py_sq_inplace_repeat
* array_length              = Py_mp_length
* array_subscr              = Py_mp_subscript  -> array_item
* array_ass_subscr          = Py_mp_ass_subscript  / CRITICAL2
* array_buffer_getbuf       = Py_bf_getbuffer
* array_buffer_relbuf       = Py_bf_releasebuffer

The following have not been modified but depend for safety on the functions they call:

+ array_array_append        = ARRAY_ARRAY_APPEND_METHODDEF  -> ins
+ array_array___copy__      = ARRAY_ARRAY___COPY___METHODDEF  -> array_slice
+ array_array___deepcopy__  = ARRAY_ARRAY___DEEPCOPY___METHODDEF  -> array_array___copy__
+ array_array_extend        = ARRAY_ARRAY_EXTEND_METHODDEF  -> array_do_extend  / CRITICAL2
+ array_array_frombytes     = ARRAY_ARRAY_FROMBYTES_METHODDEF  -> frombytes  / CRITICAL2
+ array_array_insert        = ARRAY_ARRAY_INSERT_METHODDEF  -> ins
+ array_array___reduce_ex__ = ARRAY_ARRAY___REDUCE_EX___METHODDEF  -> array_array_tolist_impl, array_array_tobytes_impl
+ array_inplace_concat      = Py_sq_inplace_concat  -> array_do_extend  / CRITICAL2

The following look safe as is:

- array_array_fromfile      = ARRAY_ARRAY_FROMFILE_METHODDEF  -> array_array_frombytes
- array_dealloc             = Py_tp_dealloc
- array_iter                = Py_tp_iter
- array_tp_traverse         = Py_tp_traverse

And the following non-public utility functions have been made safe:

* array_slice
* array_do_extend  / CRITICAL2
* ins
* frombytes  / CRITICAL2

Here are a few questions needing guidance:

Changed clinic sig of array_array_frombytes() so I could lock the original source object during op (maybe its a writeable bytearray). Want to prevent modification of data during the operation but not sure this will even do this so not sure is necessary or even desired. Should I remove this and leave just the PyObject_GetBuffer() for protection? Also for this added a forward declaration of array_array_frombytes() for array_array_fromfile_impl() instead of moving stuff around for minimal impact.
In array_new there may or may not be an initializer object, if there is not then the args tuple is used as a stand-in for the critical section lock. Is there a better way to do this?

Misc:

array_richcompare was incrementing refcount of Py_True and Py_False, removed that as they are immortal.
The following were larger / slightly more complicated functions to check so should double check array_array_frombytes, array_richcompare, array_ass_subscr and array_new.

More work is still needed, like the array iterator and more testing and verification.

Issue: Python array module is not free-thread safe. #128942

bedevere-app · 2025-01-17T13:51:43Z

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

tom-pytel · 2025-01-17T13:55:41Z

Requesting a look from @ZeroIntensity.

ZeroIntensity

Thanks for doing this! At a glance, don't use goto with critical sections--it's messier and can be error-prone (IIRC there's problems with it compiling on MSVC too). Instead, create a wrapper function like this:

static PyObject *
array_something(arrayobject *self, PyObject *arg)
{
    PyObject *res;
    Py_BEGIN_CRITICAL_SECTION(self);
    res = array_something_lock_held(); // the actual implementation function
    Py_END_CRITICAL_SECTION();
    return res;
}

For functions that are defined with the Argument Clinic, you can use @critical_section to do this even faster. (See what I did for the ssl module.)

tom-pytel · 2025-01-17T18:33:09Z

Made requested changes, also made arrayiter safe.

Also found a good old-fashioned non-free-threaded bug which probably goes back a long way - if try to arrayiter.__setstate__() when it is exhausted then segfault, fixed here obviously.

Question remaining about need to lock bytes source object in frombytes() or if holding the buffer is sufficient?

erlend-aasland · 2025-01-17T20:46:28Z

Also found a good old-fashioned non-free-threaded bug which probably goes back a long way - if try to arrayiter.__setstate__() when it is exhausted then segfault, fixed here obviously.

Could you separate that out from this PR and submit a dedicated PR for this bug? We might want to backport it.

erlend-aasland · 2025-01-17T23:00:41Z

Regarding the style nits: please make sure all new code (and in most cases, also changed code) follow PEP-7.

erlend-aasland · 2025-01-17T23:04:57Z

Modules/arraymodule.c

 /*[clinic input]
 array.array.frombytes

-    buffer: Py_buffer
+    bytes: object


Please remind me again why we can't use @critical_section here? It's been a while since I've had a look at these kinds of changes. cc. @ZeroIntensity

I just put it on the internal function same as with ins(), but I can raise the critical sections to the public functions array_array_frombytes, array_array_insert and array_array_append if you prefer.

Right. If Peter is fine with this variant, I'm also fine with it, but please leave the docstring intact. You need this (coupled with a make clinic):

Suggested change

bytes: object

bytes as buffer: object

And maybe you can answer that question for me here, does bytes need to be locked in this case or is it sufficient just to hold the buffer as was the case in the original non-free safe code?

For this case, I don't think we need a lock_held function. Just @critical_section should do.

I'm not sure what the rules are for the buffer protocol in regards to free-threading. Hopefully it's fine to leave bytes unlocked.

Modules/arraymodule.c

erlend-aasland · 2025-01-18T00:56:48Z

Modules/arraymodule.c

+    if (value != NULL) {
+        _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(value);
+    }


Use pre-processor guard instead of runtime guards for these: Py_DEBUG.

The whole if block gets optimized out when Py_DEBUG is not defined, but having the #ifdef doesn't hurt either.

tom-pytel · 2025-01-18T12:43:15Z

Still would like to know if array_array_frombytes() really needs to lock the bytes object or if just holding the buffer is sufficient. The idea being trying to prevent the CONTENTS of the buffer changing during the operation. Just holding buffer is not enough if is writable but not sure locking object would help that anyway.

Misc/NEWS.d/next/Library/2025-01-17-13-53-32.gh-issue-128942.DxzaIg.rst

Modules/arraymodule.c

ZeroIntensity

We're getting there! Now, we're overlocking things a bit. This isn't always the rule, but in general you only need to lock something if you want to avoid inconsistent state (making it a "critical section"!).

That, and keep in mind that things that have a _Py or Py prefix are generally thread safe themselves and will lock if they need to. You don't need to lock for every call to Python.

ZeroIntensity · 2025-01-18T19:19:06Z

Modules/arraymodule.c

 /*[clinic input]
 array.array.frombytes

-    buffer: Py_buffer
+    bytes: object


For this case, I don't think we need a lock_held function. Just @critical_section should do.

I'm not sure what the rules are for the buffer protocol in regards to free-threading. Hopefully it's fine to leave bytes unlocked.

ZeroIntensity · 2025-01-18T19:24:01Z

Modules/arraymodule.c

@@ -2657,6 +2825,7 @@ array_buffer_getbuf(arrayobject *self, Py_buffer *view, int flags)
        return -1;
    }

+    Py_BEGIN_CRITICAL_SECTION(self);


I think this should probably just be a lock_held function.

ZeroIntensity · 2025-01-18T19:24:15Z

Modules/arraymodule.c

    return 0;
 }

 static void
 array_buffer_relbuf(arrayobject *self, Py_buffer *view)
 {
-    self->ob_exports--;
+    _Py_atomic_add_ssize(&self->ob_exports, -1);


This will affect the default build, we need an #ifdef Py_GIL_DISABLED.

ZeroIntensity · 2025-01-18T19:26:13Z

Modules/arraymodule.c

@@ -754,7 +760,7 @@ array_richcompare(PyObject *v, PyObject *w, int op)
            res = Py_False;
        else
            res = Py_True;
-        return Py_NewRef(res);
+        return res;


This looks unrelated. Let's try to avoid that for now.

pythongh-128942: make arraymodule.c free-thread safe

7c8edcd

bedevere-app bot mentioned this pull request Jan 17, 2025

Python array module is not free-thread safe. #128942

Open

bedevere-app bot added the awaiting review label Jan 17, 2025

📜🤖 Added by blurb_it.

b4c38d6

tom-pytel added 2 commits January 17, 2025 08:57

Update 2025-01-17-13-53-32.gh-issue-128942.DxzaIg.rst

523c049

Update 2025-01-17-13-53-32.gh-issue-128942.DxzaIg.rst

714da01

ZeroIntensity reviewed Jan 17, 2025

View reviewed changes

ZeroIntensity added the topic-free-threading label Jan 17, 2025

tom-pytel added 2 commits January 17, 2025 12:34

refactor array methods to established patterns

6893468

made arrayiter freethread safe

65b4a63

tom-pytel added 2 commits January 17, 2025 16:38

cleanup to match other segfault fix PR

761d27a

style nit from other PR

0bcd3c7

erlend-aasland reviewed Jan 17, 2025

View reviewed changes

Modules/arraymodule.c Outdated Show resolved Hide resolved

tom-pytel added 2 commits January 17, 2025 18:38

add _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED()

5676c92

misc style

c18290c

erlend-aasland reviewed Jan 18, 2025

View reviewed changes

tom-pytel mentioned this pull request Jan 18, 2025

gh-128961: Fix exhausted array iterator crash in __setstate__ #128962

Merged

erlend-aasland mentioned this pull request Jan 18, 2025

array iterator segfault on __setstate__() when exhausted #128961

Closed

misc cleanups

6ee2303

picnixz reviewed Jan 18, 2025

View reviewed changes

Misc/NEWS.d/next/Library/2025-01-17-13-53-32.gh-issue-128942.DxzaIg.rst Outdated Show resolved Hide resolved

Modules/arraymodule.c Outdated Show resolved Hide resolved

tom-pytel added 2 commits January 18, 2025 07:47

misc

7d5de1f

Merge branch 'main' into fix-issue-128942

02fc79c

ZeroIntensity reviewed Jan 18, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-128942: make arraymodule.c free-thread safe #128943

gh-128942: make arraymodule.c free-thread safe #128943

tom-pytel commented Jan 17, 2025 •

edited by bedevere-app bot

Loading

bedevere-app bot commented Jan 17, 2025

tom-pytel commented Jan 17, 2025

ZeroIntensity left a comment

tom-pytel commented Jan 17, 2025 •

edited

Loading

erlend-aasland commented Jan 17, 2025

erlend-aasland commented Jan 17, 2025

erlend-aasland Jan 17, 2025

tom-pytel Jan 17, 2025

erlend-aasland Jan 17, 2025 •

edited

Loading

tom-pytel Jan 17, 2025

ZeroIntensity Jan 18, 2025

erlend-aasland Jan 18, 2025

tom-pytel Jan 18, 2025

tom-pytel commented Jan 18, 2025

ZeroIntensity left a comment

ZeroIntensity Jan 18, 2025

ZeroIntensity Jan 18, 2025

ZeroIntensity Jan 18, 2025

ZeroIntensity Jan 18, 2025

gh-128942: make arraymodule.c free-thread safe #128943

Are you sure you want to change the base?

gh-128942: make arraymodule.c free-thread safe #128943

Conversation

tom-pytel commented Jan 17, 2025 • edited by bedevere-app bot Loading

bedevere-app bot commented Jan 17, 2025

tom-pytel commented Jan 17, 2025

ZeroIntensity left a comment

Choose a reason for hiding this comment

tom-pytel commented Jan 17, 2025 • edited Loading

erlend-aasland commented Jan 17, 2025

erlend-aasland commented Jan 17, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erlend-aasland Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tom-pytel commented Jan 18, 2025

ZeroIntensity left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tom-pytel commented Jan 17, 2025 •

edited by bedevere-app bot

Loading

tom-pytel commented Jan 17, 2025 •

edited

Loading

erlend-aasland Jan 17, 2025 •

edited

Loading