Title: WeakSet is not pickleable
Type: Stage:
Components: Library (Lib) Versions:
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: georg.brandl, pitrou, rhettinger, serhiy.storchaka, twouters
Priority: normal Keywords:

Created on 2017-06-17 18:54 by serhiy.storchaka, last changed 2017-06-18 06:09 by rhettinger.

Messages (4)
msg296243 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-06-17 18:54
WeakSet contains the __reduce__ method, but it isn't pickleable (and never was), because the pickle state contains the value of the __dict__ dict attribute which contains a reference to unpickleable local function _remove().

>>> pickle.dumps(weakref.WeakSet())
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: Can't pickle local object 'WeakSet.__init__.<locals>._remove'

I wondering whether WeakSet should be made pickleable or the __reduce__ method should be removed. __reduce__() can be used also for copying, but  there are no tests for this feature.
msg296256 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2017-06-18 03:42
> I wondering whether WeakSet should be made pickleable or 
> the __reduce__ method should be removed.

When considering whether to remove a method from long published code, if the method isn't broken, our guidance should come from whether user's have actually taken advantage of __reduce__.   Determining that answer involves searching published code (Google's code search used to be a good tool for this, now we've got Github's code search tools).  

In general, the burden is high for removing an existing feature (even if untested); otherwise, we risk breaking people's code for no good reason other than the joy that comes with churning code to solve an invented problem (one that has never arisen in real code and has never been reported by an actual user).

When considering whether to add pickle support, the bar is much lower.  Roughly the question is amounts to balancing the potential benefits (whether someone might need to pickle a weakset someday even though we have no evidence that anyone has ever wanted this) versus the costs (risk of introducing bugs, creating cross-version incompatabilities, increasing future maintenance costs, increasing the total code volume, etc).

If adding pickling capability is easy and clean, then it seems reasonable.   On the other hand, if it is even slightly tricky, we should skip adding a feature that no one has ever asked for.  The 
weak reference containers have long been a source of bugs, some of which were challenging to fix.
msg296262 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-06-18 05:58
The method is broken (and always was). Pickling doesn't work because the state contains unpickleable object. Copying works incorrectly, since the pickle state contains references to the original WeakSet and it overrides the __dict__ of the copy, making its state inconsistent. If the purpose of implementing the __reduce__ method was the support of the copying, the __reduce__ method should be fixed or the copying should be implemented with implementing the __copy__ and __deepcopy__ methods.

However there is a subtle moment with pickling WeakSet (as well as with pickling weakref.proxy, see issue18068). The problem is that if you pickled a WeakSet, but not hard references to its items, the items will be disappeared just after unpickling, since they have no hard references. This may confuse. If you pickle also hard refereces to the items, a WeakSet can be pickled and unpickled as expected (after fixing __reduce__).
msg296264 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2017-06-18 06:09
> If you pickle also hard refereces to the items,
>  a WeakSet can be pickled and unpickled as expected (after 
> fixing __reduce__).

That seems contrary to the original intent of a WeakSet.  I recommend not going down the path of adding pickle support.

If copying is also broken, then it is reasonable to either fix it or remove it depending on whether a fix is straight-forward.
Date User Action Args
2017-06-18 06:09:54rhettingersetmessages: + msg296264
2017-06-18 05:58:43serhiy.storchakasetmessages: + msg296262
2017-06-18 03:42:07rhettingersetmessages: + msg296256
2017-06-17 18:54:10serhiy.storchakacreate