Support keep_alive where nurse may be None

[GlenWalker]

For example keep_alive<0,1>() should work where the return value may sometimes be None. At present a "Could not allocate weak reference!" exception is thrown.

[wjakob]

Having a None nurse is hugely problematic because it causes a leak: the patient can never be garbage collected. Why would you want to do this? Or am I misunderstanding something?

[GlenWalker]

The nurse is checked against None before the patient reference count is incremented in keep_alive_impl, so I don't think there should be a problem with leaks.

I'm wrapping a C++ class with a method that may return an object pointer or nullptr depending on other factors. I'd like the Python wrapper to return an instance or None, which currently doesn't work with keep_alive<0,1>.

[wjakob]

You changed the statement to patient.ptr() == Py_None || nurse.ptr() == Py_None. This means that it's possible to have situations where patient is non-null but nurse is null, in which case keep_alive will self-deactivate with your change. Since this is a kind of exceptional situation that indicates a problem, I would strongly prefer to keep the error message.

[GlenWalker]

The situation where patient is non-null and nurse is null is exactly the situation I am targeting, and self-deactivation is the desired behaviour. When nullptr is returned no keep_alive is required, but if a valid pointer is returned the keep_alive is activated. Note that this behaviour is consistent with the behaviour of return_value_policy reference_internal when None is returned.

In the absense of this fix do you have any suggestions on how to wrap a function that may or may not return nullptr, and where the return value requires keep_alive<0,1>?

[GlenWalker]

I have to disagree that the situation is exceptional and indicates a problem - I have a valid use case wrapping a real-world C++ library where a method return value may sometimes be nullptr, but when it is not nullptr a keep_alive<0,1> is required.

If it is acceptable for patient to be None, acting as a no-op with no error raised, then it makes sense to me that it would be acceptable for nurse to be None, also acting as a no-op with no error raised. Logically "Nurse, please keep nobody alive" and "Nobody, please keep patient alive" both result in no keep-alive.

With this patch, having a None nurse becomes equivalent to having a valid nurse that is immediately garbage collected, in that the end result for both is a patient that is not kept alive by a nurse. To me, the following pseudo-code should have the same end-result, and I was surprised that they do not:

nurse = Nurse()
patient = Patient()
keep_alive(nurse, patient)
patient = None
nurse = None
gc.collect()

nurse = None
patient = Patient()
keep_alive(nurse, patient)
patient = None
gc.collect()

[jagerman]

Note that this behaviour is consistent with the behaviour of return_value_policy reference_internal when None is returned.

That's not actually true (anymore) in current master since f2ecd89: reference_internal is now implemented (and documented) as a shortcut for specifying both reference and keep_alive<0, 1>(). So this might actually have implications for backwards compatibility if people are using reference_internal with nullptr-returnable functions.

Boost.Python also appears to support this (presumably for the same reasons @GlenWalker has given):

If the custodian object does not support weak references and is not None, an appropriate exception will be thrown.

and so without this, we aren't quite equivalent to Boost.Python's with_custodian_and_ward and ..._postcall as claimed in the "Additional call policies" documentation.

On that note, if @wjakob decides to accept the change, it deserves an addition to docs/advanced.rst that documents the feature in the Additional call policies section: while I see the value after reading your posts, I don't find it immediately obvious whether it should work as you expect or in the current fashion, which is just the sort of thing that deserves a documentation sentence or two.

[wjakob]

That's a good point -- I forgot about the reference_internal case, where things are indeed broken now. @GlenWalker, can you add a sentence to the keep_alive docs in advanced.rst?

[GlenWalker]

Commit now includes an update to advanced.rst to clarify the behaviour of keep_alive (based on boost::python documentation of with_custodian_and_ward)