Support compression for reactive routes and resteasy reactive #24558

mkouba · 2022-03-25T14:31:50Z

Functionality:

nothing is compressed by default
if quarkus.http.enable-compression=true then an HTTP response body of a static resource, reactive route and resteasy-reactive resource method is compressed if:
- the method is annotated with @io.quarkus.vertx.http.Compressed, OR
- the content-type header is set and the value is listed in quarkus.http.compress-media-types (text/html,text/plain,text/xml,text/css,text/javascript,application/javascript by default).
a reactive route or resource method is never compressed if annotated with @io.quarkus.vertx.http.Uncompressed

Impl. notes:

if quarkus.http.enable-compression=true then every response has the Content-Encoding: identity header set by default; this effectively disables the compression
static resources, reactive routes and resteasy-reactive remove this header to enable the compression (rules described above)
resteasy-reactive removes the header in a special ServerRestHandler that is added to the chain for any resource method unless @Uncompressed is used
reactive routes use an end handler to remove the header if needed, unless @Uncompressed is used

Note that it's a breaking change in the sense that previously quarkus.http.enable-compression=true enabled compression for every HTTP response which is not the case anymore. The configuration property is effectively ignored for reasteasy-classic endpoints and routes registered manually.

mkouba · 2022-03-25T14:33:21Z

What's missing:

more tests
docs
optimizations?

cescoffier

LGTM! I just had a few questions, but it looks great (exactly what I had in mind).

What I like is that it will ease the integration of other compression mechanism once they will be supported by the engine.

cescoffier · 2022-03-27T07:12:14Z

...nsions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/HttpConfiguration.java

+     * List of media types for which the compression should be enabled automatically, unless declared explicitly via {@link Compressed} or {@link Uncompressed}. 
+     */
+    @ConfigItem(defaultValue = "text/html,text/plain,text/xml,text/css,text/javascript,application/javascript")
+    public Optional<List<String>> compressMediaTypes;


Are this list also used when you return a response from resteasy reactive?

Yes, it should be. Although it's not tested yet. It's however not used for resteasy classic.

It's tested now.

I wonder if we should honor this config property for reasteasy-classic or ignore it completely? Currently, reasteasy-classic has a separate config and annotation.

For now this config property and enableCompression is just ignored, i.e. it has no effect on http compression of resteasy classic endpoints.

cescoffier · 2022-03-27T07:13:19Z

...nsions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/HttpConfiguration.java

+    /**
+     * The compression level used when compression support is enabled.
+     */
+    public OptionalInt compressionLevel;


Should we have both enableCompression and compressionLevel? Should we infer "enable" if the level is set?

Hm, I don't think it's convenient to say: use quarkus.http.compression-level=6 to enable compression with the default compression level. In other words, enable=true should work in most cases and is simple and expressive.

cescoffier · 2022-03-27T07:17:52Z

...erver/runtime/src/main/java/org/jboss/resteasy/reactive/server/spi/RuntimeConfiguration.java

@@ -12,6 +13,10 @@

    Limits limits();

+    boolean enableCompression();
+
+    Set<String> compressMediaTypes();


shouldn't it be compressed?

Well, we have HttpConfiguration.compressMediaTypes so I just wanted to keep this aligned. HttpConfiguration.compressedMediaTypes is ok though. Should I rename both?

FTR I used the verb (compress) to keep consistency with enableCompression..

...loyment/src/test/java/io/quarkus/resteasy/reactive/server/test/compress/CompressionTest.java

docs/src/main/asciidoc/resteasy.adoc

cescoffier · 2022-03-29T06:11:57Z

...active-routes/runtime/src/main/java/io/quarkus/vertx/web/runtime/HttpCompressionHandler.java

