sde1000 / python-xkbcommon

Python bindings for libxkbcommon using cffi
MIT License
19 stars 5 forks source link

Fix Context.keymap_new_from_names() #6

Closed sde1000 closed 3 years ago

sde1000 commented 3 years ago

It appears this had never been tested other than with null arguments!

@flacjacket — please let me know whether this looks sane.

@hickford — does this fix your problem?

sde1000 commented 3 years ago

Hmm. I can't find anything in the python execution model that guarantees that keep_alive would never be garbage collected until the end of the function (although I'm sure that in the current implementations it wouldn't be). A python interpreter that performed liveness analysis, for example, might garbage collect the object referred to by keep_alive immediately before the call to lib.xkb_keymap_new_from_names() since there are no further references to it in the function.

This would never normally matter, but cffi is exposing more of python's implementation details than one regularly experiences! I prefer the WeakKeyDictionary() approach because it makes it clear that names depends on the ffi pointer objects.

flacjacket commented 3 years ago

Couldn't you say the same thing about the wkd variable that it could possibly be garbage collected if the interpreter were to reason about it not being referenced later in the function? I would think that would also remove the objects that need to be kept alive. That said, I do see your point about the weak key dictionary encoding the semantics of name keeping the objects alive, but a comment may be helpful in in explaining what is going on even if it just links to the cffi documentation.

Reading this, the keep_alive object and its contained objects should be kept alive as long as the variable is reachable, which I would take to be to the end of the function scope, rather than as long as the variable is used, but I don't see a definition there for "reachable". But, it looks like from the note on the CPython implementation detail, that the CPython interpreter is as aggressive as the data model allows in removing unreachable variables, at least in the absence of cyclic references.

sde1000 commented 3 years ago

Good point!

How about using the keep_alive list, and then explicitly unbinding both it and names after the call to lib.xkb_keymap_new_from_names() with a suitable comment about cffi memory management? At this point I think it's just a matter of style and making sure nobody (including me!) breaks it in the future.