Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable Websocket compression for Netty #11169

Closed
wants to merge 0 commits into from

Conversation

TheAntimist
Copy link

@TheAntimist TheAntimist commented Feb 14, 2022

Pull Request Checklist

Helpful things

Fixes

Fixes #10863 and #4186

Purpose

Enable WebSocket Compression for Netty, using the WebSocketServerCompressionHandler. Adds the Netty 4.x WebSocketServerCompressionHandler to the chain of Netty's codecs.

Tests have been added to the netty-reactive-streams repository, as that seemed the correct place to add them. Let me know in case there's any concerns over this.

Tests

Added here: playframework/netty-reactive-streams#126

Background Context

This had to be added here, because if I added it to HttpStreamsServerHandler it would be too late for processing, and Compression would not get enabled. This is because it requires detecting of the Websocket Upgrade requests to enable Websocket Extension Headers.

References

From the Netty Websocket examples: https://github.com/netty/netty/blob/4.1/example/src/main/java/io/netty/example/http/websocketx/server/WebSocketServerInitializer.java#L47

@TheAntimist
Copy link
Author

@mkurz Hopefully this looks good, let me know in case of any changes or questions.

@mkurz
Copy link
Member

mkurz commented Feb 14, 2022

Thanks! The test in the netty-reactive-streams repo is broken.
Adding tests in the netty-reactive-streams repo however does not test this change here. I mean, it shows nicely which effect adding WebSocketServerCompressionHandler has, however not sure if it's worth also adding a scripted test here too, like http-backend-netty or http-backend-netty-channel-options.

@TheAntimist
Copy link
Author

The test in the netty-reactive-streams repo is broken.

Fixed, I was using Java 11, and missed the final requirement for the variables.

I mean, it shows nicely which effect adding WebSocketServerCompressionHandler has, however not sure if it's worth also adding a scripted test here too, like http-backend-netty or http-backend-netty-channel-options.

I agree. Honestly, I was not certain where this would be placed. I will add a test similar to the http-backend as it would provide a more holistic view.

@TheAntimist TheAntimist changed the title Enable Websocket compression. Fixes #10863 Enable Websocket compression Feb 14, 2022
@TheAntimist
Copy link
Author

FYI, got caught up in some work but the integration test is a WIP and should be done by this weekend.

@mkurz
Copy link
Member

mkurz commented Feb 23, 2022

No hurry, I am busy with other stuff this week as well 😉

@mkurz mkurz modified the milestone: 2.8.14 Mar 23, 2022
@mkurz
Copy link
Member

mkurz commented Sep 12, 2022

@TheAntimist are you still willing to work on this pull request?

@TheAntimist
Copy link
Author

@mkurz Apologies I got busy with some other personal things and also an internship in between. I cannot find the work that I had completed on it for some reason.

I can continue working on it now. I have a question on the IntegrationTest, would you want me to modify the controllers for the http-netty-backend here or do you want me to create one seperate integration test for just this?

@mkurz
Copy link
Member

mkurz commented Sep 12, 2022

You can make use of that existing scripted test. E.g. you can add new routes here and new action methods here, then you should be able to write the test(s) needed. Be aware that here we talk about scripted tests, so you need to run publishLocal before running them with scripted play-sbt-plugin/http-backend-netty. I also highly recommend to set (ThisBuild / version) := "2.9.0-SNAPSHOT" within the build.sbt file of your play repo clone (before running sbt). This way you makes sure each time you restart sbt and start running scripted ... again the locally publish artifact will be found (because right now the local published artifacts contain the date and time, like 2.8.1+1835-919b5bda+20220912-1743-SNAPSHOT, which of course would change as soon as you reload sbt).

Actually... As an alternative to the scripted test you could add a test here as well if you prefer that:
https://github.com/playframework/playframework/blob/main/core/play-integration-test/src/it/scala/play/it/http/websocket/WebSocketSpec.scala

@mkurz mkurz changed the title Enable Websocket compression Enable Websocket compression for Netty Oct 28, 2022
@mkurz mkurz closed this Mar 14, 2024
@mkurz
Copy link
Member

mkurz commented Mar 14, 2024

Could not re-open this pull request, so I had to create a new one:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enabling Compression to Websocket on Netty
2 participants