Nicer API to pass py::capsule destructor by wjakob · Pull Request #752 · pybind/pybind11

wjakob · 2017-03-20T20:57:15Z

The current way of specifying a destructor for py::capsule instances strikes me as quite inelegant (the Python C API leaks through, requiring the user to deal with PyCapsule_GetPointer). This PR changes the convention.

virtuald · 2017-03-20T21:01:23Z

I'm already using it in a module deployed to pypi.. any chance you can add a macro to detect the API change? :(

wjakob · 2017-03-20T21:19:25Z

If it becomes part of 2.1 or 2.2, you can of course #ifdef based on PYBIND11_VERSION_MAJOR and PYBIND11_VERSION_MINOR

dean0x7d

Nice, this definitely also removes the ambiguity of whether to borrow or steal that PyObject*.

dean0x7d · 2017-03-20T21:26:16Z

+ pybind11_fail("Could not allocate capsule object!");
+ }
+
+ explicit capsule(const void *value, void (*destructor)(void *)) {


explicit isn't needed here.

dean0x7d · 2017-03-20T21:28:29Z

 PYBIND11_OBJECT_DEFAULT(capsule, object, PyCapsule_CheckExact)
 PYBIND11_DEPRECATED("Use reinterpret_borrow<capsule>() or reinterpret_steal<capsule>()")
 capsule(PyObject *ptr, bool is_borrowed) : object(is_borrowed ? object(ptr, borrowed) : object(ptr, stolen)) { }
- explicit capsule(const void *value, void (*destruct)(PyObject *) = nullptr)


Maybe just deprecate this constructor for backward compatibility? (Just without the = nullptr default and explicit).

jagerman · 2017-03-20T21:48:11Z

+ pybind11_fail("Could not allocate capsule object!");
+ }
+
+ explicit capsule(const void *value, void (*destructor)(void *)) {


I was thinking it would be nice to make this take a function with a (deduced) T * argument, with a reinterpret cast of the capsule pointer to the deduced type when invoking the destructor. Unfortunately I think that would require some special handling to accept a lambda, so might not be worthwhile.

That's what I had implemented at first (with an extra closure object stored in the capsule context), but it struck me as a bit overkill.

(In any case, that sort of thing can still be added later on if there a good case for this functionality can be made. This PR is about preventing fallout from a problematic API that is about to be documented.)

wjakob · 2017-03-20T21:51:23Z

Thanks @dean0x7d, I added a deprecated constructor now. I'll merge this into 2.1 unless there are objections.

jagerman · 2017-03-20T21:51:46Z

No objections.

dean0x7d · 2017-03-20T21:56:39Z

I'm not sure how common the use case of #733 is -- would it warrant adding a dedicated constructor? E.g. explicit capsule(void (*destructor)()) where the "dummy" pointer would be handled internally.

jagerman · 2017-03-20T21:59:00Z

I think it would be nice to have. I don't understand why Python imposes != NULL on the pointer in the first place, but I don't see why we should be subject to the same limitation.

virtuald · 2017-03-20T22:19:57Z

Well, the only reason why #733 uses a capsule is because module destructors aren't exposed. If they were, then it would just be added to wherever the module docs are and wouldn't need to worry about capsules.

Of course, only Python 3 supports module destructors IIRC, so you'd need this hack anyways -- but maybe it could be done internally?

jagerman · 2017-03-21T16:11:59Z

Module destructors, using this as a workaround for Python 2, seems like a useful addition, but can be added later (i.e. for 2.2).

jagerman · 2017-03-22T18:50:35Z

This is the last 2.1 PR; I think it's good as is, but the dummy-pointer version might also be useful. (Though I think it can wait as well, if you prefer to merge without it).

wjakob · 2017-03-22T19:01:33Z

I had the same thought and just added it.

wjakob · 2017-03-22T19:16:34Z

Added docs & the destructor-only capsules.

wjakob · 2017-03-22T19:27:43Z

That's a pretty weird PyPy failure -- it looks like the pytest capture object is not working.

jagerman · 2017-03-22T20:09:17Z

Rebase against current master: I just pushed a change to use pypy 5.7 instead of the 5.8 nightly in the travis-ci test, which fixes the PyPy build.

wjakob · 2017-03-22T20:12:03Z

thanks -- done.

jagerman · 2017-03-22T20:12:12Z

I just turned on this cool new travis-ci setting for pybind11 PR builds: https://blog.travis-ci.com/2017-03-22-introducing-auto-cancellation

wjakob · 2017-03-22T20:17:06Z

Cool -- that's a useful feature indeed!

jagerman · 2017-03-22T20:42:03Z

Hmm, that didn't fix the PyPy test failure. I wonder if you could work around it by not using a capture, perhaps by setting/checking a static variable?

dean0x7d · 2017-03-22T20:44:39Z

Doesn't PyPy need a couple of GC calls after a del?

wjakob · 2017-03-22T20:45:01Z

that's right -- I forgot about that.

dean0x7d · 2017-03-22T20:47:36Z

+ a = m.return_capsule_with_destructor_2()
+ del a
+ pytest.gc_collect()
+ print(capture)


Leftover from debugging.

wjakob · 2017-03-22T20:49:08Z

that cancel feature is already starting to pay off :)

wjakob · 2017-03-22T21:03:40Z

Ok, that did it!

@wjacob

