2

I'm working with a legacy C library that I've wrapped with a Python C Extension. The C library has a recursive data structure Foo with an API similar to below:

Foo *Foo_create(void)  /* Create new Foo memory */

int Foo_push(Foo *parent, int field, Foo *child)  /* Add a child Foo to a parent Foo */

int Foo_destroy(Foo *foo) /* Framework will free all children, caller cannot reuse children after */

Foo *Foo_pop(Foo *parent, int field) /* User responsible for calling Foo_destroy on popped field */

I have a PyFoo struct that wraps Foo, something like:

typedef struct {
    PyObject_HEAD
    Foo *foo;
    PyObject *parent;
} PyFoo;

As well as other functions that wrap the Foo_* functions and incref/decref appropriately.

The problem I have come across is that it's possible for two independent PyFoo objects, with independent refcounts, to point to the same Foo *. If one of the PyFoo objects goes out of scope, it will call Foo_destroy, but the user may access the second PyFoo object and cause a segmentation fault.

I'm trying to prevent the user of my library from doing the following in Python:

parent = Foo()     # Foo_create();  parent's refcount is 1
a = Foo()          # Foo_create();  a's refcount is 1
parent[1] = a      # Foo_push(parent, 1, a); parent's refcount is 2; a's refcount is 1

b = parent.pop(1)  # Foo_pop(parent, 1); 
# parent's refcount is 1; a's refcount is 1; b's refcount is 1
# a and b's are now independent PyFoo objects with reference count = 1
# HOWEVER both of the *foo pointers point to the same memory

# Delete a, dropping reference count to 0, which calls Foo_destroy
del a              # parents refcount is 1; a's refcount is 0; b's refcount is 1

# Access b, which may segfault, since Foo_destroy was called in the last call.
print(b)

In other words, a and b both point to the same Foo memory. However, they are independent Python objects, with independent refcounts. Once a goes out of scope, it will destroy the memory that b points to. Accessing b will probably segfault.

This seems like this would be a common problem in writing Python Extensions.

I suppose what I want is some way to base the reference counting on the Foo pointer. For example, a and b should actually have the same identity in the above example. Or perhaps what I need is some data structure that counts the number of PyFoos that share the same Foo pointer, and Foo_destroy is only called when the the count for the Foo pointer drops to 0.

What is the idiomatic way to solve this problem?

Here is the corresponding scenario in C:

Foo *parent = Foo_create();
Foo *a = Foo_create();
Foo_push(parent, 1, a);
Foo *b = Foo_pop(parent, 1);
/* a and b both point to same memory */
Foo_destroy(a);
/* better not access b after this */
a = NULL;
b = NULL;
HoldOffHunger
  • 18,769
  • 10
  • 104
  • 133
Matthew Moisen
  • 16,701
  • 27
  • 128
  • 231
  • How about if `parent_foo` was a `PyFoo`? That'd keep the parent alive as long as `a` was. I'm not sure it solves all your problems but it might help? – DavidW Jan 10 '20 at 19:55
  • @DavidW Sorry, that was a typo. The parent is a PyObject/PyFoo, and I have solved the problem you are referring to by having the PyFoo_Dealloc function only call Foo_Destroy if the current PyFoo has no parent; if the current PyFoo is a child, it decrefs the parent. This works fine. However I can't figure out how to solve the problem where two PyFoos, both of which are not children, are pointing to the same Foo *. – Matthew Moisen Jan 10 '20 at 20:15
  • `del a` should not trigger a call to `Foo_destroy` unless the reference count drops to 0. But the refcount only goes from 2 (`a` and `b`) to 1 (`b`) in this case. – chepner Jan 10 '20 at 21:04
  • The reference count to the `parent` object never changes, because nothing new ever refers to it after the initial assignment `parent = Foo()`. – chepner Jan 10 '20 at 21:04
  • 1
    @chepner I think the issue is that `a` and `b` are different Python objects, with the same internal `Foo*` (which is obviously bad) – DavidW Jan 10 '20 at 21:12
  • Yeah, it's not obvious from the code shown how that could come to be. – chepner Jan 10 '20 at 21:13

3 Answers3

1

I suspect you don't have the information to "automatically" use the same PyFoo object, and you may well end up duplicating most of your internal Foo structure within PyFoo if you were to try to store it.

One fairly simple option that occurs to me is to have an internal dict mapping Foo* to a PyFoo object. Therefore you only create a new PyFoo if needed, but otherwise re-use the existing object. Obviously Foo* isn't a Python object so can't be stored directly in a dict, but you can store it as an integer easily enough using PyLong_FromVoidPtr. Use a WeakValueDictionary to hold the PyFoos so you don't keep them alive just by virtue of being in the dictionary.