+
+    @Override
+    public void handle(RoutingContext context) {
+        context.addEndHandler(new Handler<AsyncResult<Void>>() {


To avoid that interception, we could imagine moving that code directly to the HTTP Server "root" request handler. Just an idea if we see a large decrease in terms of performance.

Yeah, so this handler should only be used if compression is enabled and I think that there will be other much more expensive calls in such case ;-)

mkouba · 2022-03-29T08:14:21Z

I believe that this PR is ready for review.

Note that it's a breaking change in the sense that previously quarkus.http.enable-compression=true enabled compression for every HTTP response which is not the case anymore. The configuration property is effectively ignored for reasteasy-classic endpoints and routes registered manually.

geoand

LGTM

mkouba · 2022-03-29T09:12:52Z

The DevConsoleConfigEditorBodyHandlerTest.testChangeHttpRoute fails with java.lang.NoSuchMethodException: java.util.OptionalInt.<init>(). It seems to be a bug in io.quarkus.runtime.configuration.ConfigInstantiator.

mkouba · 2022-03-29T12:34:51Z

The DevConsoleConfigEditorBodyHandlerTest.testChangeHttpRoute fails with java.lang.NoSuchMethodException: java.util.OptionalInt.<init>(). It seems to be a bug in io.quarkus.runtime.configuration.ConfigInstantiator.

Ah, HttpConfiguration#compressionLevel is missing the ConfigItem annotation!

- resolves quarkusio#16425

mkouba · 2022-03-29T12:55:50Z

@FroMage @gsmet could you pls take a look at the functionality description and share your opinion? ;-)

gsmet · 2022-03-29T13:50:31Z

Sorry I was busy on other stuff.
I think it makes perfect sense and this is really great.

TBH, I'm on the verge of including it for 2.8.0.Final if we get it merged before this evening. But maybe we can backport it later once people ask for it.

mkouba · 2022-03-29T14:05:46Z

Sorry I was busy on other stuff. I think it makes perfect sense and this is really great.

No problem. I was just curious if it meets your expectations.

But maybe we can backport it later once people ask for it.

+1

geoand · 2022-03-29T14:06:44Z

But maybe we can backport it later once people ask for it.

👍🏼

cescoffier · 2022-03-29T17:25:52Z

It's great!

FroMage · 2022-03-31T12:43:04Z

@FroMage @gsmet could you pls take a look at the #24558 (comment) and share your opinion? ;-)

That's just awesome, bravo!

geoand · 2022-04-15T11:22:28Z

Coming back to this: Is there a good reason for making compression a runtime property?
If it were a build time property, we could not add the handler at all (which would be a tiny boost for performance as there would be one less call-site and one less volatile field read)

cescoffier · 2022-04-15T14:03:25Z

@geoand speaking a bit about the future. At the moment, we only support one compression algorithm. This "set" will soon be expanded, and the other algorithms may (very likely) require native bits. We would need to pre-package the native bits (user config, of course). Following that logic, I would say that making it a build time property would make sense. If compression is disabled, it will avoid adding the native libs into the native executable and also tweak substitutions.

geoand · 2022-04-15T14:05:51Z

I agree. If @mkouba also agrees, I'll fix it and hopefully get the change in 2.8.1

gsmet · 2022-04-15T14:09:28Z

Yeah it seems more reasonable to me too.

For now I haven’t backported this work. But we could backport it if we all agree it’s worth it.

geoand · 2022-04-17T05:15:20Z

Hm... I see that quarkus.http.enable-compression was not added by this PR, but has existed (as a runtime) property for a while...
So if we move it to a build time property, it will be a breaking change. Do we still want to do that (I am personally +1)?

cescoffier · 2022-04-17T06:19:50Z

Yes, +1 too. As said, that change would happen eventually when we will be able to integrate Vert.x 4.3.

geoand · 2022-04-18T07:58:28Z

#24984 is the PR moving the configuration to build time

mkouba · 2022-04-19T07:41:02Z

For now I haven’t backported this work. But we could backport it if we all agree it’s worth it.

I don't think it's a good idea to backport a breaking change.

Anyway, it probably makes sense to move the config property to the build time. Although it makes the config a bit inflexible, i.e. you'll need to rebuild your app in order to enable compression.

geoand · 2022-04-19T07:42:27Z

Anyway, it probably makes sense to move the config property to the build time. Although it makes the config a bit inflexible, i.e. you'll need to rebuild your app in order to enable compression.

Indeed, but what is the legitimate case for having differ between deployments of an artifact? I don't think there is one...

mkouba · 2022-04-19T07:59:22Z

Indeed, but what is the legitimate case for having differ between deployments of an artifact? I don't think there is one...

I'm not following ;-). What I'm talking about is the use case where you build an app and in one deployment you want to enable compression because the data sent over the network are expensive and in other deployment the CPU is expensive but you don't care about the number of bytes sents. Right now, you can use the same app and change the config property. After this change, you'll have to build two versions of the app.

geoand · 2022-04-19T08:04:10Z

We are saying the same thing :)

