collective / collective.elephantvocabulary

zope vocabulary with possibility to hide/show terms
http://pypi.python.org/pypi/collective.elephantvocabulary
GNU General Public License v2.0
1 stars 0 forks source link

Reference to persistent registry utility causes object access across different ZODB connections #2

Closed lukasgraf closed 10 years ago

lukasgraf commented 10 years ago

In the plone_registry property in vocabulary.py:35 a reference to the persistent utility of plone.app.registry is being held.

This causes a problem, because the vocabulary factory will be instanciated with different contexts, which have their own ZODB connections. So in certain circumstances this will lead to the vocabulary factory's context having a ZODB connection B, whereas the registry utility still has a different connection B. This then causes a ConnectionStateError when trying to access the registry (at vocabulary.py:53 for example):

  Module ZPublisher.Publish, line 126, in publish
  Module ZPublisher.mapply, line 77, in mapply
  Module ZPublisher.Publish, line 46, in call_object
  Module plone.z3cform.layout, line 70, in __call__
  Module plone.z3cform.layout, line 54, in update
  Module plone.dexterity.browser.add, line 112, in update
  Module plone.z3cform.fieldsets.extensible, line 59, in update
  Module plone.z3cform.patch, line 30, in GroupForm_update
  Module z3c.form.group, line 134, in update
  Module z3c.form.group, line 47, in update
  Module z3c.form.group, line 43, in updateWidgets
  Module z3c.form.field, line 275, in update
  Module z3c.form.browser.select, line 50, in update
  Module z3c.form.browser.widget, line 70, in update
  Module z3c.form.widget, line 199, in update
  Module z3c.form.widget, line 193, in updateTerms
  Module zope.component._api, line 107, in getMultiAdapter
  Module zope.component._api, line 120, in queryMultiAdapter
  Module zope.component.registry, line 238, in queryMultiAdapter
  Module zope.interface.adapter, line 532, in queryMultiAdapter
  Module z3c.form.term, line 96, in ChoiceTerms
  Module zope.schema._field, line 349, in bind
  Module collective.elephantvocabulary.vocabulary, line 53, in __call__
  Module plone.registry.registry, line 43, in get
  Module ZODB.Connection, line 857, in setstate
ConnectionStateError: Shouldn't load state for 0x0dce when the connection is closed

Proposed solution: Don't memoize the plone.app.registry utility, but instead query for it every time we need to access the registry.

garbas commented 10 years ago

thx for catching this.

@memoize should actually be on method which is returning the record from registry. then it wouldn't break on different zodb connections.

lukasgraf commented 10 years ago

@garbas you mean something like this?

from plone.memoize import instance

    @instance.memoize
    def record_from_registry(self, key):
        registry = getUtility(IRegistry)
        record = registry.get(key, None)
        return record

But wouldn't that mean that it's not possible to edit terms in the registry TTW anymore? Since memoize() uses the method arguments as the cache key, so even if the user changes a registry record, the key stays the same, and updates to the vocabulary will only be effective upon next restart of the zope instance. This would kind of defeat the purpose of making the terms configurable through the registry, no?

Unless maybe we use registry events to implement cache invalidation.

lukasgraf commented 10 years ago

I looked into implementing memoizing registry records and doing proper cache invalidation, but it's definitely non-trivial and would require some substantial changes.

For the moment I would prefer if we could just fix the problem at hand, and consider caching the registry records at a later point in time (they haven't been cached up until now, after all - just a reference to the utility was kept).

I would assume that fetching the actual records is much more of an expensive operation than the getUtility call to get the utility from the component registry. So by simply not keeping a reference to the utility performance shouldn't degrade substantially, and the problem regarding ZODB connections would be fixed.

lukasgraf commented 10 years ago

Added a proof-of-concept implementation for caching registry records and cache invalidation in #7

phgross commented 10 years ago

Changes are looking good. :+1:

I think the caching should be processed in an separate issue, therefore i will merge that one.