nexedi / erp5

erp5ish actions tool doesn't support downgrade. If erp5 instance is started with this patch once, it will not start without this patch.

First, I must say that I am not authoritative either on action providers nor on erp5-ification of tools.

For the code, I have two worries:

• _setId which changes given ID: maybe override _checkId so it fails if prefix is absent and make callers pass the prefix ? I do not know much about possible callers.
• Usage of a volative as a marker. Volatiles can disappear in a surprisingly high number of places (between consecutive python statements, and likely even inside them if they somehow trigger ZODB code). The pattern used is safe as far reading the volatile (unlike many places in Zope code, perventing the easy test of setting object cache size to 0 by causing unrelated failures), but the overall usage pattern seems dangerous. Would the not ob_id.startswith(prefix_) condition be enough on its own ?

I think there is no problem with being downgrade-incompatible.

[EDIT] s/erp5-ification or tools/erp5-ification of tools/

Thank you for you find time to review.

In fact i can't imagine the examples, where prefix is absent.

I think _checkId is method of an objects container. Will not it be a bad practice to use it?

As i see the callers you talked about are used in 2 places:

!84 (diffs) and !84 (diffs)

---

About volatile. It's used only for break an infinite loop. As the transparent migration is needed then the migration code is run while getId is called. Infinite loop takes place because getId is called somewhere during of setId work. I can exclude using of volatile by changing the place where migration code is run, but i did't find this place. Do you have an idea?

I'm not sure i have understood your remark correctly. I agree that volatile lives only in one instance, so if there is a cluster, the migration could run several times. Can we consider this fact as negligible? If the volatile is missed inside one instance, then the loop will be breaked in the next iteration.

I think _checkId is method of an objects container.

Woops, I forgot this is indeed checked by container, not by contained. Then, next proposal would be: make _setId itself fail if prefix is absent, rather than adding it. As you say there should be no case where prefix is missing, it would feel safer (then no need to make CopySupport read id after setting it).

I'm not sure i have understood your remark correctly. I agree that volatile lives only in one instance, so if there is a cluster, the migration could run several times. Can we consider this fact as negligible? If the volatile is missed inside one instance, then the loop will be breaked in the next iteration.

Reformulating as code:

  self._v_foo = 'bar'
  try:
    self._v_foo
  except AttributeError:
    print 'This statement may sometime be executed when ZODB is under memory pressure'

(and the more there are statements beween __setattr__ and __getattr__, the likelier it is to happen as they may cause ZODB to reduce object cache to free memory)

So while it is indeed true that the volatile lives on a single process, and even on a single ZODB.Connection instance (which is bound to a thread while a transaction is active, which is the only case we care about here) in that process, it may disappear within the transaction which added it, as soon as on the next statement depending on ZODB cache magic happening.

This said, I just thought of a way to ensure that a volatile survives some time: modify its parent (here, the instance whose class inherits from GenerateIdFromReferenceMixin). ZODB cache cannot discard modified objects as it would mean loosing that change. As a result, the volatile attribute will survive on that connection until transaction (or subtransaction, but AFAIK we should not use them anyway) ends (be it commit or abort). Ideally, this change should happen before the volatile is set so that ZODB cannot trap us. It is possible to do a no-op change by self._p_modified = True. It costs disk space on commit as the object get stored to ZODB, but if it gets actually modified anyway then that's free (object will be stored only once anyway).

Going away from volatiles a moment, I wonder about the alternative of using a module-level set: add instance in that set when you would set the volatile, and remove it on the way up (try...finally) if (and only if) it was added on the way down. I do not like module globals, but this seems safe enough.