What I am asking is if there a real scenario were one would actually do that...

mkouba · 2022-04-19T08:08:42Z

What I am asking is if there a real scenario were one would actually do that...

I have no idea..

quarkus-bot bot added area/rest area/vertx labels Mar 25, 2022

mkouba mentioned this pull request Mar 25, 2022

RESTEasy Reactive - GZIP support #16425

Closed

mkouba requested review from cescoffier, FroMage, geoand and gsmet March 25, 2022 14:33

cescoffier requested changes Mar 27, 2022

View reviewed changes

geoand reviewed Mar 28, 2022

View reviewed changes

...loyment/src/test/java/io/quarkus/resteasy/reactive/server/test/compress/CompressionTest.java Show resolved Hide resolved

mkouba force-pushed the issue-16425 branch 2 times, most recently from 171a9ba to c38c2bd Compare March 28, 2022 11:25

quarkus-bot bot added the area/documentation label Mar 28, 2022

mkouba requested review from cescoffier and geoand March 28, 2022 11:55

geoand reviewed Mar 28, 2022

View reviewed changes

docs/src/main/asciidoc/resteasy.adoc Outdated Show resolved Hide resolved

cescoffier reviewed Mar 29, 2022

View reviewed changes

mkouba force-pushed the issue-16425 branch 2 times, most recently from 247e06b to ef05d35 Compare March 29, 2022 08:10

mkouba marked this pull request as ready for review March 29, 2022 08:14

mkouba added the release/breaking-change label Mar 29, 2022

mkouba requested review from cescoffier and geoand March 29, 2022 08:15

mkouba added this to the 2.9 - main milestone Mar 29, 2022

mkouba changed the title ~~WIP: Support compression for reactive routes and resteasy reactive~~ Support compression for reactive routes and resteasy reactive Mar 29, 2022

geoand approved these changes Mar 29, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

Support compression for reactive routes and resteasy reactive

670a205

- resolves quarkusio#16425

mkouba force-pushed the issue-16425 branch from ef05d35 to 670a205 Compare March 29, 2022 12:43

mkouba added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Mar 29, 2022

cescoffier approved these changes Mar 29, 2022

View reviewed changes

mkouba merged commit 1c9dbbd into quarkusio:main Mar 29, 2022

quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Mar 29, 2022

mkouba mentioned this pull request Mar 30, 2022

Documentation - HTTP compression of static resources #24622

Merged

gsmet added the release/noteworthy-feature label Apr 26, 2022

tnmtechnologies mentioned this pull request Jun 14, 2022

no more HTTP compression for resteasy-classic endpoints #26112

Closed

geoand mentioned this pull request Oct 18, 2022

WIP: Test gzip support in RESTEasy Reactive #16924

Closed

mkouba mentioned this pull request Feb 27, 2023

GZIP Compression not working in servlet application #31415

Closed

Support compression for reactive routes and resteasy reactive #24558

Support compression for reactive routes and resteasy reactive #24558

Conversation

mkouba commented Mar 25, 2022 • edited Loading

mkouba commented Mar 25, 2022 • edited Loading

cescoffier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkouba commented Mar 29, 2022

geoand left a comment

Choose a reason for hiding this comment

mkouba commented Mar 29, 2022

This comment has been minimized.

mkouba commented Mar 29, 2022 • edited Loading

mkouba commented Mar 29, 2022

gsmet commented Mar 29, 2022

mkouba commented Mar 29, 2022

geoand commented Mar 29, 2022

cescoffier commented Mar 29, 2022

FroMage commented Mar 31, 2022

geoand commented Apr 15, 2022

cescoffier commented Apr 15, 2022

geoand commented Apr 15, 2022

gsmet commented Apr 15, 2022

geoand commented Apr 17, 2022

cescoffier commented Apr 17, 2022

geoand commented Apr 18, 2022

mkouba commented Apr 19, 2022

geoand commented Apr 19, 2022

mkouba commented Apr 19, 2022

geoand commented Apr 19, 2022

mkouba commented Apr 19, 2022

mkouba commented Mar 25, 2022 •

edited

Loading

mkouba commented Mar 25, 2022 •

edited

Loading

mkouba commented Mar 29, 2022 •

edited

Loading