BUG: Fix `dtype` refcount in `array` #29715

mtsokol · 2025-09-08T17:26:55Z

This PR fixes a refcount bug raised in #29707 and adds a test which fails on main branch for linux, x86.

ngoldbaum

LGTM but I'll let someone else look this over before merging

numpy/_core/src/multiarray/methods.c

numpy/_core/tests/test_api.py

ngoldbaum · 2025-09-10T15:23:59Z

@benburrill thanks for the code review help!

The reference counting here is tricky - I'd like @seberg to look this over. He's on vacation this week so might not look at it until next week.

mtsokol · 2025-09-10T16:00:20Z

Sure, let's wait for another pair of eyes. In general, PyArg_ParseTupleAndKeywords increments refcount for newtype, and PyArray_CastToType steals it - therefore the standard path where we pass dtype doesn't alter refcount.

When we don't pass it, and have copy=True, we need an additional Py_INCREF(newtype); to even out PyArray_CastToType.

When we pass it (and is the same as array's) and have copy=None, we have one refcount extra from PyArg_ParseTupleAndKeywords parsing, ergo we need an additional Py_XDECREF(newtype); for that path to even out missing stealing call (which is only present later, when newtype is present and differs from array's dtype).

The test covers: dtype(none | same | different) x copy(true | false) (6) scenarios.

ngoldbaum · 2025-09-10T16:03:57Z

I think it would help if you added an edited-down subset of that last comment as a comment in the source code. Probably some of it makes more sense as a comment in the test and some of it makes sense as a comment in the __array__ implementation.

benburrill · 2025-09-10T17:21:48Z

In general, PyArg_ParseTupleAndKeywords increments refcount for newtype

Just to be slightly more precise, PyArray_DescrConverter2 is (as I understand it) what's actually responsible for incrementing the refcount for newtype, not PyArg_ParseTupleAndKeywords itself.

mtsokol · 2025-09-10T17:29:38Z

Correct, PyArray_DescrConverter2 converter function which is passed to PyArg_ParseTupleAndKeywords.

I approved this with a bug

benburrill · 2025-09-10T17:37:53Z

Yeah, I just thought the wording could potentially be a bit misleading to readers (if you're planning to put something similar in a comment in the source code), since although

In general, PyArg_ParseTupleAndKeywords increments refcount for newtype

is true for this specific code,

In general, PyArg_ParseTupleAndKeywords increments refcount

is not true.

benburrill · 2025-09-11T20:49:45Z

numpy/_core/tests/test_api.py

+            return self.val.__array__(dtype=dtype, copy=copy)
+
+    # test all possible scenarios:
+    # dtype(none | same | different) x copy(true | false)


I just realized you haven't fully tested the COPY_NEVER case. Really, you should be testing copy(True | False | None). Currently, you're actually testing copy(True | None).

This test fails:

with pytest.raises(ValueError): np.array(MyArray(dt), dtype=dt2, copy=False) assert_equal(old_refcount2, sys.getrefcount(dt2))

Adding Py_DECREF(newtype); to the npy_no_copy_err_msg branch fixes it.

I'm also suspicious about the if (!PyArray_CheckExact(self)) branch (I think at the very least there should be Py_XDECREF(newtype) before the return NULL;, and possibly more than that), but I'm not totally sure how to test it.

Right, copy=False when dtypes are different was a path missed in the test - thanks!

About PyArray_CheckExact: Py_XDECREF was clearly missing there, but I think it's already an esoteric path, as PyArray_NewFromDescrAndBase needs to fail and set an exception.

seberg

LGTM, although I think the new comments are more verbose than helpful. Half the paths (at least the error paths) have no comment. What is useful, IMO, is that CastToType steals the reference (because reference stealing is confusing API basically).

If we want to really clarify this all, I think the right thing would be to simplify the code instead...

Swap self and new naming. Because new is clearly a new reference ;), while self is suprisingly a new reference.
One could move newtype == NULL logic to the top directly after the argument parsing, that way it get's much clearer why the Incref is needed. (at the cost of an unnecessary incref/decref.)
The best way to avoid piling up DECREF() before the return would be to just make it goto finish; that has the logic. But since that requires adding newtype = NULL; // Stolen by CastToType (or another incref), I am not sure it's nicer. It would be clearer though.

Anyway, besides thinking that the comments are not useful except as comments during review, this looks right to me.

seberg · 2025-09-16T09:03:19Z

numpy/_core/src/multiarray/methods.c

+            // Take a new strong reference to compensate for
+            // PyArray_CastToType stealing a reference to newtype.
+            Py_INCREF(newtype);


Suggested change

// Take a new strong reference to compensate for

// PyArray_CastToType stealing a reference to newtype.

Py_INCREF(newtype);

Py_INCREF(newtype); // newtype is owned.

Maybe. I don't like the overly long comments, TBH.

We could add a comment on all CastToType calls that it steals the reference, though.

Done! Also added CastToType comments.

seberg · 2025-09-16T09:05:59Z

numpy/_core/src/multiarray/methods.c

