[WIP] New Tide middleware: Automatic compression handling #117

kimsnj · 2019-01-15T12:04:52Z

Description

This PR aims at brining a new compression handling middleware to Tide.
The goal of it would to automatically:

decompress incoming data if it's compressed
detect the preferred encoding from the request and encode the response accordingly.

This is still work-in-progress for several reasons:

Missing features:

Some target features are not there yet (but I guess this could come with time)

Decompression of request
Brotli support
Quality value support (by the way, I expected to find their parsing in http/hyper without success. Did I miss something?)

Streaming compression

I struggled (and actually didn't manage) to compress the body as a stream of chunks.

Trying to use the Stream impl of hyper::Body, I encountered a few challenges:

Compiler complaints as I get mixed up between futures 0.1 and 0.3 versions
I was not familiar enough with the flate2 interface. Can the AsyncRead/AsyncWrite version be used?

I guess I should probably manually implement a Stream and call flate2::Compress raw struct.
Any hint on how to best go about this are very welcome :)

Meanwhile, to get something working, I used read_to_vec on tide::Body. This works well for small response… But will put more memory pressure payloads (e.g. streaming a file compressed).

Error handling

For some errors, it is quite straight-forward to have a fallback behaviour (e.g. if compression fails, return body as is), but others are un-clear to me (e.g. what should happen in read_to_vec fails?).

Motivation and Context

This follows from issue #26

How Has This Been Tested?

(automated) Unittests in the compression middleware
(manual) Integrated to the hello world example and ping via curl with various headers

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

yoshuawuyts · 2019-01-15T15:08:46Z

src/middleware/compression.rs

+            "gzip" => Some(Gzip),
+            "deflate" => Some(Deflate),
+            "identity" => Some(Identity),
+            _ => None,


What is the difference between Some(Identity) and None? My understanding is that they're both treated the same way, so perhaps we could could combine these to make matching on them easier?

Right now, the behavior for both is the same.
I suspect it might come in handy to have the explicit differentiation once quality value.
A request containing identity;q=0,unknown-compressio-algo could (should?) respond 406 Not Acceptable

yoshuawuyts · 2019-01-15T15:09:42Z

Overall this looks very promising! Thanks for putting in the work @kimsnj. Keen to see how this PR evolves!

aturon · 2019-04-24T21:35:58Z

@kimsnj just checking in on this -- are you still planning to finish this PR?

kimsnj · 2019-04-25T13:58:19Z

Hi @aturon,

TLDR: I must admit I have a bit abandoned it the last few months due to other priorities, but could be back to it in the upcoming weeks if it's not taken in the meantime.

Since the last update, I took a bit the time to play with flate2 to try to figure out how to use compression for a Stream.
My findings so far would be to directly use the raw Compress struct with a buffer held by the middleware for the (temporary) output, feeding each generated chunk to flate2 and streaming back each of its outputs.
I have tried it successfully for Deflate compression in a stand-alone synchronous example, but didn't have time to integrate it.
GZip would then need an extra header and checksum.

Does this approach seems fine?

Regarding this PR, should I close it in the meantime to avoid cluttering the pending ones on this repo, and re-open it once I have an update ?

pickfire · 2019-04-26T02:30:46Z

src/middleware/compression.rs

+use futures::future::FutureObj;
+
+use crate::body::Body;
+use crate::{middleware::RequestContext, Middleware, Response};


Why not merge both into

use crate::{body::Body, middleware::RequestContext, Middleware, Response};

pickfire · 2019-04-26T02:36:10Z

src/middleware/compression.rs

+            .iter()
+            .filter_map(|hval| hval.to_str().ok())
+            .flat_map(|val| val.split(|c| c == ','))
+            .filter_map(|encoding| Encoding::parse(encoding))


Suggested change

.filter_map(|encoding| Encoding::parse(encoding))

.filter_map(Encoding::parse)

IIRC, clippy have a warning related to that.

