Add partial support for the 'Range' header when serving static files #290
Conversation
atheriel
force-pushed
the
range-requests
branch
2 times, most recently
from
5a12894
to
64444d5
Compare
|
Conflicts and tests on Windows fixed. |
src/filedatasource-win.cpp
Outdated
| @@ -63,6 +63,10 @@ uint64_t FileDataSource::size() const { | |||
| return _length.QuadPart; | |||
| } | |||
|
|
|||
| bool FileDataSource::setRange(size_t start, size_t end) { | |||
| return false; | |||
There was a problem hiding this comment.
This PR will need a Windows implementation.
There was a problem hiding this comment.
I do not have a Windows test environment at the moment -- is that a dealbreaker?
There was a problem hiding this comment.
Yes -- no Windows support for this would be a dealbreaker. Whenever possible, we don't want to have features that work on one platform and not another. We want code to work consistently across platforms -- it's a bad experience if someone's code works on one computer, but not another. For example, imagine if static file serving worked on Unixes but not Windows. Then someone would complain about it, and then someone is going to be on the hook to implement it.
If there's some technical reason that a feature can't work on Windows, that's one thing. For example, the Windows process model doesn't use fork, so some parallel processing code won't work. That's a legitimate reason for only supporting Unix -- but even so, it's an annoying one for users to have to work around (speaking from experience). If there's not a technical reason like that, then the code really should be made to work on all platforms.
There was a problem hiding this comment.
That's a fair assessment, but it will take me some time. I'll try to resolve the other issues with this PR in the meantime.
atheriel
force-pushed
the
range-requests
branch
4 times, most recently
from
0585d2b
to
a8ab17a
Compare
This includes * Parsing and validation of the most common forms[0] of the `Range` header, namely `bytes=0-1000` and `bytes=1001-`, for serving slices of files and enabling resumable downloads, etc. * Support for returning partial files matching these slices on Unix and Windows platforms. * Support for returning the appropriate `Content-Range` header and HTTP 206 responses for these requests. * Support for sending the `Accept-Ranges: bytes` header when appropriate. * Tests for these features. There is no support for suffix length ranges, multipart ranges[2] or the `If-Range` header[3]. Since `Range` header support is always optional (a server can just respond with the whole file and a HTTP 200 instead), we just fall back on existing behaviour in these cases instead of issuing some kind of error. Closes rstudio#259. [0]: https://tools.ietf.org/html/rfc7233#section-3.1 [2]: https://tools.ietf.org/html/rfc7233#appendix-A [3]: https://tools.ietf.org/html/rfc7233#section-3.2 [4]: https://tools.ietf.org/html/rfc7233#section-2.3
atheriel
force-pushed
the
range-requests
branch
from
a8ab17a
to
9f16769
Compare
|
|
As proposed in #259. I haven't done extensive 'real-world' testing, but I've been able to use this to enable rewinding/skipping in HTML
<audio>tags in a Shiny app.This PR includes
Parsing and validation of the most common forms of the
Rangeheader, namelybytes=0-1000andbytes=1001-, for serving slices of files and enabling resumable downloads, etc.Support for returning partial files matching these slices on Unix and Windows platforms.
Support for returning the appropriate
Content-Rangeheader and HTTP 206 responses for these requests.Tests for these features.
It does not support Windows as of yet, largely because I don't have a good test environment -- it should be possible to accomplish by passing some
OVERLAPPEDstructure toReadFile().There is also no support for advanced features such as multipart ranges or the
If-Rangeheader.Since
Rangeheader support is always optional (a server can just respond with the whole file and a HTTP 200 instead), we just fall back on existing behaviour in these cases instead of issuing some kind of error.Along these same lines, we don't yet advertise that we support theRangeheader by sending anAccept-Ranges: bytesheader on other GET/HEAD requests.Closes #259.