+            // If newtype isn't NULL then release a strong reference
+            // introduced by PyArray_DescrConverter2 in arg parsing.


Suggested change

// If newtype isn't NULL then release a strong reference

// introduced by PyArray_DescrConverter2 in arg parsing.

I disagree that it is helpful to add it here when it's not everywhere else.

mtsokol · 2025-09-16T22:16:09Z

@seberg thanks for feedback! I found moving newtype == NULL check to the top really useful - now it's clearer how we count refs here. I decided skip the other two points if it's OK.

numpy/_core/src/multiarray/methods.c

seberg · 2025-09-17T06:33:36Z

Nice, thanks! Yeah, I didn't care about doing all of those things, more of a rambling that in my opinion code reorganization would do more good than long comments :).

* BUG: Fix `dtype` refcount in `__array__` * Consider all possible code paths * Remove else-if branch * Move refcount checks to a separate test * Add code comments * Add missing `Py_DECREF` for error path * Apply review comments

BUG: Fix `dtype` refcount in `__array__` (#29715)

* BUG: Fix `dtype` refcount in `__array__` * Consider all possible code paths * Remove else-if branch * Move refcount checks to a separate test * Add code comments * Add missing `Py_DECREF` for error path * Apply review comments

BUG: Fix dtype refcount in __array__

2214ad2

mtsokol self-assigned this Sep 8, 2025

mtsokol added the 00 - Bug label Sep 8, 2025

mtsokol mentioned this pull request Sep 8, 2025

BUG: Reference counting problem on dtypes with __array__ #29707

Closed

mtsokol linked an issue Sep 8, 2025 that may be closed by this pull request

BUG: Reference counting problem on dtypes with __array__ #29707

Closed

ngoldbaum previously approved these changes Sep 8, 2025

View reviewed changes

charris added the 09 - Backport-Candidate PRs tagged should be backported label Sep 9, 2025

benburrill reviewed Sep 9, 2025

View reviewed changes

numpy/_core/src/multiarray/methods.c Outdated Show resolved Hide resolved

Consider all possible code paths

107d419

ngoldbaum reviewed Sep 9, 2025

View reviewed changes

numpy/_core/src/multiarray/methods.c Outdated Show resolved Hide resolved

Remove else-if branch

1270626

benburrill reviewed Sep 9, 2025

View reviewed changes

numpy/_core/tests/test_api.py Outdated Show resolved Hide resolved

Move refcount checks to a separate test

72fa4cf

Add code comments

9049949

benburrill reviewed Sep 11, 2025

View reviewed changes

Add missing Py_DECREF for error path

f806882

seberg approved these changes Sep 16, 2025

View reviewed changes

Apply review comments

7502650

seberg reviewed Sep 17, 2025

View reviewed changes

numpy/_core/src/multiarray/methods.c Outdated Show resolved Hide resolved

Apply suggestions from code review

24cdfa7

seberg merged commit c9e7592 into numpy:main Sep 17, 2025
77 checks passed

mtsokol deleted the ms/fix-__array__-refcount branch September 17, 2025 07:19

charris mentioned this pull request Sep 23, 2025

BUG: Fix dtype refcount in __array__ (#29715) #29790

Merged

charris removed the 09 - Backport-Candidate PRs tagged should be backported label Sep 23, 2025

charris added a commit that referenced this pull request Sep 23, 2025

Merge pull request #29790 from charris/backport-29715

e1086a8

BUG: Fix `dtype` refcount in `__array__` (#29715)

		// If newtype isn't NULL then release a strong reference
		// introduced by PyArray_DescrConverter2 in arg parsing.

Uh oh!

BUG: Fix dtype refcount in __array__ #29715

BUG: Fix dtype refcount in __array__ #29715

Uh oh!

Conversation

mtsokol commented Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ngoldbaum left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ngoldbaum commented Sep 10, 2025

Uh oh!

mtsokol commented Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ngoldbaum commented Sep 10, 2025

Uh oh!

benburrill commented Sep 10, 2025

Uh oh!

mtsokol commented Sep 10, 2025

Uh oh!

benburrill commented Sep 10, 2025

Uh oh!

benburrill Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtsokol Sep 11, 2025

Choose a reason for hiding this comment

Uh oh!

seberg left a comment

Choose a reason for hiding this comment

Uh oh!

seberg Sep 16, 2025

Choose a reason for hiding this comment

Uh oh!

mtsokol Sep 16, 2025

Choose a reason for hiding this comment

Uh oh!

seberg Sep 16, 2025

Choose a reason for hiding this comment

Uh oh!

mtsokol Sep 16, 2025

Choose a reason for hiding this comment

Uh oh!

mtsokol commented Sep 16, 2025

Uh oh!

Uh oh!

seberg commented Sep 17, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

BUG: Fix `dtype` refcount in `array` #29715

BUG: Fix `dtype` refcount in `array` #29715

mtsokol commented Sep 8, 2025 •

edited

Loading

mtsokol commented Sep 10, 2025 •

edited

Loading

benburrill Sep 11, 2025 •

edited

Loading