Skip to content

Fix dx serve proxying of websocket connections established from the browser #3895

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

Merged
merged 9 commits into from
May 7, 2025

Conversation

arvidfm
Copy link
Contributor

@arvidfm arvidfm commented Mar 20, 2025

This PR makes the following changes:

  • Check for the Upgrade header to detect websocket connections where the URI doesn't include the scheme
  • Replace redirect with explicit proxying of websocket connections

The explicit proxying is required as browsers (at least Firefox and Chrome) don't handle redirect responses. E.g. Chrome emits an error complaining about an invalid response.

This PR is somewhat of a proof of concept in its current state. I'm happy to make any necessary changes to conform to Dioxus's code style and general guidelines, and to discuss alternative approaches.

Fixes #3894

@arvidfm arvidfm requested a review from a team as a code owner March 20, 2025 23:58
@ealmloff ealmloff added bug Something isn't working cli Related to the dioxus-cli program labels Mar 21, 2025
@arvidfm arvidfm force-pushed the fix-dx-serve-websocket-proxying branch from ad90a06 to 9c8930e Compare March 24, 2025 16:57
@arvidfm
Copy link
Contributor Author

arvidfm commented Apr 29, 2025

@ealmloff Hi, just wondering if there's anything I need to/can do here to help move this along? If you're not interested in the change I'm happy to close the PR, just let me know whichever way.

Copy link
Member

@ealmloff ealmloff left a comment

Choose a reason for hiding this comment

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

@ealmloff Hi, just wondering if there's anything I need to/can do here to help move this along? If you're not interested in the change I'm happy to close the PR, just let me know whichever way.

Thanks for the ping. I think there were some conflicts with #3820. If you can fix those issues, this looks good to me once checks pass. I confirmed this works with axum websockets and with the server function websocket integration.

Requesting Jon's review on this as well since he wrote the original websocket proxy logic

@ealmloff ealmloff requested a review from jkelleyrtp April 29, 2025 22:54
@arvidfm
Copy link
Contributor Author

arvidfm commented Apr 29, 2025

I fixed the compilation errors following the Axum upgrade. Unfortunately even though Tungstenite and Axum now use the same Utf8Bytes type for its messages there doesn't seem to be an easy way to reuse the allocation since Axum doesn't expose the underlying Tungstenite Utf8Bytes value that it wraps. You can in principle do something like Bytes::from(axum_string).try_into().unwrap() but I was hesitant to introduce an unwrap() call even if it's guaranteed to succeed in this case.

Not that I expect it'll actually have a measurable impact on performance, it just feels a bit silly.

@arvidfm
Copy link
Contributor Author

arvidfm commented Apr 30, 2025

It's not entirely clear to me what the cause of the Playwright issues is, but I don't think it's related to these changes? From what I can tell digging through the logs it looks like the dioxus crate is failing to build for certain tests due to the specific combination of feature flags passed

@ealmloff
Copy link
Member

ealmloff commented May 1, 2025

The playwright test failures are not related. Playwright is failing on main. #4042 fixes some of the issues

@jkelleyrtp jkelleyrtp merged commit 8ca8b04 into DioxusLabs:main May 7, 2025
16 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cli Related to the dioxus-cli program
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't establish websocket connection between browser and server when using dx serve
3 participants