Skip to content

Allow pickling Cancelled #3250

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

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Conversation

A5rocks
Copy link
Contributor

@A5rocks A5rocks commented Apr 12, 2025

Fixes #3248

Copy link

codecov bot commented Apr 12, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00000%. Comparing base (737d96a) to head (9dc7386).
Report is 36 commits behind head on main.

Additional details and impacted files
@@               Coverage Diff                @@
##                 main        #3250    +/-   ##
================================================
  Coverage   100.00000%   100.00000%            
================================================
  Files             124          124            
  Lines           18848        19340   +492     
  Branches         1277         1318    +41     
================================================
+ Hits            18848        19340   +492     
Files with missing lines Coverage Δ
src/trio/_core/_exceptions.py 100.00000% <100.00000%> (ø)
src/trio/_core/_tests/test_run.py 100.00000% <100.00000%> (ø)

... and 5 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jakkdl
Copy link
Member

jakkdl commented Apr 13, 2025

we could instead add __reduce__ to the NoPublicConstructor metaclass to enable pickling for all objects that make use of it

@A5rocks
Copy link
Contributor Author

A5rocks commented Apr 13, 2025

we could instead add __reduce__ to the NoPublicConstructor metaclass to enable pickling for all objects that make use of it

I was thinking that too (we'd need to add it in NoPublicConstructor's __init__ or make a class we inherit from that uses NoPublicConstructor, but totally possible) but I'm not sure we want pickling for everything. I guess I can't think of any reasons against though.

@A5rocks
Copy link
Contributor Author

A5rocks commented Apr 13, 2025

Alright generally applying __reduce__ is simple and TBH it's probably fine. People could work around not having a public constructor anyways.

The main point is the number of type: ignores it uses but I assume that could be minimized if I actually cared to.

def __new__(cls, name: str, bases: tuple[type, ...], namespace: dict[str, Any], **kwargs: object) -> type: # type: ignore[explicit-any]
old_reduce = namespace.get("__reduce__")

def __reduce__(self: object) -> Any: # type: ignore[explicit-any]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, wouldn't self be an instance of NoPublicConstructor? If so, that would probably alleviate the attr-defined error on line 335

Copy link
Contributor Author

@A5rocks A5rocks Apr 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, Cancelled itself is an instance of NoPublicConstructor, (well not exactly cause we hand that off to type in __new__ but that's the idea) not instances of Cancelled. Metaclasses are weird!

Copy link
Member

@jakkdl jakkdl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a couple small things, otherwise looks as good as it could given the messiness of metaclasses :)

A5rocks and others added 2 commits April 17, 2025 18:50
@A5rocks A5rocks requested a review from jakkdl April 18, 2025 10:11
Copy link
Member

@jakkdl jakkdl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay I looked a bit more thorough now and found some small things :)

@A5rocks
Copy link
Contributor Author

A5rocks commented Apr 19, 2025

Turns out I underestimated pickling. It's more complicated than I expected and I don't think there's a way to generalize support to any class. I was operating in a world where Exception.__reduce__ was the default but object.__reduce__ is really weird (try it yourself!). In fact, I would need to assume what __set_state__ is doing and have special casing for if it's bypassed using a dict, and compare the first element to copyreg._reconstructor to know whether to replace the second element, and whatever. It's not worth the complexity, so I'm going to undo generalizing this. Nobody should be pickleing these anyways!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

trio.Cancelled can't be pickled
3 participants