Skip to content

pytypes: Resurrect via leak instead of un-finalize in Python 3.8 #39

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 17 additions & 12 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,14 @@ matrix:
# - $PY_CMD -m sphinx -W -b html docs docs/.build
- |
# Make sure setup.py distributes and installs all the headers
set -ex
$PY_CMD setup.py sdist
$PY_CMD -m pip install --user -U ./dist/*
installed=$($PY_CMD -c "import pybind11; print(pybind11.get_include(True) + '/pybind11')")
diff -rq $installed ./include/pybind11
- |
# Barebones build
set -ex
cmake -DCMAKE_BUILD_TYPE=Debug -DPYBIND11_WERROR=ON -DDOWNLOAD_CATCH=ON -DPYTHON_EXECUTABLE=$(which $PY_CMD) .
make pytest -j 2 && make cpptest -j 2
# The following are regular test configurations, including optional dependencies.
Expand Down Expand Up @@ -124,12 +126,14 @@ matrix:
# TODO: remove next before_install, install and script clause when the wheels become available
before_install:
- pyenv global $(pyenv whence 2to3) # activate all python versions
- PY_CMD=python3
- PY_CMD=python3.8
- $PY_CMD -m ensurepip --user
- $PY_CMD -m pip install --user --upgrade pip wheel setuptools
install:
- $PY_CMD -m pip install --user --upgrade pytest
script:
- |
set -ex
# Barebones build
cmake -DCMAKE_BUILD_TYPE=Debug -DPYBIND11_WERROR=ON -DDOWNLOAD_CATCH=ON -DPYTHON_EXECUTABLE=$(which $PY_CMD) .
make pytest -j 2 && make cpptest -j 2
Expand All @@ -141,17 +145,18 @@ matrix:
name: Python 3.7, c++14, AppleClang 9, Debug build
osx_image: xcode9.4
env: PYTHON=3.7 CPP=14 CLANG DEBUG=1
# Test a PyPy 2.7 build
- os: linux
dist: trusty
env: PYPY=5.8 PYTHON=2.7 CPP=11 GCC=4.8
name: PyPy 5.8, Python 2.7, c++11, gcc 4.8
addons:
apt:
packages:
- libblas-dev
- liblapack-dev
- gfortran
# WARNING: This current fails on download. Should try to upgrade OS + Python version?
# # Test a PyPy 2.7 build
# - os: linux
# dist: trusty
# env: PYPY=5.8 PYTHON=2.7 CPP=11 GCC=4.8
# name: PyPy 5.8, Python 2.7, c++11, gcc 4.8
# addons:
# apt:
# packages:
# - libblas-dev
# - liblapack-dev
# - gfortran
# Build in 32-bit mode and tests against the CMake-installed version
- os: linux
dist: trusty
Expand Down
8 changes: 8 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,21 @@ This is a fork of the [official `pybind/pybind11` repository](https://github.com
All the links and badges within this document may link back to the official
repository.

This fork's CI badge:

[![Build Status](https://travis-ci.com/RobotLocomotion/pybind11.svg?branch=drake)](https://travis-ci.com/github/RobotLocomotion/pybind11)

# pybind11 — Seamless operability between C++11 and Python

<!--
Drake Fork: Hide upstream CI badges.

[![Documentation Status](https://readthedocs.org/projects/pybind11/badge/?version=master)](http://pybind11.readthedocs.org/en/master/?badge=master)
[![Documentation Status](https://readthedocs.org/projects/pybind11/badge/?version=stable)](http://pybind11.readthedocs.org/en/stable/?badge=stable)
[![Gitter chat](https://img.shields.io/gitter/room/gitterHQ/gitter.svg)](https://gitter.im/pybind/Lobby)
[![Build Status](https://travis-ci.org/pybind/pybind11.svg?branch=master)](https://travis-ci.org/pybind/pybind11)
[![Build status](https://ci.appveyor.com/api/projects/status/riaj54pn4h08xy40?svg=true)](https://ci.appveyor.com/project/wjakob/pybind11)
-->

**pybind11** is a lightweight header-only library that exposes C++ types in Python
and vice versa, mainly to create Python bindings of existing C++ code. Its
Expand Down
14 changes: 11 additions & 3 deletions README_DRAKE.md
Original file line number Diff line number Diff line change
Expand Up @@ -43,20 +43,28 @@ source code (e.g. no whitespace reflowing), and try to stay relatively close to
For simplicity, these checks are copied from upstream's CI which uses Travis
CI as part of GitHub's Checks. They test:

* Ubuntu and macOS
* Ubuntu and macOS (Windows tests disabled)
* C++11, C++14, and C++17
* Release and debug builds
* GCC 4.8, 6, and 7
* clang 7
* Apple clang 7.3 and 9
* 64bit and 32bit
* CPython and PyPy
* Python 2.7, 3.5, 3.6, and 3.7
* CPython and PyPy (though PyPy is partially supported on this fork)
* Python 2.7, 3.5, 3.6, 3.7, and 3.8

To see builds, see [this fork's Travis CI page](https://travis-ci.com/RobotLocomotion/pybind11/branches).

Windows testing (with AppVeyor) is disabled for this repository.

### Quick Testing

mkdir build && cd build
cmake .. \
-DPYTHON_EXECUTABLE=$(which python3) \
-DPYBIND11_TEST_OVERRIDE='test_builtin_casters.cpp;test_class.cpp;test_eigen.cpp;test_multiple_inheritance.cpp;test_ownership_transfer.cpp;test_smart_ptr.cpp'
make -j 4 pytest

## Local Git Setup

For development, please make your own GitHub fork of the upstream repository.
Expand Down
36 changes: 25 additions & 11 deletions include/pybind11/pytypes.h
Original file line number Diff line number Diff line change
Expand Up @@ -1422,7 +1422,7 @@ class wrapper : public Base {
}
holder_type_id_ = holder_type_id;
patient_ = std::move(patient);
// @note It would be nice to put `revive_python3` here, but this is called by
// @note It would be nice to put `resurrect_python3` here, but this is called by
// `PyObject_CallFinalizer`, which will end up reversing its effect anyways.
}

Expand All @@ -1431,7 +1431,7 @@ class wrapper : public Base {
if (!lives_in_cpp()) {
throw std::runtime_error("Instance does not live in C++");
}
revive_python3();
resurrect_python3();
// Remove existing reference.
object tmp = std::move(patient_);
assert(!patient_);
Expand Down Expand Up @@ -1462,16 +1462,27 @@ class wrapper : public Base {
}
}

// Python3 unfortunately will not implicitly call `__del__` multiple times,
// even if the object is resurrected. This is a dirty workaround.
// @see https://bugs.python.org/issue32377
inline void revive_python3() {
#if PY_VERSION_HEX >= 0x03000000
// Reverse single-finalization constraint in Python3.
if (_PyGC_FINALIZED(patient_.ptr())) {
// Handle PEP 442, implemented in Python3, where resurrection more than once
// is a bit more dicey.
inline void resurrect_python3() {
#if PY_VERSION_HEX >= 0x03080000
// Leak it as a means to stay alive for now.
// See: https://bugs.python.org/issue40240
if (_PyGC_FINALIZED(patient_.ptr())) {
if (leaked_) {
throw std::runtime_error("__del__ called twice in Python 3.8+?");
}
leaked_ = true;
patient_.inc_ref();
}
#elif PY_VERSION_HEX >= 0x03000000
// Reverse single-finalization constraint in Python3.
// This was a really dirty workaround:
// See: https://bugs.python.org/issue32377
if (_PyGC_FINALIZED(patient_.ptr())) {
_PyGC_SET_FINALIZED(patient_.ptr(), 0);
}
#endif // PY_VERSION_HEX >= 0x03000000
}
#endif // PY_VERSION_HEX >= 0x03080000
}

private:
Expand All @@ -1483,6 +1494,9 @@ class wrapper : public Base {

object patient_;
detail::HolderTypeId holder_type_id_{detail::HolderTypeId::Unknown};
#if PY_VERSION_HEX >= 0x03080000
bool leaked_{false};
#endif // PY_VERSION_HEX >= 0x03080000
};

NAMESPACE_BEGIN(detail)
Expand Down
37 changes: 26 additions & 11 deletions tests/test_ownership_transfer.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
from pybind11_tests import ConstructorStats

import pytest
import sys
import weakref


Expand Down Expand Up @@ -44,7 +45,15 @@ def get_cstats():
# capturing deletion properly.
@pytest.unsupported_on_pypy
def test_shared_ptr_derived_slicing(capture):
from sys import getrefcount
leaked_count = [0]
is_py38 = sys.version_info[:2] >= (3, 8)

def py38_leak():
if is_py38:
leaked_count[0] += 1

def cstats_alive_except_leaked():
return cstats.alive() - leaked_count[0]

# [ Bad ]
cstats = ChildBad.get_cstats()
Expand Down Expand Up @@ -75,42 +84,48 @@ def test_shared_ptr_derived_slicing(capture):
assert cstats.alive() == 1
assert obj.value() == 100
del obj
assert cstats.alive() == 0
py38_leak()
assert cstats_alive_except_leaked() == 0
# Use something more permanent.
obj = Child() # Use factory method.
obj_weak = weakref.ref(obj)
assert cstats.alive() == 1
assert cstats_alive_except_leaked() == 1
c = m.BaseContainer(obj)
del obj
assert cstats.alive() == 1
assert cstats_alive_except_leaked() == 1
# We now still have a reference to the object. py::wrapper<> will intercept Python's
# attempt to destroy `obj`, is aware the `shared_ptr<>.use_count() > 1`, and will increase
# the ref count by transferring a new reference to `py::wrapper<>` (thus reviving the object,
# per Python's documentation of __del__).
assert obj_weak() is not None
assert cstats.alive() == 1
assert cstats_alive_except_leaked() == 1
# This goes from C++ -> Python, and then Python -> C++ once this statement has finished.
assert c.get().value() == 100
assert cstats.alive() == 1
assert cstats_alive_except_leaked() == 1
# Destroy references (effectively in C++), and ensure that we have the desired behavior.
del c
assert cstats.alive() == 0
py38_leak()
assert cstats_alive_except_leaked() == 0

# Ensure that we can pass it from Python -> C++ -> Python, and ensure that C++ does not think
# that it has ownership.
obj = Child(10)
c = m.BaseContainer(obj)
del obj
assert cstats.alive() == 1
assert cstats_alive_except_leaked() == 1
obj = c.get()
# Now that we have it in Python, there should only be 1 Python reference, since
# py::wrapper<> in C++ should have released its reference.
assert getrefcount(obj) == 2
expected_refcount = 2
if is_py38:
expected_refcount += 1
assert sys.getrefcount(obj) == expected_refcount
del c
assert cstats.alive() == 1
assert cstats_alive_except_leaked() == 1
assert obj.value() == 100
del obj
assert cstats.alive() == 0
py38_leak()
assert cstats_alive_except_leaked() == 0


@pytest.unsupported_on_pypy
Expand Down