This can still crash and produce data races if the sequence gets mutated but the size remains same. Inside the loop, the items of sequences are accessed using borrowed references (PySequence_Fast_GET_ITEM) so if the critical section gets suspended and other thread sets a new object at current index then it can crash with use-after-free.

You should be able to reproduce this case with a test which tries to create an ndarray from a list whose size is fixed but it getting concurrently modified by other thread with list.__setitem__.

To fix that would I need to use an API that returns a strong reference or could I just INCREF after getting the borrowed reference?

You can incref the item after getting the borrowed reference but it would cause a lot of reference counting contention. I don't think that would scale on free-threading builds.

I guess I'm trying to ask: how would you suggest fixing the issue?

You can try to further reduce the critical section and check the size after each iteration, so something like:

for (int i = 0; i = original_length; i++) {
NPY_BEGIN_CRITICAL_SECTION_SEQUENCE_FAST(orig_obj);
if (check length changed) {
    raise size changed exception
}
PyObject *value = PySequence_Fast_GET_ITEM(obj, i);
INCREF(value);
END_CRITICAL_SECTION();
// rest of loop
}

The strong reference is still needed though but I think with the reduced critical section, it would not cause a lot of contention.

I implemented your suggestion and added a new test but I was never able to trigger a race as you described. Do you have any idea why the test I modify in this PR doesn't trigger races using the old code that you said was buggy?

Either way, IMO it's way clearer with the critical section is as short as possible like I have it here. I also think doing the length check every iteration makes sense - I think it may be possible even on the GIL-enabled build for an unlucky thread switch to trigger issues that would be caught by the new repeated check.

critical sections are nop on default builds and error can happen there as well, I suggest to name it generic like err

You should be passing mutation_func here, no?

or do you trust the compiler warnings on this one?

You likely did, but can you explain why not obj? If PySequence_Fast did convert to tuple, we'll have our own object anyway?
(Ideally in the comment, I guess.)

Also, can't we assume that a fast-sequence will not change its size if we hold a critical section on it? (But then, I guess critical sections are not fool-proof, so maybe that is a silly thought.)

When handed a list, PySequence_Fast returns a proxy object with its own per-object lock. If we passed obj in, then we'd lock the PySequence_Fast proxy object and not the original list.

We could check for this in the CRITICAL_SECTION_SEQUENCE_FAST macros but then we'd be doing a check every time we enter the critical section. This is an optimization that I copied from CPython where these macros originally came from.

Also, can't we assume that a fast-sequence will not change its size if we hold a critical section on it? (But then, I guess critical sections are not fool-proof, so maybe that is a silly thought.)

It can't change while we're executing code in the body of the critical section. It also can't change at all if we never call back into the CPython C API.

Here we are calling back into the CPython C API, but we're using the low-level accessor macros which do no error checking and do not cause the critical section to be released. In principle a future version of Python could decide to change that but I doubt that would happen because the property that critical sections aren't released by low-level accessor macros is being used in lots of places.

The rule of thumb I have in my head is that a critical section can be released in exactly the same places where GIL switches are possible (e.g. if the interpreter enters a possibly blocking call). As long as we ensure there are no possibly blocking calls while we hold the critical section, then it behaves like a "real" lock and does not get released by code called inside the critical section body.

When handed a list, PySequence_Fast returns a proxy object with its own per-object lock

Aha, I really thought that it just returns the original list in that case, not a proxy that can be mutated by mutating the original list...
Well, I'll let you decide whether you want to explain that.

I am a bit confused is it really important for the relevant use-case to unlock the criticial section on every iteration?

Yes, it's possible that the size gets changed by another thread while the critical section is released. It could happen any time outside of the critical section if the list is shared with other threads.

Sorry, my question was that I am surprised that contention of the list objects with multiple threads is actually a very important use-case (i.e. we can't just lock the whole iteration).

That's the way it works right now, but in a very silly way where we unconditionally acquire a critical section for lists and tuples. That's the critical section I'm deleting in PyArray_FromAny_int down on line 1571 below here.

I observed contention in the benchmark from #30494 - you can see it in the screenshot from #30494 (comment).

This PR fixes that contention because the critical sections are held for a much shorter period of time in this PR.

It also gives people an escape hatch if it does ever come up again: pass a tuple instead of a list.

OK, sorry for the noise, it wasn't (isn't) quite obvious why that script needs this use-case, but fine with it. The question would really be the opposite I guess: Would this make:

l = [0] * 100_000
%timeit np.array(l)

much slower in the uncontested case. And if it doesn't all of this is moot anyway. If it is much slower, I suppose this may still be the more interesting thing (i.e. ensure we scale well on free-threading, at least for now that is hopefully how things are used)