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

Prevent websocket upgrade from replying with content-length: 0 #139

Merged
merged 1 commit into from
Nov 14, 2023
Merged

Prevent websocket upgrade from replying with content-length: 0 #139

merged 1 commit into from
Nov 14, 2023

Conversation

chonton
Copy link
Contributor

@chonton chonton commented Nov 13, 2023

Fixes quarkusio/quarkus #370505

Additionally, using the native vertx implementation increases the number of websocket clients and versions supported.

ChannelHandlerContext context = connection.channelHandlerContext();
context.pipeline().remove("httpDecoder");
context.pipeline().remove("httpEncoder");
request.toWebSocket().onSuccess( ws -> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid lambda please.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@cescoffier
Copy link
Member

Would be great to have a test case.

This comment has been minimized.

@chonton
Copy link
Contributor Author

chonton commented Nov 13, 2023

test case is in quarkus integration tests. please note that there currently aren't any tests for the vertx code

and junit 4, yuck. I'd be happy to create an incremental unit test, but bootstrapping all the vertx cases is a heavy lift.

@cescoffier
Copy link
Member

Ah yes... I forgot that this project still use junit 4.

Copy link

quarkus-bot bot commented Nov 14, 2023

✔️ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

@cescoffier cescoffier merged commit 805bb3f into quarkusio:main Nov 14, 2023
3 checks passed
@cescoffier
Copy link
Member

@chonton I will revert this PR, unfortunately.
It is breaking sub-protocols (see quarkusio/quarkus#39082). I'm not totally sure why. Let's revert and investigate.

@michaelfrid
Copy link

@chonton @cescoffier
Hello. We just encounter this issue in Quarkus 3,2,X and 3.15.X. It causes our .Net clients to close websocket connection after some period of time. Are there any plans to fix it ?
Thanks a lot

@cescoffier
Copy link
Member

Did you try with WebSocket Next?

@cescoffier
Copy link
Member

(We reverted the fix as it introduced a regression)

@michaelfrid
Copy link

michaelfrid commented Jan 9, 2025

@cescoffier
I see it's marked as experimental feature. Is it stable enough to be used in the production environment ?
It also means we need to rewrite our current implementation, right ?
Thank you

@cescoffier
Copy link
Member

cescoffier commented Jan 9, 2025 via email

@michaelfrid
Copy link

@cescoffier
So what should we do for meanwhile ? Any chances the original issue will be fixed ?

@cescoffier
Copy link
Member

Sorry, it is quite mature :-)
I would recommend switching.

I don't believe this issue will be fixed anytime soon. At least I do not have the bandwidth.

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.

Content-Length should not be added to 1xx responses
4 participants