Skip to content

[dsrouter] fix web socket startup#5121

Merged
mdh1418 merged 5 commits into
dotnet:mainfrom
pavelsavara:fix_ws
Mar 27, 2025
Merged

[dsrouter] fix web socket startup#5121
mdh1418 merged 5 commits into
dotnet:mainfrom
pavelsavara:fix_ws

Conversation

@pavelsavara

@pavelsavara pavelsavara commented Dec 18, 2024

Copy link
Copy Markdown
Member
  • make the default value valid ws://127.0.0.1:8088/diagnostics
  • fix parsing the URL, so that HTTP server could be started based on ws:// or wss:// as it only accepts http:// or https:// schema

Contributes to dotnet/runtime#76316

@pavelsavara

Copy link
Copy Markdown
Member Author

cc @lateralusX

Comment thread src/Tools/dotnet-dsrouter/Program.cs Outdated
@pavelsavara pavelsavara marked this pull request as ready for review March 25, 2025 14:46
@pavelsavara pavelsavara requested a review from a team as a code owner March 25, 2025 14:46
@pavelsavara pavelsavara requested a review from lateralusX March 26, 2025 09:20
Comment thread src/Tools/dotnet-dsrouter/Program.cs
@pavelsavara pavelsavara requested a review from lateralusX March 27, 2025 11:07
lateralusX
lateralusX previously approved these changes Mar 27, 2025

@lateralusX lateralusX left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM!

Comment thread src/Microsoft.Diagnostics.WebSocketServer/WebSocketServerImpl.cs Outdated
@mdh1418

mdh1418 commented Mar 27, 2025

Copy link
Copy Markdown
Member

The original PR that added the websocket options #3461 and the issue it was targeting #3422 seems to suggest that the ws:// URL worked, without converting the scheme to http/https. Was that not actually the case?

@pavelsavara

pavelsavara commented Mar 27, 2025

Copy link
Copy Markdown
Member Author

The original PR that added the websocket options #3461 and the issue it was targeting #3422 seems to suggest that the ws:// URL worked, without converting the scheme to http/https. Was that not actually the case?

It probably worked if you passed http or https to the -ws argument. But that's not web socket URI and it's not the same as you pass into DOTNET_DiagnosticPorts. The original EP attempt was not ready for production.

@mdh1418

mdh1418 commented Mar 27, 2025

Copy link
Copy Markdown
Member

And for clarification, its not a conflicting user experience to pass in ws wss, but the code actually creates a webserver using a webHost at http https instead?

Is there a blocker to make ws and wss scheme actually work?

@mdh1418 mdh1418 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me if the ws/wss -> http/https would be expected/okay from a user standpoint and if the end-to-end scenario can be tested (I'm assuming there's no automated tests for ws/wss since it turned out it doesn't work)

@pavelsavara

Copy link
Copy Markdown
Member Author

Looks good to me if the ws/wss -> http/https would be expected/okay from a user standpoint

WS of built on top of HTTP, it's protocol upgrade scheme. You need HTTP server to be able to handle WS endpoints.

end-to-end scenario can be tested

I'm end to end testing manually with dotnet/runtime#110818
Eventually there would be some automated test, probably next PR.

@mdh1418 could you please click merge on my behalf ? I don't have the permissions on this repo.

Also when it's the next release of these tools ?

@mdh1418 mdh1418 merged commit 58da378 into dotnet:main Mar 27, 2025
@mdh1418

mdh1418 commented Mar 27, 2025

Copy link
Copy Markdown
Member

@mikem8361 @hoyosjs any estimates for when the next release might be?

@hoyosjs

hoyosjs commented Mar 27, 2025

Copy link
Copy Markdown
Member

This just missed the next train. And it's likely to be a bit before an official release

@github-actions github-actions Bot locked and limited conversation to collaborators Apr 27, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants