divmod-dev team mailing list archive

[Jean-Paul Calderone <exarkun@xxxxxxxxxxxxxxxxx>]

So the cause of the problem seems to be this PyPy behavior, which differs from behavior we can observer on CPython: the object a weakref refers to may be collected and then, before the weakref's callback is called, some more Python code gets a chance to run.  This means that the callback which cleans up the `data` dict might not run until after some code uses the `get` method.  This confuses `get` because it thinks only an internal bookkeeping error can result in a key being present in `data` but the value of the weakref being `None`.  On PyPy this is actually a legitimate case, since it's only a matter of the callback not having run /yet/.

This hypothesis is testable, actually.  Adding any observable side-effect to the callback will prove that the callback is not being called in time - or that it is.

And some experimentation seems to bear this out.  A print statement at the top of `remove` doesn't show up until *after* the first Axiom test which fails this way - that is, the weakref callback has not been called until after the test has failed.

So I am probably convinced this is the right direction to be going now.  That said:

  1) The `assert` in the `cache` method seems like it may run afoul of this very issue.  The key may be present in `data` after the associated object has been collected.  It seems this check needs to include inspection of the value of `self.data[k]()`.  It is not an error to re-use a cache key if a weakref remains in `data` for the key, but the value of the weakref is `None`.
  2) The code could probably use a few more comments.  This stuff is not obvious. ;)
  3) I think I convinced myself that the new try/except KeyError inside `remove` is also necessary - in case the weakref callback runs after the weakref has been discarded by the new `del` in `get` but PyPy runs its callback anyway (because the garbage collector hasn't collected the weakref object yet).  Again, a comment or two explaining this would help the code a bunch, I think.
  4) If you don't mind, `w`, `k`, and `f` are not ideal argument names for `createCacheRemoveCallback` (not to mention undocumented).

Changes elsewhere in Axiom seem fine.  Thanks!  Feel free to merge after addressing the above.  I'm also happy to take another look if you like.

-- 
https://code.launchpad.net/~divmod-dev/divmod.org/829879-pypy-finalization-semantics/+merge/173053
Your team Divmod-dev is subscribed to branch lp:divmod.org.