[ZODB-Dev] problems w/ Pickle based deep-copy of Blob objects

[Rob Miller]

i'm assuming from the lack of response that nobody has a problem w/ this 
change... if i don't hear differently within the next 24 hours or so, i'll 
commit the change to the 3.8 branch and the trunk.

here's the patch:

Index: src/ZODB/blob.py
===================================================================
--- src/ZODB/blob.py	(revision 91033)
+++ src/ZODB/blob.py	(working copy)
@@ -93,7 +93,7 @@
          # XXX should we warn of this? Maybe?
          if self._p_changed is None:
              return
-        for ref in self.readers+self.writers:
+        for ref in (self.readers or [])+(self.writers or []):
              f = ref()
              if f is not None:
                  f.close()

-r


Rob Miller wrote:
> hi all,
> 
> i've been experimenting w/ getting ZODB 3.8's blob storage support to work w/ 
> the openplans.org stack, and have hit a snag about which i could use some 
> feedback.
> 
> the problem is that CMFEditions, which we use to version our content, uses 
> Pickler.dump() and Unpickler.load() to create deep copies of objects, which 
> then get stored as historical versions of those objects.  the heart of the 
> issue is probably best illustrated by an interactive session:
> 
>  >>> blob
> <ZODB.blob.Blob object at 0xa0c1230>
>  >>> blob.readers
> []
>  >>> blob.writers
> []
>  >>> stream = StringIO()
>  >>> p = Pickler(stream, 1)
>  >>> p.dump(blob)
> <cPickle.Pickler object at 0x9fb9cc8>
>  >>> u = Unpickler(stream)
>  >>> stream.seek(0)
>  >>> copy = u.load()
>  >>> copy
> <ZODB.blob.Blob object at 0xa0c12a8>
>  >>> copy.readers
>  >>> copy.writers
> 
> 'readers' and 'writers' are defined as None on the Blob class, but they get 
> initialized as empty list instance variables by the __init__ and __setstate__ 
> methods.  neither of these are triggered by the deep copy process, however, 
> and apparently the __getstate__ and __setstate__ implementations prevent these 
> instance variables from propagating to the pickled copy.
> 
> this seems okay, at first, but if anything is done w/ the transaction that 
> triggers the copy's _p_invalidate method to be called, it blows up with a 
> TypeError on the following:
> 
>      for ref in self.readers+self.writers:
> 
> the problem seems to go away w/ a simple change:
> 
>      for ref in (self.readers or []) + (self.writers or []):
> 
> this seems safe to me; it's not actually changing the state of any objects, 
> and it's only going to have an impact when an object w/ no readers and writers 
> is trying to be invalidated.  but i don't know enough about what's going on 
> here to feel confident that this is the right solution, that i'm not 
> introducing some subtle ZODB bug that's going to bite me (or someone else) 
> down the road.
> 
> can anyone tell me whether or not this seems safe?  if this isn't the right 
> approach, what would you suggest?  (my second guess would be to fix the 
> __getstate__ and __setstate__ implementation so that these instance variables 
> make it through the pickling process in the first place...)
> 
> any help very appreciated, thanks!
> 
> -r