Skip to content

add support for streaming uploads - #8

Open
fabian-paul wants to merge 1 commit into
dcoles:masterfrom
fabian-paul:add_streaming_upload
Open

add support for streaming uploads#8
fabian-paul wants to merge 1 commit into
dcoles:masterfrom
fabian-paul:add_streaming_upload

Conversation

@fabian-paul

Copy link
Copy Markdown

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

dcoles commented Jan 6, 2023

Copy link
Copy Markdown
Owner

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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_PYCURL_REQUESTS = requests.__name__ == 'pycurl_requests'


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

Copy link
Copy Markdown
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
Copy Markdown
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