Create alternative CObject API that is safe and clean

[larryhastings]

BPO	5630
Nosy	@jcea, @larryhastings, @devdanzin, @benjaminp
Files	cobject.diff: Patch against py3k/trunk r70718. lch.capsule.r71304.diff: Patch against py3k/trunk r71304. lch.capsule.r71641.diff: Patch against py3k/trunk svn r71641. lch.capsule.r71641.diff.2: Patch against py3k/trunk svn r71641, revision 2. lch.capsule.r72125.diff: Patch against py3k/trunk r72125. lch.capsule.r72270.diff: Patch against py3k/trunk r72270. lch.capsule.r72309.diff: Patch against py3k/trunk r72309.

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2009-05-05.22:32:26.799>
created_at = <Date 2009-03-31.19:07:48.893>
labels = ['interpreter-core', 'type-feature']
title = 'Create alternative CObject API that is safe and clean'
updated_at = <Date 2010-04-25.03:15:29.386>
user = 'https://github.com/larryhastings'

bugs.python.org fields:

activity = <Date 2010-04-25.03:15:29.386>
actor = 'jcea'
assignee = 'none'
closed = True
closed_date = <Date 2009-05-05.22:32:26.799>
closer = 'benjamin.peterson'
components = ['Interpreter Core']
creation = <Date 2009-03-31.19:07:48.893>
creator = 'larry'
dependencies = []
files = ['13521', '13626', '13703', '13706', '13820', '13864', '13892']
hgrepos = []
issue_num = 5630
keywords = ['patch']
message_count = 14.0
messages = ['84864', '84927', '85074', '85075', '85259', '85619', '86023', '86028', '86834', '87106', '87135', '87213', '87220', '87296']
nosy_count = 5.0
nosy_names = ['jcea', 'dalcinl', 'larry', 'ajaksu2', 'benjamin.peterson']
pr_nums = []
priority = 'normal'
resolution = 'accepted'
stage = None
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue5630'
versions = ['Python 3.1']

[larryhastings]

The CObject API has two flaws.

First, there is no usable type safety mechanism. You can store a void
*object, and a void *description. There is no established schema for
the description; it could be an integer cast to a pointer, or it could
point to memory of any configuration, or it could be NULL. Thus users
of the CObject API generally ignore it--thus working without any type
safety whatsoever. A programmer could crash the interpreter from pure
Python by mixing and matching CObjects from different modules (e.g. give
"curses" a CObject from "_ctypes").

Second, the destructor callback is defined as taking *either* one *or*
two parameters, depending on whether the "descr" pointer is non-NULL.
One can debate the finer points of what is and isn't defined behavior in
C, but at its heart this is a sloppy API.

MvL and I discussed this last night and decided to float a revision of
the API. I wrote the patch, though, so don't blame Martin if you don't
like my specific approach.

The color of this particular bike shed is:

• The PyCObject is now a private data structure; you must use accessors.
  I added accessors for all the members.
• The constructors and the main accessor (PyCObject_AsVoidPtr) now all
  *require* a "const char *type" parameter, which must be a non-NULL C
  string of non-zero length. If you call that accessor and the "type" is
  invalid *or doesn't match,* it fails.
• The destructor now takes the PyObject *, not the PyCObject *. You
  must use accessors to get your hands on the data inside to free it.

Yes, you can easily skip around the "matching type" restriction by
calling PyCObject_AsVoidPtr(cobj, PyCObject_GetType(cobj)). The point
of my API changes is to *encourage* correct use.

The attached patch was written py3k/trunk r70718. It compiles with no
new warnings/errors and doesn't seem to cause any new failures in the
regression test.

Note: this patch is not complete; I fixed all the .c and .h files, but I
have yet to update the documentation. I figure I don't want to put the
effort in until the dust settles.

[dalcinl]

Two questions:

1. Why do you prefer to pass destructor as a first argument?

2. Do we really need two different calls for providing a context
   pointer? Why no just one call and pass a NULL context?

A comment:

Suppose one wants to implement export/import module C-API's in a
function-by-function basis. This is nice, as you can extend your module
C-API, and make it be backward "ABI" compatible.

As the void* <-> void(*)(void) conversion is illegal(?) in C(99?), one
has to go to the oddities of doing some sort of type-punning with
unions... this could be somewhat tedious for hand-written extension
modules.

Do you have any idea on how extend the CObject API for the commented use
case?

[larryhastings]

dalcinl:

I made the destructor the first argument because APIs where arguments
move around irritate me. In the existing CObject API, the destructor is
"always" the last argument--which means in practice it is either the
second or third argument, depending on which one you call. I also think
that the destructor is a different kind of argument from the rest; the
others are data attributes for the object, and the destructor is a
callback. I wanted to group the data attributes together.

We don't really need two different calls for providing a context
pointer, and if folks thought the shorter call should go we can get rid
of it. But 90% of the time all you'll need . Also, the new API grew
out of the existing API, which had a similar two calls
(PyCObject_FromVoidPtr, and PyCObject_FromVoidPtrAndDesc).

