`bind_extension_point` prevents object garbage collection

[pankajp]

bind_extension_point stores a reference to the binding and prevents collection of the object, and it does not offer any way to unbind either.

ExtensionPointBinding objects are stored in a WeakKeyDictionary mapping object weakref to the ExtensionPointBinding instance, but the binding itself stores a strong reference to the object and hence prevents its garbage collection.

[mdickinson]

Related: #79

[pankajp]

Great, thanks. Should this be closed as duplicate of the other issue? I have a simple fix using Weakref trait for obj and it seems to fix a garbage collection issue. I could submit a PR if you think this approach would be acceptable.

[mdickinson]

I don't think it's quite a duplicate; it seems reasonable to me to leave both issues open. (But also reasonable to create a PR that solves both issues simultaneously.)

[mdickinson]

So if I understand correctly, this is a bug, in the sense that it's in clear opposition to the design intent:

    # We keep a reference to each binding alive until its associated object
    # is garbage collected.
    _bindings = weakref.WeakKeyDictionary()

(from https://github.com/enthought/envisage/blob/38c1ffc40cdd0b9a2e19faed80c8f253a08c553c/envisage/extension_point_binding.py#L19-L21)

That is, the WeakKeyDictionary is pointless here, since the ref-counting machinery will never remove items from it. We could replace it with a regular Python dict and no behaviour would change. Does that match your understanding of the current code?

If so, I'm actually quite happy about this, since WeakKeyDictionary objects can be a source of weird and non-deterministic behaviour. :-)

Rather than fixing the weak-ref mechanics, I'd be much happier to have an extension unbinding operation available.

[pankajp]

That is, the WeakKeyDictionary is pointless here, since the ref-counting machinery will never remove items from it. We could replace it with a regular Python dict and no behaviour would change. Does that match your understanding of the current code?

Yes indeed. In the current code (w/o this PR), the WeakKeyDictionary used for _bindings is useless because the value (ExtensionPointBinding) itself holds a reference to the key preventing its collection. That is what I tried to fix by changing the obj reference to be a weakref, so that it works as designed.

Even with adding an unbind operation, I would like to fix this issue as well.

[mdickinson]

I would like to fix this issue as well.

Right, but I think there's a case for "fixing" it by simply replacing the WeakKeyDictionary with a regular dictionary, and requiring that clients explicitly unbind when necessary.

[pankajp]

Right, but I think there's a case for "fixing" it by simply replacing the WeakKeyDictionary with a regular dictionary, and requiring that clients explicitly unbind when necessary.

Yes, and that would be perfectly fine since it would match the current behavior as well. I just thought not having to do any changes in exiting projects would be good, and would also behave much like existing on_trait_change and friends about not holding references.

As of now, the need for this has been worked around. I'll get to adding unbind functionality later. Would adding a remove argument to the bind_extension_point be the canonical way to implement it, as is done for on_trait_change and sync_trait?

[mdickinson]

I just thought not having to do any changes in exiting projects would be good

Yes, there is that. But on the flip side, I'm a bit worried about breaking existing code that was inadvertently relying on the non-collection of the bindings. Maybe that's not a serious possibility.

would also behave much like existing on_trait_change and friends about not holding references.

Agreed that it would be good to have that consistency.

[mdickinson]

This has just bitten me yet again. Let's see if we can fix for the upcoming Envisage release.

[mdickinson]

Some thoughts about possible steps towards a fix:

• In the bind_extension_point function, warn if the extension_registry argument is not supplied. The goal is to eventually make this a required argument, so that we can get rid of the sneaky global introduced in the _extension_registry_default.
• Move the addition of the extension point to the global dict out of ExtensionPointBinding.__init__ and into the bind_extension_point function. This will make it possible to create an ExtensionPointBinding without modifying global state in the process. We can update the bind_extension_point docstring to clarify the global state change.
• Add an unbind_extension_point function that takes an ExtensionPointBinding instance and both undoes the connection between the extension point and the trait, and removes the extension point from the global dict.
• Consider moving the _bindings dictionary out of the ExtensionPointBinding class. (This may have to be done later, since we currently have downstream code that relies on ExtensionPointBinding._bindings existing, precisely so that it can work around this issue.)
• Consider making the _bindings dictionary a real dictionary (ditto)
• Consider moving the _bindings dictionary to the extension registry (ditto)

An org-wide code search turned up no direct uses of ExtensionPointBinding beyond those being used specifically to work around this bug, and no uses of bind_extension_point where the registry was not given.

But: there's an ExtensionPoint.bind method that doesn't allow passing a registry. Looks like we'll need to deprecate that, too. It's probably not used much (it's generally inconvenient to use a method on a TraitType).

[mdickinson]

There are some horrible things going on here. At first sight, it looks as though the reference to ExtensionPoint.extension_registry in ExtensionPointBinding._extension_registry_default can't possibly work (and indeed, when I tried to write tests for deprecating the ExtensionPoint.bind method, I got an AttributeError: type object 'ExtensionPoint' has no attribute 'extension_registry'). It turns out that the way that this is expected to work is that the client code is supposed to inject that extension_registry value onto the ExtensionPoint trait type (the class, not any instance), and the tests for extension_point_binding do exactly that (and then never undo that global state change).

I believe (and sincerely hope) that we have no client code that's making that injection, in which case the three-argument form of bind_extension_point simply doesn't work in practice, and I don't think we need to worry about going through a deprecation period. I think we can remove it.

[mdickinson]

It's probably not used much

A code search turned up no evidence that ExtensionPoint.bind is used anywhere.