C'mon folks! MOD THIS UP

This is the lead author of the majorly potential impacted extension!

As far as I know PEP703 includes provisions to make operations on containers thread-safe. That should at least avoid most crashes.

Borrowed references can be more problematic, but it seems most cases could be fixed by replacing GetItem with FetchItem calls.

Overall, as another C Python extension, I don’t really think it’s going to be that much of a pain. In fact, I could even get away with no changes (other than build fixes and such) if, for example, I guarantee that each “main object instance” is only accessed by one thread, and I’d still get a lot of benefits from the nogil.

Here is code you see all the time in extensions:

    if (PyList_Size(lst) >= 3) {
        PyList_SetItem(lst, 0, ...);
        PyList_SetItem(lst, 1, ...);
        PyList_SetItem(lst, 2, ...);
    }

Whether the individual operations are thread-safe is irrelevant. For the code to be thread-safe it needs to acquire lst's lock before the if statement and release it afterwards.

The GIL does not protect that code anymore than it does this equivalent Python code:

  if len(lst) >= 3:
    lst[0] = "..."
    lst[1] = "..."
    lst[2] = "..."

Neither is thread safe with respect to ensuring that multiple threads don't modify lst in an interleaved fashion. If that's a critical section, then it has to be protected with a lock other than the GIL.

Your example is not thread safe because the GIL does not protect critical sections like that. The GIL only protects the Python internal interpreter state. PyList_SetItem discards a reference to the item being replaced. If the ref count of the item becomes zero, then the item's destructor is run. The destructor can release the GIL.

IOW, your code is not much different than the equivalent Python code:

  if len(lst) >= 3:
    lst[0] = "..."
    lst[1] = "..."
    lst[2] = "..."

You'd never expect that to be thread safe either without using something like a threading.RLock() around it.

References:

• https://docs.python.org/3.8/c-api/list.html#c.PyList_SetItem

This function “steals” a reference to item and discards a reference to an item already in the list at the affected position.

• https://docs.python.org/3/c-api/refcounting.html#c.Py_DECREF

Warning: The deallocation function can cause arbitrary Python code to be invoked (e.g. when a class instance with a __del__() method is deallocated).

• https://docs.python.org/3/extending/newtypes_tutorial.html#a...

But this would be risky. Our type doesn’t restrict the type of the first member, so it could be any kind of object. It could have a destructor that causes code to be executed that tries to access the first member; or that destructor could release the Global interpreter Lock and let arbitrary code run in other threads that accesses and modifies our object.

Let's look at the code:

1. PyList_SetItem calls Py_XSETREF(dst, src) where dst is the list position:

https://github.com/python/cpython/blob/ee46cb6aa959d891b0a48...

2. Py_XSETREF calls Py_XDECREF on the old item:

https://github.com/python/cpython/blob/ca8b55c7f54b38e264056...

3. Py_XDECREF calls Py_DECREF on non-Null objects:

https://github.com/python/cpython/blob/main/Include/object.h...

4. Py_DECREF can cause arbitrary code to run.

Yes, you are absolutely right, but that doesn't change the fact that many extensions are written as if innocuous function calls like PyList_SetItem won't context switch. It mostly works fine since custom destructors releasing the GIL are extremely rare.

"Mostly works fine."

I dunno, I'd rather have a reliable bug than a heisenbug.

Any class with a __del__() method is going to release the GIL. That's not uncommon. My intuition is that threaded Python is rarer than classes with a __del__() method.

Nothing between Release GIL and Reacquire GIL needs to change. Depending upon your extension, possibly nothing needs to change for the other steps either. Per Sam Gross:

> Most C API extensions don’t require any changes, and for those that do require changes, the changes are small. For example, for “nogil” Python I’m providing binary wheels for ~35 extensions that are slow or difficult to build from source and only about seven projects required code changes (PyTorch, pybind11, Cython, numpy, scikit-learn, viztracer, pyo3). For four of those projects, the code changes have already been contributed and adopted upstream. For comparison, many of those same projects also frequently require changes to support minor releases of CPython.

If you're using the GIL to protect access to non-Python objects, that will need to change.

The PEP mentions a future HOWTO on how to updating existing extensions. I wish that were already written.

There's disagreement among Python maintainers in the discussion thread on the PEP in how much work will be involved and I don't expect to resolve it here.

I don't know about the complexity of your extension, but the PEP provides per-container locks ("This PEP proposes using per-object locks to provide many of the same protections that the GIL provides. For example, every list, dictionary, and set will have an associated lightweight lock. All operations that modify the object must hold the object’s lock").

This is automatic via the PyList_GetItem/SetItem API, so I guess the error you're talking about is that you read a list y = [A, ...], your code reads A and copies the data to (new) A2, and then you iterate over it again and see that A != A2 because another thread has modified y?

Yeah, and then everyone has to have the same locking hierarchy, and be conscious of it.

This sounds like a nightmare waiting to happen.