gh-155606: Increment the managed buffer export count atomically - #155882
gh-155606: Increment the managed buffer export count atomically#155882ayaangazali wants to merge 1 commit into
Conversation
mbuf_add_view() and mbuf_add_incomplete_view() registered a new view on the shared _PyManagedBufferObject with a plain mbuf->exports++. On free-threaded builds concurrent slices of one memoryview lose increments, so the count drops to zero while views are still alive and mbuf_release() frees the buffer early. The matching PyMemoryViewObject.exports counter already uses FT_ATOMIC_ADD_SSIZE; use it here too. On default builds the macro expands to a plain +=, so this is a no-op there.
|
All commit authors signed the Contributor License Agreement. |
BHUVANSH855
left a comment
BHUVANSH855
left a comment
There was a problem hiding this comment.
CLA test failing, kindly sign CLA so that maintainers can proceed with review process.
|
Thanks for the nudge. The CLA is with the account owner and is being sorted out, I'll comment here once it clears so nobody spends review time on a blocked PR. One note on the other red check while this sits: |
|
Following up as promised: the CLA now shows as signed, so this is unblocked and ready for review whenever someone has time. The one remaining red check,
That job has also been green on Worth repeating from the description since it is easy to miss: this fixes only the increment half of the |
memoryviewslicing registers a new view on the shared_PyManagedBufferObjectand bumps its export count. Both registration sites do that with a plain++:https://github.com/python/cpython/blob/main/Objects/memoryobject.c#L699-L703
mbuf_add_view()andmbuf_add_incomplete_view()are reached frommemory_subscript(), so slicing one shared memoryview from several threads is concurrent unsynchronised read-modify-write onmbuf->exports. Increments get lost, the count ends up lower than the number of live views, and dropping those views walks it through zero early.mbuf_release()then frees the buffer while views are still using it, which is theValueError: operation forbidden on released memoryview objectfrom gh-155606. On a debug build it tripsassert(self->mbuf->exports > 0)in_memory_release()first.The sibling counter,
PyMemoryViewObject.exports, is already atomic viaFT_ATOMIC_ADD_SSIZE(added in gh-127085). This does the same for the managed buffer's counter. On default builds the macro expands to a plain+=, so nothing changes there.Verifying
Free-threaded debug build,
--disable-gil --with-pydebug, 8 threads slicing one sharedmemoryview. I built each combination separately and ran each several times:mbuf->exports++fixedSo this is one of two independent races on the same counter and it is not enough on its own. The decrement side in
_memory_release()is already covered by #154770 (open, for gh-127716), and I confirmed that PR does not fix gh-155606 by itself either. No overlapping hunks between the two, they touch different functions. Flagging it because reviewing this diff alone would suggest gh-155606 is closed, and it is not until both land.The added test covers the increment on its own: slices are created concurrently but only dropped afterwards on a single thread, so the decrement race cannot contribute. It aborts on unpatched main and passes with just this change, 5 runs each. It is skipped on default builds.
Also ran
test_memoryview,test_buffer,test_free_threading,test_capion both a normal and a free-threaded build, plus-R 3:3ontest_memoryviewandtest_buffer. All clean.pre-commitpasses on the touched files.One thing I did not do: I have no ThreadSanitizer build here, so the race is evidenced by the assert and the failure counts above rather than a TSan report.
apologies if I've missed something obvious in here, I worked through the logic myself and talked the design decisions over with Claude Code as a sanity check. still a freshman in college so I'm sure there's plenty I don't know yet, and I'd genuinely like the correction if I've got it wrong somewhere :)