-
Notifications
You must be signed in to change notification settings - Fork 382
http: general enhancements #731
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
Conversation
Sorry I didn't comment yet. I can look in detail, but currently |
Ah, didnt notice that. Seems irrelevant though |
Oh, you already fixed, nvm. |
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 like what's here, but have some comments.
kw = self.kwargs.copy() | ||
kw.update(kwargs) | ||
logger.debug(rpath) | ||
session = await self.set_session() | ||
async with session.get(rpath, **self.kwargs) as r: | ||
try: | ||
size = int(r.headers["content-length"]) |
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.
What are the chances of this being wrong due to transfer compression? (_file_info
has head["Accept-Encoding"] = "identity"
)
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.
Humm, not sure. This seemed like the easiest way to predict the general length, though there might certainly some catches about it. Do you have other suggestions?
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.
If the purpose is to report on the transfer, then I suppose it's not important - but the number of bytes written to disk won't match. I don't know if aiohttp makes available the actual number of bytes read on the stream before decompression (probably no).
Thanks for the review @martindurant, all should be addressed now! |
Thanks for making HTTP r/w! Maybe the File object can be made writable too (with the limitation of being able to PUT the file in one call!). We'll see if the file size for the callback ends up being a problem. |
TODO: