Skip to content

Pickled Responses should include a None value for the raw attribute - #1949

Merged
kennethreitz merged 1 commit into
psf:masterfrom
Lukasa:pickle
Mar 12, 2014
Merged

kennethreitz merged 1 commit into
psf:masterfrom
Lukasa:pickle

Conversation

@Lukasa

@Lukasa Lukasa commented Mar 12, 2014

Copy link
Copy Markdown
Member

I received this issue in CacheControl where a pickled response throws an error because there is no raw attribute on the response object. The __setstate__ function should set this explicitly to an empty urllib3.response.HTTPResponse.

@Lukasa

Lukasa commented Mar 12, 2014

Copy link
Copy Markdown
Member

Yep, this is a good catch. We should fix this up.

@Lukasa

Lukasa commented Mar 12, 2014

Copy link
Copy Markdown
Member

I don't think we should add a HTTPResponse object because it violates Kenneth's condition about being too tied to urllib3. It also makes an incorrect assumption that urllib3 is always being used to provide a raw response, which is simply untrue.

I'd argue it should be present and set to None.

Some people will assume that .raw() is present, and they shouldn't get
AttributeErrors when they make that assumption on a pickled Response.
However, @kennethreitz has asked that we not be too dependent on
urllib3. For that reason, set to None.
@Lukasa

Lukasa commented Mar 12, 2014

Copy link
Copy Markdown
Member

Can I get review from @sigmavirus24 and @ionrock?

kennethreitz added a commit that referenced this pull request Mar 12, 2014
Pickled Responses should include a None value for the raw attribute
@kennethreitz
kennethreitz merged commit 4112b8f into psf:master Mar 12, 2014
@ionrock

ionrock commented Mar 12, 2014

Copy link
Copy Markdown
Contributor Author

This seems to fix things in CacheControl. Thank you @Lukasa!

My reasoning for suggesting the HTTPResponse was because then someone doesn't have to check whether or not a response was cached or created outside a normal request. For example, if you used resp.raw.seek(0) you'd get an error as None obviously doesn't have a seek method.

With that said, seek doesn't work anyway! No blood, no foul.

Thanks for fixing this so quickly. I'll add some docs in CacheControl to help communicate that a cached response's raw attribute will be None.

@sigmavirus24

Copy link
Copy Markdown
Contributor

Not that you needed it, but 👍. Also we should document this for others relying on pickling and using libraries like multiprocessing (which pickle objects).

@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants