Skip to content

Make Request and Response picklable - #1579

Merged
lovelydinosaur merged 6 commits into
encode:masterfrom
hannseman:pickles
Apr 21, 2021
Merged

Make Request and Response picklable#1579
lovelydinosaur merged 6 commits into
encode:masterfrom
hannseman:pickles

Conversation

@hannseman

@hannsemanhannseman commented Apr 16, 2021

Copy link
Copy Markdown
Contributor

Refs: #1562

I took the liberty of creating a PR as I felt that there was a consensus about implementing this.

Some open questions is what state the instances should end up in after being loaded. I.e should we override is_closed, is_stream_consumed and if so to what? I also think we need to set stream to something to avoid unexpected AttributeError being raised on access. Currently set it to an empty ByteStream to make mypy happy.

It might also be cleaner to set the overrides in the dict produced in __getstate__ instead of directly in __setstate__.

An alternative to the __attrs__ pattern would be to pop invalid attributes from __dict__ in __getstate__ but being explicit about it feels safer.

@hannsemanhannseman changed the title Make Request and Response picklable (#1562)Make Request and Response picklableApr 16, 2021
@hannseman
hannsemanforce-pushed the pickles branch 3 times, most recently from e69822a to 23b4be2CompareApril 16, 2021 19:54
@lovelydinosaur

lovelydinosaur commented Apr 19, 2021

Copy link
Copy Markdown
Contributor

Okay, so thoughts here...

Firstly, I don't really like the __attrs__ thing. Although it looks like a Python built-in, it turns out that's just a bit of requests internal naming that they decided on.

I think we probably want to approach this like so:

classUnattachedStream(AsyncByteStream, SyncByteStream):
""" If a request or response is serialized using pickle, then it is no longer attached to a stream for I/O purposes. Any stream operations should result in `httpx.StreamClosed`. """def__iter__(self) ->Iterator[bytes]:
raiseStreamClosed()
asyncdef__aiter__(self) ->AsyncIterator[bytes]:
raiseStreamClosed()
classResponse:
...
def__getstate__(self):
return {
name: valueforname, valueinself.__dict__.items()
ifnamenotin ['stream', 'is_closed', '_decoder']
}
def__setstate__(self, state):
forname, valueinstate.items():
setattr(self, name, value)
self.is_closed=Trueself.stream=UnattachedStream()
classRequest:
...
def__getstate__(self):
return {
name: valueforname, valueinself.__dict__.items()
ifnamenotin ['stream']
}
def__setstate__(self, state):
forname, valueinstate.items():
setattr(self, name, value)
self.stream=UnattachedStream()

I'd consider #1584 to be a pre-requisite.

The tidying up in #1583 is also relevant here.

(Aside: We might well end up with an is_closed on the Request class too at some point in the future, but we don't really need to consider that here.)

@lovelydinosaurlovelydinosaur added this to the v0.18 milestone Apr 19, 2021
@lovelydinosaurlovelydinosaur added the enhancement New feature or request label Apr 19, 2021
@hannseman
hannsemanforce-pushed the pickles branch 4 times, most recently from 676eafa to 2201d8bCompareApril 19, 2021 17:07
@hannseman

Copy link
Copy Markdown
ContributorAuthor

@tomchristie thanks for the review. I agree that the __attrs__ thing felt a bit awkward, much better 👍

@hannseman
hannsemanforce-pushed the pickles branch 2 times, most recently from d8a99d8 to 0ad63e3CompareApril 19, 2021 17:16
Comment threadhttpx/_content.py

async def __aiter__(self) -> AsyncIterator[bytes]:
raise ResponseClosed() # TODO: StreamClosed
yield b"" # pragma: nocover

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

We need the yield here to avoid TypeError:

 async def aread(self) -> bytes:
"""
Read and return the request content.
"""
if not hasattr(self, "_content"):
assert isinstance(self.stream, typing.AsyncIterable)
> self._content = b"".join([part async for part in self.stream])
E TypeError: 'async for' received an object from __aiter__ that does not implement __anext__: coroutine

Comment threadtests/models/test_responses.py Outdated
Comment threadtests/models/test_responses.py Outdated
Comment threadhttpx/_content.py Outdated
Comment threadhttpx/_content.py Outdated
Comment threadhttpx/_content.py Outdated
Comment threadtests/models/test_responses.py Outdated
Comment threadtests/models/test_requests.py Outdated
Comment threadtests/models/test_requests.py Outdated
Comment threadtests/models/test_requests.py Outdated
@lovelydinosaur

Copy link
Copy Markdown
Contributor

Right, let's go with this.

Great work @hannseman!

@lovelydinosaur
lovelydinosaur merged commit 2d57104 into encode:masterApr 21, 2021
@hannseman

Copy link
Copy Markdown
ContributorAuthor

@tomchristie thanks a lot for the review and fixups! 🎉

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancementNew feature or request

Development

Successfully merging this pull request may close these issues.

2 participants

@hannseman@lovelydinosaur