fix #970 Ensure WebSocket compression is enabled on the server in cases there is a compressionPredicate

[violetagg]

No description provided.

[amizurov]

@violetagg thx for reactive fix and in common case it works, but if we change configuration to

HttpServer server =
           HttpServer.create()
               .port(8080)
               .compress(false)
               .compress(2048)
               .wiretap(true)
               .route(r -> r.ws("/ws", (in, out) -> out.sendObject(in.receiveFrames().doOnNext(WebSocketFrame::retain))));

the WebSocket compression is applied because predicate present, looks unobvious. WDYT ?

[violetagg]

@violetagg thx for reactive fix and in common case it works, but if we change configuration to

HttpServer server =
           HttpServer.create()
               .port(8080)
               .compress(false)
               .compress(2048)
               .wiretap(true)
               .route(r -> r.ws("/ws", (in, out) -> out.sendObject(in.receiveFrames().doOnNext(WebSocketFrame::retain))));

the WebSocket compression is applied because predicate present, looks unobvious. WDYT ?

There is kind of misunderstanding here
.compress(true) means compress always
then you override this setting with
.compress(2048) which mean stop compressing always (because this was the previous setting) and start compressing when this condition is met.

then the second example is
.compress(false) means DO NOT compress
then you override this setting with
.compress(2048) which mean start compressing when this condition is met.

https://projectreactor.io/docs/netty/release/reference/index.html#_compression

[amizurov]

@violetagg thx for reactive fix and in common case it works, but if we change configuration to

HttpServer server =
           HttpServer.create()
               .port(8080)
               .compress(false)
               .compress(2048)
               .wiretap(true)
               .route(r -> r.ws("/ws", (in, out) -> out.sendObject(in.receiveFrames().doOnNext(WebSocketFrame::retain))));

the WebSocket compression is applied because predicate present, looks unobvious. WDYT ?

There is kind of misunderstanding here
.compress(true) means compress always
then you override this setting with
.compress(2048) which mean stop compressing always (because this was the previous setting) and start compressing when this condition is met.

then the second example is
.compress(false) means DO NOT compress
then you override this setting with
.compress(2048) which mean start compressing when this condition is met.

Yes i understand this, but compression for http response and webSocket compression is not depend to each other, also i can use simultaneously ws and http and i want to configure each of them independently IMHO.

[violetagg]

Yes i understand this, but compression for http response and webSocket compression is not depend to each other, also i can use simultaneously ws and http and i want to configure each of them independently IMHO.

We do not support that. If you want I can revert this change and we can treat your issue as a feature request.
wdyt?

[amizurov]

Yes i understand this, but compression for http response and webSocket compression is not depend to each other, also i can use simultaneously ws and http and i want to configure each of them independently IMHO.

We do not support that. If you want I can revert this change and we can treat your issue as a feature request.
wdyt?

Thx again for your time. I'm ok with this PR but we change behavior for org.springframework.boot.web.embedded.netty.NettyWebServer because before it websocket compression never work :). I agree that is a good idea to create separate feature request where we can improve current logic and clear api. Also we can improve websocket compression to selective approach (Netty suported this https://github.com/netty/netty/pull/8910/files/6bdf7ad8bf6c6f23111671fbaba7403ab38c53fe) and apply only to websocket frames by predicate like http does.

[violetagg]

Thx again for your time. I'm ok with this PR but we change behavior for org.springframework.boot.web.embedded.netty.NettyWebServer because before it websocket compression never work :). I agree that is a good idea to create separate feature request where we can improve current logic and clear api. Also we can improve websocket compression to selective approach (Netty suported this https://github.com/netty/netty/pull/8910/files/6bdf7ad8bf6c6f23111671fbaba7403ab38c53fe) and apply only to websocket frames by predicate like http does.

@rstoyanchev wdyt? My opinion I that we should separate both configurations.

[rstoyanchev]

Yes I think it would be a good idea to separate the two. Unless only compress(true) is used, the presence of any other compress predicate (size or custom), since it can't be evaluated properly, that makes it ambiguous whether it should even apply to WebSocket.

[violetagg]

I'm closing this PR - we will NOT apply this fix.

There will be a WebSocket configuration for compression on/off.
For the WebSocketExtensionFilterProvider support I created a separate feature request #977

[violetagg]

new PR #980