Skip to content

Add all GitHubSession attributes to __attrs__. #946 #947

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

Merged
merged 1 commit into from
Jun 5, 2019
Merged

Add all GitHubSession attributes to __attrs__. #946 #947

merged 1 commit into from
Jun 5, 2019

Conversation

nedbat
Copy link
Contributor

@nedbat nedbat commented Jun 4, 2019

GitHubSession objects can't be properly copied without the __attrs__
list being correct. The requests Session object implements __getstate__
using the __attrs__ list.

@nedbat
Copy link
Contributor Author

nedbat commented Jun 4, 2019

BTW, I wanted to write a better test, that a pickled session would have the same attributes as the original. But it looks like requests has an error in their __attrs__, so unpickling adds an extra ghost "prefetch" attribute.

@sigmavirus24
Copy link
Owner

Can you rebase? Seems this is behind the HEAD of master and this needs an update

GitHubSession objects can't be properly copied without the __attrs__
list being correct.  The requests Session object implements __getstate__
using the __attrs__ list.
@nedbat
Copy link
Contributor Author

nedbat commented Jun 4, 2019

BTW, requests fix: https://github.com/kennethreitz/requests/pull/5110

@sigmavirus24
Copy link
Owner

I'm not a maintainer of requests anymore. I was mysteriously removed and haven't bothered trying to regain access. I doubt anyone will bother reviewing that or any CVEs for the project at this point

@nedbat
Copy link
Contributor Author

nedbat commented Jun 5, 2019

Would you like a better test for pickling of sessions? I can exclude the known extra attribute from requests, and check that other than that, the original and the unpickled are the same.

@sigmavirus24
Copy link
Owner

No need to worry about it :)

@sigmavirus24 sigmavirus24 merged commit da63ac5 into sigmavirus24:master Jun 5, 2019
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.

2 participants