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

Static range requests #1382

merged 19 commits into from Nov 18, 2016

Conversation

@andrewleech
Copy link
Contributor

@andrewleech andrewleech commented Nov 8, 2016

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.
andrewleech added 4 commits Nov 8, 2016
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

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

This comment has been minimized.

@fafhrd91

fafhrd91 Nov 8, 2016
Member

move it to module level import.

This comment has been minimized.

@andrewleech

andrewleech Nov 8, 2016
Author Contributor

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:

This comment has been minimized.

@fafhrd91

fafhrd91 Nov 8, 2016
Member

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

This comment has been minimized.

@andrewleech

andrewleech Nov 8, 2016
Author Contributor

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

@andrewleech andrewleech commented Nov 8, 2016

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 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 #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 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
andrewleech added 3 commits Nov 10, 2016
…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

@asvetlov asvetlov commented Nov 11, 2016

Please run make isort for fixing travis build.

@codecov-io
Copy link

@codecov-io 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

@asvetlov asvetlov commented Nov 15, 2016

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
4 checks passed
4 checks passed
codecov/patch 100% of diff hit (target 98.78%)
Details
codecov/project 98.88% (+0.09%) compared to 7ced089
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@asvetlov
Copy link
Member

@asvetlov asvetlov commented Nov 18, 2016

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

@asvetlov
Copy link
Member

@asvetlov asvetlov commented Nov 18, 2016

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

@andrewleech
Copy link
Contributor Author

@andrewleech andrewleech commented Nov 20, 2016

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!

@andrewleech andrewleech deleted the andrewleech:static_range_requests branch Nov 20, 2016
@lock
Copy link

@lock 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.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.