An outline of your wrapping of Foo_pop would look a bit like:

PyObject* PyFoo_pop(args...) {
    Foo* popped = Foo_pop(args...);
    PyObject* pf = PyObject_GetItem(internal_dictionary_of_pyfoos,
                                  PyLong_FromVoidPtr(popped));
    if (pf == NULL) {
       pf = create_a_new_PyFoo(popped);
    }
    return pf;
}

create_a_new_PyFoo obviously needs to add the PyFoo to the dictionary as it's created.

Obviously that's vague, untested, and missing all error-checking, but it seems like an easy way of doing what you want without having to track too many of the details of Foo's internals.

WeakValueDictionary: as you say, it's accessed through a Python interface. The code is essentially just a C version of what you'd do in Python. Roughly:

PyObject *weakref_mod = PyImport_ImportModule("weakref");
PyObject *weakvaluedict = PyObject_GetAttrString(weakref_mod, "WeakValueDictionary");
PyObject *wd_instance = PyObject_CallFunctionObjArgs(weakvaluedict, NULL);

(Untested, and ignoring error checking). Note that it isn't a direct subclass of dict I think, so use PyObject_GetItem not PyDict_GetItem (which behaves slightly different and returns something with an incremented reference)

PyFoo: Note that C API types need some small modifications to be weak referenceable. An example is in the docs but roughly they need a PyObject* to store the weakref list, and tp_weakreflistoffset set in the type object. That obviously adds a little overhead.

DavidW
  • 29,336
  • 6
  • 55
  • 86
  • I would need a `Py_INCREF(pf)` after calling PyDict_GetItem (for PyFoos that already exist), correct? – Matthew Moisen Jan 10 '20 at 21:02
  • Cool and inside of my `PyFoo_dealloc`, before calling `Foo_destroy`, I would need to remove it from this dict. – Matthew Moisen Jan 10 '20 at 21:08
  • You mentioned "I suspect you don't have the information to 'automatically' use the same PyFoo object, and you may well end up duplicating most of your internal Foo structure within PyFoo if you were to try to store it." - Can you think of a different case where one would have enough information to automatically use the same PyFoo object? – Matthew Moisen Jan 10 '20 at 21:09
  • 1
    @MatthewMoisen You shouldn't have to. By using a `WeakValueDictionary` it should remove itself when no other references exist, so `PyFoo_dealloc` doesn't have to worry about the dictionary – DavidW Jan 10 '20 at 21:11
  • @MatthewMoisen in some cases you might just be able to return the `parent` field of the `PyFoo`, for example. – DavidW Jan 10 '20 at 21:14
  • I can't seem to find WeakValueDictionary in the Python C API - isn't that only a Python Land data structure? If not, do you happen to have a link on how to instantiate a WeakValueDictionary in C land? – Matthew Moisen Jan 10 '20 at 22:36
1

