Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

frozen=True incompatible with cache_hash=True as of 19.1.0 #611

Closed
pganssle opened this issue Jan 7, 2020 · 0 comments · Fixed by #612
Closed

frozen=True incompatible with cache_hash=True as of 19.1.0 #611

pganssle opened this issue Jan 7, 2020 · 0 comments · Fixed by #612

Comments

@pganssle
Copy link
Member

pganssle commented Jan 7, 2020

Prior to #489, this used to work:

import attr
import copy

@attr.s(frozen=True, cache_hash=True)
class FrozenWithCache:
    pass


if __name__ == "__main__":
    copy.deepcopy(FrozenWithCache())

The issue is that frozen is implemented by throwing an error in __setattr__, and clearing the hash cache calls setattr. I think this can be solved by using the same mechanism that _make_hash uses for frozen classes, but I have not tried implementing this before.

Another option is to not clear the hash cache on serialization for frozen classes.

pganssle added a commit to pganssle/attrs that referenced this issue Jan 7, 2020
pganssle added a commit to pganssle/attrs that referenced this issue Jan 8, 2020
pganssle added a commit to pganssle/attrs that referenced this issue Jan 8, 2020
This is a failing test for GH issue python-attrs#611, with the xfail decorator to be
removed when the issue is fixed.
pganssle added a commit to pganssle/attrs that referenced this issue Jan 8, 2020
Fixes GH issue python-attrs#611: serialization of a class with frozen=True and
cache_hash=True will now succeed rather than raising a
FrozenInstanceError.
pganssle added a commit to pganssle/attrs that referenced this issue Jan 8, 2020
Fixes GH issue python-attrs#611: serialization of a class with frozen=True and
cache_hash=True will now succeed rather than raising a
FrozenInstanceError.
hynek added a commit that referenced this issue Jan 13, 2020
* Add xfailing test for frozen/cache_hash issue

This is a failing test for GH issue #611, with the xfail decorator to be
removed when the issue is fixed.

* Fix incompatibility between frozen and cache_hash

Fixes GH issue #611: serialization of a class with frozen=True and
cache_hash=True will now succeed rather than raising a
FrozenInstanceError.

Co-authored-by: Hynek Schlawack <[email protected]>
pganssle added a commit to pganssle/attrs that referenced this issue Feb 10, 2020
This change was overshadowed by a more fundamental change in python-attrs#620.
hynek pushed a commit that referenced this issue Feb 10, 2020
* Use an self-clearing subclass to store hash cache

Rather than attempting to remove the hash cache from the object state on
deserialization or serialization, instead we store the hash cache in an
object that reduces to None, thus clearing itself when pickled or
copied.

This fixes GH #494 and #613.

Co-authored-by: Matt Wozniski <[email protected]>

* Add test for two-argument __reduce__

I couldn't think of any way to make a useful and meaningful class that
has no state and also has no custom __reduce__ method, so I went
minimalist with it.

* Improve test for hash clearing behavior.

Previously, there was some miniscule risk of hash collision, and also it
was relying on the implementation details of `pickle` (the assumption
that `hash()` is never called as part of `pickle.loads`).

* Add improved testing around cache_hash

* Update src/attr/_make.py

Co-Authored-By: Ryan Gabbard <[email protected]>

* Update comment in slots_setstate

Since the cached hash value is not actually serialized in __getstate__,
__setstate__ is not actually "clearing" it on deserialization - it's
initializing the value to None.

* Add changelog entry

* Remove changelog for #611

This change was overshadowed by a more fundamental change in #620.

Co-authored-by: Matt Wozniski <[email protected]>
Co-authored-by: Ryan Gabbard <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants