Skip to content
This repository has been archived by the owner on May 6, 2024. It is now read-only.

Fix a bad memory leak. #115

Merged
merged 1 commit into from
Nov 5, 2020
Merged

Fix a bad memory leak. #115

merged 1 commit into from
Nov 5, 2020

Conversation

heiner
Copy link
Contributor

@heiner heiner commented Nov 5, 2020

It turns out I misunderstood what pybind11's py::array::ensure does.
Looking at the code superficially (https://github.com/pybind/pybind11/blob/master/include/pybind11/numpy.h#L785)
I assumed this function steals a reference, hence the call to
release(). But the reference is being stolen from the return value
of raw_array(), which returns a new reference. Hence ensure() itself
does not steal anything but in fact increases the refcount (and will
decref it in ~array).

Thanks to @yobibyte for both finding this issue and insisting that
it happens on the Python side (I thought this might be related to
malloc calls in NetHack itself).

It turns out I misunderstood what pybind11's py::array::ensure does.
Looking at the code superficially (https://github.com/pybind/pybind11/blob/master/include/pybind11/numpy.h#L785)
I assumed this function steals a reference, hence the call to
release(). But the reference is being stolen from the return value
of raw_array(), which returns a new reference. Hence ensure() itself
does not steal anything but in fact increases the refcount (and will
decref it in ~array).

Thanks to Vitaly Kurin for both finding this issue and insisting that
it happens on the Python side (I thought this might be related to
malloc calls in NetHack itself).
@heiner heiner requested review from yobibyte and cdmatters November 5, 2020 21:14
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 5, 2020
Copy link
Contributor

@yobibyte yobibyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing that, @heiner!

@heiner heiner merged commit f9253f2 into master Nov 5, 2020
@heiner heiner deleted the bug-hotfix branch November 5, 2020 21:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants