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

Fix file upload not chunked #327

Merged
merged 3 commits into from
Apr 15, 2015
Merged

Fix file upload not chunked #327

merged 3 commits into from
Apr 15, 2015

Conversation

hoh
Copy link
Contributor

@hoh hoh commented Apr 14, 2015

The upload of a single file is not chunked anymore as the content-length can be determined,
except if the chunked argument is passed forced as True.

This fixes #126 for uploading a single file, but does not apply to multipart uploads.

The upload of a single file is not chunked anymore as the content-length can be determined,
except if the chunked argument is passed forced as True.

This fixes #126 for uploading a single file, but does not apply to multipart uploads.
@fafhrd91
Copy link
Member

@kxepal could you review this PR?

self.chunked = True
if not self.chunked and isinstance(data, io.BufferedReader):
# Not chunking if content-length can be determined
self.headers[hdrs.CONTENT_LENGTH] = str(getsize(data.name))
Copy link
Member

Choose a reason for hiding this comment

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

What if file was read a bit before it's going to be sent with a request?

with open('foo.bar', 'rb') as f:
  magic = f.read(3)
  ...
  yield from aiohttp.request('post', 'http://localhost/upload', data=f)

In this case Content-Length header will declare more bytes than actually could be sent. Use tell() to figure out how much were already read. Also, since you have file description here, you can do just os.fstat(data.fileno()).st_size what is a bit more efficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thx, I'll have a look at that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed both seeking and more efficient file size access.

@kxepal
Copy link
Member

kxepal commented Apr 14, 2015

@fafhrd91 sure.

self.chunked = True
if not self.chunked and isinstance(data, io.BufferedReader):
# Not chunking if content-length can be determined
size = fstat(data.fileno()).st_size - data.tell()
Copy link
Member

Choose a reason for hiding this comment

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

I would use full os.fstat here since it doesn't simply to figure out what fstat is and where it comes from. Also look on the other imports from stdlib.

@kxepal
Copy link
Member

kxepal commented Apr 14, 2015

LGFM expect the import thing. Please, squash all the commits and it will be ready for merge (:

fafhrd91 added a commit that referenced this pull request Apr 15, 2015
@fafhrd91 fafhrd91 merged commit 26f8100 into aio-libs:master Apr 15, 2015
@asvetlov
Copy link
Member

Thanks, Hugo!

@asvetlov
Copy link
Member

@kxepal looks like your request for os.fstat is not satisfied yet.
Would you apply it as separate commit on your own?

kxepal added a commit that referenced this pull request Apr 15, 2015
@kxepal
Copy link
Member

kxepal commented Apr 15, 2015

@asvetlov yes, fixed that. thanks for notice!

@hoh hoh deleted the upload_nochunked branch April 15, 2015 16:23
@lock
Copy link

lock bot commented Oct 30, 2019

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

@lock lock bot added the outdated label Oct 30, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Uploading files shouldn't always use chunked, especally when HTTP1.0 is used.
4 participants