pickfire · 2019-04-26T02:41:36Z

src/middleware/compression.rs

+        let mut buf = Vec::new();
+        match encoding {
+            Encoding::Gzip => {
+                let mut gz = flate2::bufread::GzEncoder::new(body, flate2::Compression::fast());


Should we not use flate2::Compression::default() rather than flate2::Compression::fast()?

pickfire · 2019-04-26T02:46:40Z

src/middleware/compression.rs

+    }
+
+    fn apply(body: &[u8], encoding: Encoding) -> Option<Vec<u8>> {
+        let mut buf = Vec::new();


I wonder if this buf is being allocated even if there is no encoding to be done (see the None block below)?

As well, I think it might be possible to not have an Option returned here with compile-time checking?

fairingrey · 2019-04-26T23:45:06Z

@kimsnj I'm willing to try working on this WIP middleware if you're busy, though it might happen in a separate PR.

kimsnj · 2019-04-28T14:51:17Z

@fairingrey I'll close this PR for now so that you can pick it up if you want and are available.

If this is of any help, here is a gist where I played around with flate API. I planned to use something like raw_compress to read from a stream and create a new one.

fairingrey · 2019-04-28T16:41:46Z

@kimsnj Much appreciated! It will help, actually, as I'm figuring out a strategy for encoding the Body stream.

The other PR is here if you're interested in taking a look: #194

Addition of a compression middleware

77390ca

yoshuawuyts reviewed Jan 15, 2019

View reviewed changes

pickfire reviewed Apr 26, 2019

View reviewed changes

fairingrey mentioned this pull request Apr 28, 2019

Middleware-based compression and decompression #194

Merged

8 tasks

kimsnj closed this Apr 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[WIP] New Tide middleware: Automatic compression handling #117

[WIP] New Tide middleware: Automatic compression handling #117

Uh oh!

kimsnj commented Jan 15, 2019

Uh oh!

yoshuawuyts Jan 15, 2019

Uh oh!

kimsnj Jan 15, 2019

Uh oh!

yoshuawuyts commented Jan 15, 2019

Uh oh!

aturon commented Apr 24, 2019

Uh oh!

kimsnj commented Apr 25, 2019

Uh oh!

pickfire Apr 26, 2019

Uh oh!

pickfire Apr 26, 2019

Uh oh!

pickfire Apr 26, 2019

Uh oh!

pickfire Apr 26, 2019 •

edited

Loading

Uh oh!

fairingrey commented Apr 26, 2019

Uh oh!

kimsnj commented Apr 28, 2019

Uh oh!

fairingrey commented Apr 28, 2019

Uh oh!

Uh oh!

	.filter_map(\|encoding\| Encoding::parse(encoding))
	.filter_map(Encoding::parse)

[WIP] New Tide middleware: Automatic compression handling #117

[WIP] New Tide middleware: Automatic compression handling #117

Uh oh!

Conversation

kimsnj commented Jan 15, 2019

Description

Missing features:

Streaming compression

Error handling

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Uh oh!

yoshuawuyts Jan 15, 2019

Choose a reason for hiding this comment

Uh oh!

kimsnj Jan 15, 2019

Choose a reason for hiding this comment

Uh oh!

yoshuawuyts commented Jan 15, 2019

Uh oh!

aturon commented Apr 24, 2019

Uh oh!

kimsnj commented Apr 25, 2019

Uh oh!

pickfire Apr 26, 2019

Choose a reason for hiding this comment

Uh oh!

pickfire Apr 26, 2019

Choose a reason for hiding this comment

Uh oh!

pickfire Apr 26, 2019

Choose a reason for hiding this comment

Uh oh!

pickfire Apr 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fairingrey commented Apr 26, 2019

Uh oh!

kimsnj commented Apr 28, 2019

Uh oh!

fairingrey commented Apr 28, 2019

Uh oh!

Uh oh!

pickfire Apr 26, 2019 •

edited

Loading