-
Notifications
You must be signed in to change notification settings - Fork 5
add support for streaming uploads #8
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
base: master
Are you sure you want to change the base?
Conversation
|
Hi @fabian-paul, Sorry! I only just noticed this PR (end-of-year was pretty crazy). Supporting streaming uploads sounds like a great idea. David |
| elif isinstance(self.prepared.body, (io.RawIOBase, io.BufferedIOBase)): | ||
| self.curl.setopt(pycurl.READFUNCTION, self.prepared.body.read) | ||
| self.curl.setopt(pycurl.TRANSFER_ENCODING, 1) | ||
| elif hasattr(self.prepared.body, "__iter__"): # TODO: call iter instead of checking (e.g. to support delegates) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be better to use isinstance(obj, Collection).
collections.abc.Collection guarantees that a type implements __contains__, __iter__ and __len__. You can also use collections.abc.Iterable to specifically test for __iter__.
| else: | ||
| self.curl.setopt(pycurl.TRANSFER_ENCODING, 0) | ||
| self.curl.setopt(pycurl.INFILESIZE_LARGE, n_bytes) | ||
| reader = ChunkIterableReader(iter(self.prepared.body)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we can use the two argument form of iter here:
iter(self.prepared.body, "") # for string
iter(self.prepared.body, b"") # for bytes| def close(self): # TODO | ||
| try: | ||
| self._iterator.close() | ||
| except AttributeError: | ||
| pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you should be closing the iterator from within the library. Typically I would expect a user to be using a with block or calling close manually.
| IS_PYCURL_REQUESTS = requests.__name__ == 'pycurl_requests' | ||
|
|
||
|
|
||
| test_data = bytes(random.getrandbits(8) for _ in range(123456)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd recommend using os.urandom here.
| if not allow_chunked: | ||
| self.response('This endpoint has chunked transfer deactivated.', status=(400, "Bad Request")) | ||
| return | ||
| body = b"" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Joining immutable bytes objects can be potentially expensive. bytearray is a better choice if you need something mutable.
Hi all,
I happened to use streaming uploads with requests and curl a lot (using both individually but not in combination). So I felt like adding support for the data access protocols that requests supports to this cool project would a useful contribution.
The PR isn't ready yet and I hope to finish it soon. There are still a few sections that I have marked with TODO comments. I'm also not completely sure how the Content-Length header and libcurl's
INFILESIZE_LARGEinteract. Would be nice to have your opinion on this PR.Fabian