As for your "comment", I see CObject as a lightweight way of making a
generic "handle" type to represent C data structures without going to
the trouble of a real PyTypeObject. I think the "storing functions in a
vtable then hiding it in the Python symbol table for other modules" use
case is a misuse of the API. While I'm not going to actively prevent
it, I certainly wouldn't do anything to support it.

[larryhastings]

Sorry, in editing I forgot to finish my sentence. In the middle of the
second paragraph, the sentence should read:

But 90% of the time all you'll need is PyCObject_FromVoidPtr().

My other editing mistakes will just have to stand.

[larryhastings]

Having slept on it, I agree that we only need one API to create an
object. Since passing in a "context" will be relatively rare, users who
need that can use PyCObject_SetContext(). I'll remove
PyCObject_FromVoidPtrWithContext() in my next patch.

[larryhastings]

I discussed this off-list with GvR. He was primarily concerned with
fixing the passing-around-a-vtable C API usage of CObject, but he wanted
to preserve as much backwards compatibility as possible. In the end, he
suggested I create a new API and leave CObject unchanged. I've done
that, incorporating many of GvR's suggestions, though the blame for the
proposed new API is ultimately mine.

The new object is called a "Capsule". (I *had* wanted to call it
"Wrapper", but there's already a PyWrapper_New in descrobject.h.)
Highlights of the new API:

• PyCapsule_New() replaces PyCObject_FromVoidPtr.
  • It takes a void * pointer, a const char *name, and a destructor.
  • The pointer must not be NULL.
  • The name may be NULL; if it is not NULL, it must be a valid C string
    which outlives the capsule.
  • The destructor takes a PyObject *, not a void *.
• PyCapsule_GetPointer() replaces PyCObject_AsVoidPtr.
  • It takes a PyObject * and a const char *name.
  • The name must compare to the name inside the object; either they're
    both NULL or they strcmp to be the same.
• PyCapsule_Import() replaces PyCObject_Import.
  • It takes three arguments: const char *module_name, const char
    *attribute_name, int no_block.
  • It ensures that the "name" of the Capsule is "modulename.attributename".
  • If no_block is true, it uses PyModule_ImportModuleNoBlock.
• The PyCapsule structure is private. There are accessors for all
  fields: pointer, name, destructor, and "context"
  • The "context" is a second "void *" you can set / get.

I've attached a patch to implement this; it was written against svn
r71304. The patch isn't ready to be applied--there is no documentation
for the new API beyond the header file.

GvR and I disagree on one point: he thinks that we should leave CObject
in forever, undeprecated. I think we should deprecate it now and remove
it... whenever we'd do that. The new API does everything the old one
does, and more, and it's cleaner and safer.

[larryhastings]

I've updated the patch:

• Capsule now has a custom repr that includes whatever name is set.
• I've updated the documentation to talk about Capsules and not
  CObjects; the documentation discusses naming your capsules.
• PyCapsule_Import now takes a "no_block" parameter; if true,
  PyCapsule_Import will import the module using
  PyImport_ImportModuleNoBlock, and if the import fails it will return
  failure but *not* raise an exception.
• I checked all the exceptions to ensure they were of reasonable types.

This is as far as I can take the patch without some input from the
community. I hope the fate of this patch can be decided before the 3.1
feature freeze.

[larryhastings]

Whoopsie-daisy, I forgot to touch up a whole lot of places to match the
new API. Attached is an updated patch; here's what changed:

• The documentation is now much better; there is a capsule.rst, and
  cobject.rst has a deprecation warning.
• Added a deprecation warning in a comment to Include/cobject.h.
• Added the PyCapsule APIs to refcounts.dat and the .def files for OS/2
  builds.
• Changed a commented mention of "CObject" to "Capsule" in
  Lib/test/test_sys.py.

[larryhastings]

Added a test case for capsules to _testcapimodule.c, and updated to the
latest trunk.

[larryhastings]

Updated patch based on impressively thorough Rietveld feedback from
Benjamin Peterson. Thanks, Benjamin!

This patch should not be considered final; we already know the API
documentation in the comments in Include/pycapsule.h needs to change
(somehow).

[dalcinl]

In Doc/c-api/capsule.rst, you wrote

.. cfunction:: int PyCapsule_Import(const char* name, int no_block)

but it should be:

.. cfunction:: void* PyCapsule_Import(const char* name, int no_block)

Additionally, you wrote "disambugate" in many places.

[larryhastings]

dalcinl: Thanks, I've fixed the doc wrt writing "int" where I meant
"void *".

As for using "disambiguate" in many places: there are a couple of
sentences that are repeated in the documentation for several functions.
Those repeated sentences all use "ambiguous" and "disambiguate".
Having them read almost exactly the same is a subtle cue to the reader,
suggesting they will behave similarly, which is correct. Is that a problem?

[larryhastings]

A nice fresh patch, against r72309. Incorporates changes based on
Benjamin's latest batch of Rietveld comments. They're thinning out, so
we must be near the end--and with a day to spare. Also strips out
almost-all documentation from "pycapsule.h".

Note: this still has CObject flagged as deprecated, both in doc and with
a runtime warning. Dare I hope these will survive into the accepted patch?

[benjaminp]

Applied in r72363.