Use `PyCapsule_Destructor` (part of the stable Python ABI) instead of spelling out the C `typedef`. The deprecation message is misleading. Replace with a message pointing to another existing ctor. Background: According to @wjacob the original motivation for deprecating the ctor (in PR pybind#752) was to hide Python C API details, but PR pybind#902 brought those back with a new ctor, it cannot be avoided. Having a `PyCapsule_Destructor` or a `void (*destructor)(void *)` are two separate and valid use cases.

@wjakob

Use `PyCapsule_Destructor` (part of the stable Python ABI) instead of spelling out the C `typedef`. The deprecation message is misleading. Replace with a message pointing to another existing ctor. Background: According to @wjakob the original motivation for deprecating the ctor (in PR pybind#752) was to hide Python C API details, but PR pybind#902 brought those back with a new ctor, it cannot be avoided. Having a `PyCapsule_Destructor` or a `void (*destructor)(void *)` are two separate and valid use cases.

@wjakob

Use `PyCapsule_Destructor` (part of the stable Python ABI) instead of spelling out the C `typedef`. The deprecation message is misleading. Replace with a message pointing to another existing ctor. Background: According to @wjakob the original motivation for deprecating the ctor (in PR #752) was to hide Python C API details, but PR #902 brought those back with a new ctor, it cannot be avoided. Having a `PyCapsule_Destructor` or a `void (*destructor)(void *)` are two separate and valid use cases.

wjakob mentioned this pull request Mar 20, 2017
Add note about module destructors #733
Closed

dean0x7d reviewed Mar 20, 2017
View reviewed changes

jagerman reviewed Mar 20, 2017
View reviewed changes

wjakob added this to the v2.1 milestone Mar 20, 2017

wjakob added a commit to wjakob/pybind11 that referenced this pull request Mar 22, 2017
added documentation for module destructors (fixespybind#752)
c053462

wjakob force-pushed the capsule_destructor branch from c053462 to 15c8e40Compare March 22, 2017 19:14

jagerman force-pushed the master branch from cc88278 to 773339fCompare March 22, 2017 19:18

wjakob added 5 commits March 22, 2017 21:11

nicer py::capsule destructor mechanism
7332300

incorporated feedback by@dean0x7d
af46e11

added destructor-only version of capsule & tests
00e864d

added documentation for module destructors (fixespybind#733)
b63f1b1

style fix
601f4aa

wjakob force-pushed the capsule_destructor branch from bbe8715 to 601f4aaCompare March 22, 2017 20:11

added gc_collect calls
5817886

dean0x7d reviewed Mar 22, 2017
View reviewed changes

cleanup leftover from debugging
7f6262d

wjakob merged commit b16421e into pybind:masterMar 22, 2017

wjakob deleted the capsule_destructor branch March 22, 2017 21:04

jagerman mentioned this pull request Mar 22, 2017
Proposed changelog for v2.1.0 #759
Merged

wjakob restored the capsule_destructor branch March 22, 2017 21:07

rwgk mentioned this pull request Oct 10, 2022
fix: Improve PyCapsule exception handling #4232
Merged

rwgk mentioned this pull request Oct 12, 2022
Minor py::capsule cleanup. No functional change. #4238
Merged

rwgk mentioned this pull request Feb 9, 2023
FWD pybind11 google/pybind11clif#752
Closed

Uh oh!

Conversation

wjakob commented Mar 20, 2017

Uh oh!

virtuald commented Mar 20, 2017

Uh oh!

wjakob commented Mar 20, 2017

Uh oh!

dean0x7d left a comment

Choose a reason for hiding this comment

Uh oh!

dean0x7dMar 20, 2017

Choose a reason for hiding this comment

Uh oh!

dean0x7dMar 20, 2017

Choose a reason for hiding this comment

Uh oh!

wjakobMar 20, 2017

Choose a reason for hiding this comment

Uh oh!

jagermanMar 20, 2017

Choose a reason for hiding this comment

Uh oh!

wjakobMar 20, 2017

Choose a reason for hiding this comment

Uh oh!

wjakobMar 20, 2017

Choose a reason for hiding this comment

Uh oh!

wjakob commented Mar 20, 2017

Uh oh!

jagerman commented Mar 20, 2017

Uh oh!

dean0x7d commented Mar 20, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jagerman commented Mar 20, 2017

Uh oh!

virtuald commented Mar 20, 2017

Uh oh!

jagerman commented Mar 21, 2017

Uh oh!

jagerman commented Mar 22, 2017

Uh oh!

wjakob commented Mar 22, 2017

Uh oh!

wjakob commented Mar 22, 2017

Uh oh!

wjakob commented Mar 22, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jagerman commented Mar 22, 2017

Uh oh!

wjakob commented Mar 22, 2017

Uh oh!

jagerman commented Mar 22, 2017

Uh oh!

wjakob commented Mar 22, 2017

Uh oh!

jagerman commented Mar 22, 2017

Uh oh!

dean0x7d commented Mar 22, 2017

Uh oh!

wjakob commented Mar 22, 2017

Uh oh!

dean0x7dMar 22, 2017

Choose a reason for hiding this comment

Uh oh!

wjakob commented Mar 22, 2017

Uh oh!

wjakob commented Mar 22, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dean0x7d commented Mar 20, 2017•
edited
Loading

wjakob commented Mar 22, 2017•
edited
Loading