Skip to content
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

add support for streaming uploads #8

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fabian-paul
Copy link

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_LARGE interact. Would be nice to have your opinion on this PR.

Fabian

@dcoles dcoles self-requested a review January 6, 2023 07:44
@dcoles
Copy link
Owner

dcoles commented Jan 6, 2023

Hi @fabian-paul,

Sorry! I only just noticed this PR (end-of-year was pretty crazy).

Supporting streaming uploads sounds like a great idea.
I'll take a look over the PR tomorrow morning.

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)
Copy link
Owner

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))
Copy link
Owner

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

Comment on lines +93 to +97
def close(self): # TODO
try:
self._iterator.close()
except AttributeError:
pass
Copy link
Owner

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 this _really_ PyCurl-Requests?
#: Should be used when testing for PyCurl-Requests extensions.
IS_PYCURL_REQUESTS = requests.__name__ == 'pycurl_requests'


test_data = bytes(random.getrandbits(8) for _ in range(123456))
Copy link
Owner

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""
Copy link
Owner

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.

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