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

Static range requests #1382

Merged
merged 19 commits into from
Nov 18, 2016
Merged

Conversation

andrewleech
Copy link
Contributor

What do these changes do?

Add support for html range header in static file handling.

Generally follows https://tools.ietf.org/html/rfc7233 however does not support multiple ranges in one request.

Usage is by adding the range header similar to {'Range': 'bytes=0-99'} to a request to receive a partial chunk of the file instead of the entire thing.

Are there changes in behavior for the user?

If the range heading is not explicitly added to a request there should be no change in usage.

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • Add yourself to CONTRIBUTORS.txt
    • The format is <Name> <Surname>.
    • Please keep alphabetical order, the file is sorted by names.
  • Add a new entry to CHANGES.rst
    • Choose any open position to avoid merge conflicts with other PRs.
    • Add a link to the issue you are fixing (if any) using #issue_number format at the end of changelog message. Use Pull Request number if there are no issues for PR or PR covers the issue only partially.

Generally follows  https://tools.ietf.org/html/rfc7233 however does not currently support multiple ranges in one request.
Add PR number to change
Revert changelog update to allow tests to run
Copy link
Member

@fafhrd91 fafhrd91 left a comment

Choose a reason for hiding this comment

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

i think aiohttp should provide Range support on general level


# Handle 206 range response if requested
if 'range' in request.headers:
from .web_exceptions import HTTPPartialContent, HTTPRequestRangeNotSatisfiable
Copy link
Member

Choose a reason for hiding this comment

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

move it to module level import.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I normally would by standard, but was following what had been done a few lines earlier in the same function. I'll fix both.

end = file_size - 1

# Handle 206 range response if requested
if 'range' in request.headers:
Copy link
Member

Choose a reason for hiding this comment

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

i think it make sense to move range related functionality to helper module or even to separate module. and create Range class, it could be useful for external libraries and applications

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had thought that but wasn't sure where would make sense to put it.
It could be in a parse_range_header(request) function in helpers, it's not necessarily making things clearer but would certainly would aid in reuse.

@andrewleech
Copy link
Contributor Author

Global range support would be nice sure, but would probably be a significantly larger change.

That being said most use cases I know of for range is to get part of static resources for pagination or seeking in video files etc. It's less useful on dynamic content where it's harder to know a range to request of something if the content could change in between requests.

@asvetlov
Copy link
Member

asvetlov commented Nov 8, 2016

@andrewleech your PR is very long waited addition.
Thank you for it.
I don't think it's ready for merging right now but you are following in right direction.

Couple brief notes:

  1. parse_range_header should be web.Request property like req.content_lenght or req.content_type. Or even a property of web.BaseRequest after New server #1362 merging. The property result caching is not required, sure.
  2. range_request should be extracted to module level and presented as Public API. The name is not perfect, maybe RequestHeader or RequestHeaderInfo

@fafhrd91
Copy link
Member

fafhrd91 commented Nov 8, 2016

  1. i'd rename range_request to just Range
  2. move parse_range_header to separate module
  3. do not import inside function

…est(), returns (start,end) in format ready to use in a slice of a string/bytearray for convenient reuse

Handling specific to file offset and length calculations brought back into send() of FileSender.
Similar handling could be implemented in other request handlers depending on type of data being requested.
@asvetlov
Copy link
Member

Please run make isort for fixing travis build.

@codecov-io
Copy link

codecov-io commented Nov 11, 2016

Current coverage is 98.88% (diff: 100%)

Merging #1382 into master will increase coverage by 0.09%

@@             master      #1382   diff @@
==========================================
  Files            29         30     +1   
  Lines          6765       6885   +120   
  Methods           0          0          
  Messages          0          0          
  Branches       1124       1142    +18   
==========================================
+ Hits           6683       6808   +125   
+ Misses           38         33     -5   
  Partials         44         44          

Powered by Codecov. Last update 7ced089...c884825

@asvetlov
Copy link
Member

The code looks perfect.
The only thing is left -- documentation.
aiohttp doesn't use autodoc markup (at least I'm trying to remove all places where we still do it).
So, please, drop markup from .http_range property and add explicit documentation for the property to docs/web_reference.rst

@asvetlov asvetlov merged commit 10eff37 into aio-libs:master Nov 18, 2016
@asvetlov
Copy link
Member

Ok, I'll update docs.
Thank you very much!

@asvetlov
Copy link
Member

Done by c693473
I've changed http_range value from tuple to slice also.

@andrewleech
Copy link
Contributor Author

Oh, thanks!
I was going to fix the documentation as suggested but have just been slammed the last week.

That's great about the slice object; I didn't know they existed and my initial search to find one didn't come up with anything. I'll keep that in mind for future!

@lock
Copy link

lock bot commented Oct 29, 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 29, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 29, 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.

4 participants