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

Fixing a couple of issues with HTTP-Range for static files #414

Closed
wants to merge 1 commit into from
Closed

Fixing a couple of issues with HTTP-Range for static files #414

wants to merge 1 commit into from

Conversation

augensalat
Copy link
Contributor

Moin sri,

Mojolicious had some problems with certain HTTP-Range requests.
Here are my changes:

  • fixed bug where a wrong Content-Length was calculated
    for Range requests on empty files
  • fixed bug where a wrong Content-Length was calculated
    for swapped Range values
  • do not silently ignore non-numeric value in Range
  • support HTTP-Range for final bytes (Range: bytes -42)

- fixed bug where a wrong Content-Length was calculated
  for Range requests on empty files
- fixed bug where a wrong Content-Length was calculated
  for swapped Range values
- do not silently ignore non-numeric value in Range
- support HTTP-Range for final bytes (Range: bytes -42)
@kraih
Copy link
Member

kraih commented Nov 20, 2012

And why is range support for empty files useful?

@kraih
Copy link
Member

kraih commented Nov 20, 2012

Also, which user agents actually use Range: bytes=-42?

@kraih
Copy link
Member

kraih commented Nov 20, 2012

In the future, please do not bundle multiple changes into a single pull request, unless they all have been discussed and accepted already, odds are the whole patch gets rejected because of a small subset.

@kraih
Copy link
Member

kraih commented Nov 20, 2012

Since handling range requests for empty files does not actually require additional code, i've just added it anyway. 6804499

@kraih kraih closed this in 77c38c0 Nov 20, 2012
@kraih
Copy link
Member

kraih commented Nov 20, 2012

Turns out a clean implementation is actually less code, that made the decision very simple. :)

@augensalat
Copy link
Contributor Author

And why is range support for empty files useful?

Wrong question. M was pretending "Content-Length: 1" and returned 0
bytes, which was clearly a bug.

@augensalat
Copy link
Contributor Author

Also, which user agents actually use |Range: bytes=-42|?

It's defined in RFC 2616 §14.35.1, therefore a server should handle it,
if possible.

@augensalat
Copy link
Contributor Author

In the future, please do not bundle multiple changes into a single pull
request, unless they all have been discussed and accepted already, odds
are the whole patch gets rejected because of a small subset.

OK. Thank you.

@augensalat
Copy link
Contributor Author

Turns out a clean implementation is actually less code, that made the
decision very simple. :)

from RFC 2616 §14.35.1

If the last-byte-pos value is absent, or if the value is greater than or
equal to the current length of the entity-body, last-byte-pos is taken
to be equal to one less than the current length of the entity- body in
bytes.

Your way doesn't handle that correctly atm.

@kraih
Copy link
Member

kraih commented Nov 20, 2012

It's defined in RFC 2616 §14.35.1, therefore a server should handle it, if possible.

That's not a good reason, there are many things in the RFCs around HTTP that don't really belong in the real world. For example we will never support multipart/byteranges.

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.

None yet

2 participants