Implements #89 by adding a FileResponse class #94
Conversation
Add a string webtime parser that returns a time_t to go with the webtime generator that takes a time_t and returns a string.
Codecov Report
@@ Coverage Diff @@
## master #94 +/- ##
========================================
- Coverage 39.23% 37% -2.24%
========================================
Files 40 42 +2
Lines 2039 2162 +123
========================================
Hits 800 800
- Misses 1239 1362 +123
Continue to review full report at Codecov.
|
|
Personally I would consider if it's possible for your application to use a server that specialises in file serving (nginx is good) because it can probably do it much more efficiently (ie using That said, I'm not against this as long as it doesn't increase the attack surface for apps that just want to use websockets (which of course is the main point of seasocks). Also I quickly grepped your patch and didn't see any Vary headers, you'll probably want to add a "Vary: Accept-Encoding": https://blog.stackpath.com/accept-encoding-vary-important |
|
I just realised my comment may have come off sounding discouraging, sorry for that. I actually do think this is a worthwhile patch! Also, this was just my opinion and I of course don't speak for @mattgodbolt nor about the direction he is planning for seasocks. Cheers! |
Definitely would have used an engine like that if most of the things that my application was doing was returning files but for the most part it's programmatic. Person accesses address, C++ function is called, data is returned. So far Seasocks has been a fantastic match for the common case. It's just that a few addresses return an actual file from the filesystem which is why I need this thing. Doesn't need to be fancy, just work well enough. I'll look into the Vary header, thanks for the great link |
A shared pointer to a FileResponse can be returned from a PathHandler's handle() method to asynchronously return the indicated file back to a client. It understands and supports the Range header for non-multipart byte ranges along with the If-Range, If-Modified-Since, and If-Unmodified-Since conditional headers. Additionally, if enabled during construction and the client supports it, the file data being sent back to the client is compressed using the built in zlib support for deflate Content-Encoding. This does not compress ahead of time and cache the compressed data, instead the data is compressed while it is sent back to the client. Because of this, content length is not known ahead of time and resume features are disabled.
Add a static fileResponse method that creates and returns a file response, modeled after the text, json, and html response methods.
scallopedllama
force-pushed
the
file-response
branch
from
8e31e0f to
0c473a4
Compare
|
Added the Vary header to the response so it will be cached correctly. |
|
Wow, you've put a lot of work into this! I've added some implementation related comments. Btw. some tests would be good. |
| @@ -52,6 +53,7 @@ class Response { | |||
| static std::shared_ptr<Response> textResponse(const std::string& response); | |||
| static std::shared_ptr<Response> jsonResponse(const std::string& response); | |||
| static std::shared_ptr<Response> htmlResponse(const std::string& response); | |||
| static std::shared_ptr<Response> fileResponse(const Request &request, const std::string &filePath, const std::string &contentType, bool allowCompression, bool allowCaching); | |||
There was a problem hiding this comment.
Is a forward declaration possible here (to avoid the inclusion of Request.h)?
| @@ -0,0 +1,71 @@ | |||
| // Copyright (c) 2018, Joe Balough | |||
There was a problem hiding this comment.
@scallopedllama / @mattgodbolt does this require an user to handle two licenses – the default text and this one?
There was a problem hiding this comment.
I'm no lawyer, but I don't think the copyright affects the license. In other projects I have the author put their name, as long as the license remains intact below
| #include "seasocks/ToString.h" | ||
| #include "seasocks/ZlibContext.h" | ||
|
|
||
| using namespace seasocks; |
There was a problem hiding this comment.
This should be better wrapped into namespace seasocks { ... }.
|
|
||
| using namespace seasocks; | ||
|
|
||
| constexpr size_t ReadWriteBufferSize = 16 * 1024; |
There was a problem hiding this comment.
Probably anonymous namespace here and variables usually start with lower case (just to be consistent).
|
|
||
| // If compression is allowed, try to initialize the zlib context here | ||
| // It will throw a runtime_error if zlib was diabled in the build | ||
| if (sendCompressedData) { |
There was a problem hiding this comment.
We should improve this. Would it help you to have some way to directly determine whether compression is enabled / disabled?
There was a problem hiding this comment.
Also probably we can avoid the if (sendCompressedData) { ... } checks and extract it to a single place (at least it would make sense).
| } | ||
| bytesLeft -= bytesRead; | ||
| // compress data if possible | ||
| if (sendCompressedData) { |
There was a problem hiding this comment.
See above; maybe we can improve the zlib stuff in this function.
| std::string rangeString = _request.getHeader("Range"); | ||
|
|
||
| // Check for multipart range | ||
| std::regex multipartRegex("^\\S+=[0-9]+-[0-9]*, [0-9]+-[0-9]*"); |
There was a problem hiding this comment.
Instances of std::regex should be used with care as it may have poor performance on creation. It makes sense to make it const and create it only once therefore. Actually moving it as a const-variable into the anonymous namespace should do the trick.
(Same to line 240).
| @@ -125,6 +126,15 @@ std::string webtime(time_t time) { | |||
| return buf; | |||
| } | |||
|
|
|||
| time_t webtime(std::string time) { | |||
| @@ -48,6 +48,8 @@ void replace(std::string& string, const std::string& find, const std::string& re | |||
| bool caseInsensitiveSame(const std::string &lhs, const std::string &rhs); | |||
|
|
|||
| std::string webtime(time_t time); | |||
| // Returns -1 on error | |||
| time_t webtime(std::string time); | |||
There was a problem hiding this comment.
Is it reasonable to throw an exception in case of an error here?
|
|
||
| private: | ||
|
|
||
| ResponseCode parseHeaders(const off_t fileSize, const time_t fileLastModified, bool sendCompressedData, off_t& fileTransferStart, off_t& fileTransferEnd) const; |
There was a problem hiding this comment.
If possible avoid out-parameters and eg. return a tuple.
… which are not visible until Submit review is pressed … |
|
@offa how do you feel about this change? I skim-read the comments and all looks OK to me...are we waiting on the author to address anything? |
|
I think some of the comments should be addressed before. And we should get some tests in. But actually there's nothing dramatic, just a few things which have to be done. We / I can help @scallopedllama to fix them of course. But which way do we go?
I'm fine with all of them. We could also merge this to |
|
ping @scallopedllama |
|
Another ping @scallopedllama :) Thanks again for this, would be great to get it merged in. I'd prefer working off-master (another branch in this project) is fine, if we need to work on the repo here. |
|
Sorry for the delay on this, I haven't had the time to work on the changes.
I'll look into getting some time to work on it soon!
…On Mon, Oct 8, 2018 at 4:54 PM Matt Godbolt ***@***.***> wrote:
Another ping @scallopedllama <https://github.com/scallopedllama> :)
Thanks again for this, would be great to get it merged in. I'd prefer
working off-master (another branch in this project) is fine, if we need to
work on the repo here.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#94 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AALVdi3_cCiglEnsP1WxHawPliRIdHL4ks5ui7uQgaJpZM4V0aeF>
.
|
This Pull Request adds a new class FileResponse which will asynchronously stream file data back to a client.
It can be used in two ways:
respondmethod to send file data back on a provided ResponseWriter shared pointerIn either situation, the file data is read and queued up for transmission on the server thread to prevent stalling other requests. If zlib support is enabled and it is explicitly enabled within the constructor, the data response can be compressed with the deflate method.
When the file being sent back is uncompressed, file resuming with the
Rangeheader is supported. Also, theIf-Modified-Since,If-Unmodified-Since, andIf-Rangeheaders are supported, using the file's last modification time to determine this. Support for these headers required the addition of a webtime string parser to be added to the StringUtil class.When the file being sent back is compressed, it is not compressed ahead of time but while it is being loaded from the disk. As a consequence, the final data size is unknown so a
Content-Lengthheader is not provided and theRangeheader is no longer supported though theIf-Modified-SinceandIf-Unmodified-Sinceheader will still be acknowledged. If the client supports compression but would rather have file resuming ability, compression is disabled if the incomingAccept-Encodingheader does not havedeflatein it.This Pull Request implements #89