Implements #89 by adding a FileResponse class

[scallopedllama]

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:

• A shared pointer to an instance of a FileResponse can be returned from a PathHandler to stream that file back
• Another Response object can instantiate a FileResponse and use its respond method to send file data back on a provided ResponseWriter shared pointer

In 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 Range header is supported. Also, the If-Modified-Since, If-Unmodified-Since, and If-Range headers 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-Length header is not provided and the Range header is no longer supported though the If-Modified-Since and If-Unmodified-Since header will still be acknowledged. If the client supports compression but would rather have file resuming ability, compression is disabled if the incoming Accept-Encoding header does not have deflate in it.

This Pull Request implements #89

[codecov[bot]]

Codecov Report

Merging #94 into master will decrease coverage by 2.23%. The diff coverage is 0%.

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

Impacted Files	Coverage Δ
src/main/c/seasocks/Response.h	100% <ø> (ø)	:arrow_up:
src/main/c/seasocks/ZlibContext.cpp	0% <ø> (ø)	:arrow_up:
src/main/c/Response.cpp	90.47% <0%> (-9.53%)	:arrow_down:
src/main/c/Connection.cpp	22% <0%> (-0.03%)	:arrow_down:
src/main/c/StringUtil.cpp	75% <0%> (-6.36%)	:arrow_down:
src/main/c/seasocks/ResponseCodeDefs.h	3.03% <0%> (-0.2%)	:arrow_down:
src/main/c/seasocks/util/FileResponse.h	0% <0%> (ø)
src/main/c/util/FileResponse.cpp	0% <0%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 4daa1e3...0c473a4. Read the comment docs.

[hoytech]

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 sendfile(2), pre-gzipped files, etc).

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

[hoytech]

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!

[scallopedllama]

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 sendfile(2), pre-gzipped files, etc).

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

[scallopedllama]

Added the Vary header to the response so it will be cached correctly.

[offa]

Wow, you've put a lot of work into this! I've added some implementation related comments.

Btw. some tests would be good.

[offa]

I've added some implementation related comments.

… which are not visible until Submit review is pressed …

[mattgodbolt]

@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?

[offa]

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?

1. Contribute via PR to this PR
2. Contribute directly here (can we?!)
3. Merge to a development branch and once finished to master

I'm fine with all of them. We could also merge this to master and then work on it, but this is probably not the best way to go. @scallopedllama / @mattgodbolt what do you think?

[offa]

ping @scallopedllama

[mattgodbolt]

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.

[scallopedllama]

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 [email protected] 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 https://github.com/mattgodbolt/seasocks/pull/94#issuecomment-427975957, or mute the thread https://github.com/notifications/unsubscribe-auth/AALVdi3_cCiglEnsP1WxHawPliRIdHL4ks5ui7uQgaJpZM4V0aeF .