Not sure about an "idiomatic way" but in cppyy (http://cppyy.org) I keep track of python objects (by type) to preserve identity and pybind11 (https://pybind11.readthedocs.io) does something similar, so it's a workable idea.

The only issue with C++, and thus not a concern for your case so just for completeness, is multiple (virtual) inheritance, where offsets between parent and derived classes are non-zero, so auto-casting is needed to make sure that when a pointer to a derived instance is returned as a pointer to base, the offset does not mess up the tracking.

To implement, keep a hash map of C pointer to Python object. When returning a Foo* to Python-land, check whether it already exists in the map, and re-use as needed. When the ref-count reaches 0, also remove the object from the map. Note that you do NOT need to increase the ref-count or keep weak references as the hash map never leaves C-land.

In addition, if you have control over Foo destruction in C-land, then I recommend a callback to set the Python proxy's Foo* to NULL and check for NULL in all access functions (cppyy does something like that, too, if the C++ provides callbacks).

EDIT: adding links to the code here otherwise I run out of characters.

First though: this is C++, so my life is a wee bit easier in that I can use STL containers w/o having to cast pointers to integers, but yes, if you were to do so, that's perfectly safe.

I collect references per type for performance reasons (keeps the maps smaller), see fCppObjects here: https://bitbucket.org/wlav/cpycppyy/raw/c6e7662bab1623e6cb15ddf59e94423a6081d66f/src/CPPScope.h

When a new proxy carrying a pointer to C++ is returned, said object is registered through the MemoryRegulator, and when an object goes away, it is unregistered: https://bitbucket.org/wlav/cpycppyy/raw/c6e7662bab1623e6cb15ddf59e94423a6081d66f/src/MemoryRegulator.h https://bitbucket.org/wlav/cpycppyy/raw/c6e7662bab1623e6cb15ddf59e94423a6081d66f/src/MemoryRegulator.cxx

The hooks are for frameworks to take over the behavior, e.g. one client code prefers to store all pointers in a single map.

The use of flags is for performance reasons of a few corner cases.

The lookup/registration happens in various places in the as objects can cross the boundary for various reasons (construction, function return, variable access). The function return is here: https://bitbucket.org/wlav/cpycppyy/raw/c6e7662bab1623e6cb15ddf59e94423a6081d66f/src/ProxyWrappers.cxx

look for the call in BindCppObjectNoCast.

The destruction happens when the object goes away, see the proxy class: https://bitbucket.org/wlav/cpycppyy/raw/c6e7662bab1623e6cb15ddf59e94423a6081d66f/src/CPPInstance.cxx and in particular op_dealloc_nofree(a helper to delete the C++ side, not (yet) Python), which is called from the normal tp_dealloc.

For pybind11, the functions are called register_instance and deregister_instance, which you can find here: https://raw.githubusercontent.com/pybind/pybind11/master/include/pybind11/detail/class.h

The registration happens into a single multimap called registered_instances that is found here: https://raw.githubusercontent.com/pybind/pybind11/master/include/pybind11/detail/internals.h

The lookup is in get_object_handle found here: https://raw.githubusercontent.com/pybind/pybind11/master/include/pybind11/cast.h and which does a matching of ptr and type.

Ie. pretty much the same thing as cppyy (just less efficient).

Wim Lavrijsen
  • 3,453
  • 1
  • 9
  • 21
  • Would you mind linking me to your code? When you say use a C pointer as the key in the hash map, do you mean an integer representation of the C pointer? I think if there is a collision that resulting in a comparison, in C it is undefined behavior to compare pointers that are not in the same array - not sure about C++. – Matthew Moisen Jan 10 '20 at 22:14
0

del a does not drop the reference count to 0 by itself; it only decrements the reference count, because it removes a single reference. b still refers to the object, so the reference count remains at 1, and there should be no call to Foo_destroy.

chepner
  • 497,756
  • 71
  • 530
  • 681
  • `a` and `b` reference two distinct python objects, with two different reference counts. That is the crux of my problem - how to get a and b to reference the same object such that they share the reference count. – Matthew Moisen Jan 10 '20 at 21:07
  • I don't think they should; why is `Foo_pop` creating a new instance of anything? Or rather, if there is a layer of indirection I'm not considering, `del a` should get rid of the wrapper object, but not call `Foo_destroy` on the object still referenced by the `b` object. – chepner Jan 10 '20 at 21:10
  • `Foo_pop` removes the Foo pointer from the parent, and returns the pointer to the calling code. The calling code is responsible for calling `free` on this pointer. In order to not have a memory leak, we need to create a PyFoo to wrap this Foo pointer and return it back to the user, who is free to discard it or use it. However by creating a new PyFoo object, we may now have two independent PyFoo objects, both of which refer to the same Foo pointer. – Matthew Moisen Jan 10 '20 at 21:13
  • I think you just need the wrapper around `Foo_push` to keep a pointer to `a` in addition to updating `parent->foo` to point to `a -> foo`. Then the `Foo_pop` wrapper just needs to clear the `parent->foo` pointer and return the pointer to the original `a`, so that both `a` and `b` now refer to the same `PyFoo` instance (which continues to hold the only pointer to the original `Foo` value held by `a`). This means that `PyFoo.__del__` becomes responsible for calling `Foo_destroy` once the reference count on the object drops to 0. – chepner Jan 10 '20 at 21:20
  • Do you mean, the wrapper around `Foo_push`, which takes the parent, key, and the to-be child `a`, should keep a pointer on the parent pointing to the child `a`, indexed by key - such that when `Foo_pop` is called on the parent with the given key, we can then grab the pointer indexed by the key which points to the child `a`, and return that to the caller? – Matthew Moisen Jan 10 '20 at 22:00
  • Alright I tried it out and it worked except for that both the child and the parent now have a reference to each other. Doing a `del a` `del b` and `del f` result in a memory leak since the reference count never drops to 0. Following Davidw's suggestion, I made the parent's dictionary of fields:foo contain weak references. If a child's deallocator is called, it deletes itself from the parent dictionary (the child also has a field called parent_field). This seems to do the trick except I need to walk through all the possible failure points as I haven't used weak references before. – Matthew Moisen Jan 11 '20 at 16:37
  • Would you have, in the parent dictionary, used weak references to the children like I'm doing or did I miss something easy? – Matthew Moisen Jan 11 '20 at 16:37
  • Possibly; I'm not well acquainted with the C level; I'm just pointing out how this should work at the Python level. – chepner Jan 11 '20 at 17:07