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

Extract websocket functionality that is common to both server and future client #2582

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

p12tic
Copy link
Contributor

@p12tic p12tic commented Dec 14, 2024

Currently websocket server embeds a lot of functionality that would be useful for websocket client. This PR extracts it into a form that is easier to reuse. The websocket client itself has been implemented in #2552.

This PR extracts parts of #2552 to make it easier to land.

@p12tic
Copy link
Contributor Author

p12tic commented Dec 14, 2024

cc @xemul This PR is part of #2552 and all your comments have been addressed.

I wasn't sure whether the "description of commits is not sufficiently clear" applied to any commits in this PR. Please let me know if any of them should be improved.

@p12tic
Copy link
Contributor Author

p12tic commented Dec 23, 2024

@xemul Just a friendly ping.

@p12tic p12tic force-pushed the websocket-server-extract-common branch 2 times, most recently from 4c3bfeb to f044136 Compare December 24, 2024 10:37
@p12tic
Copy link
Contributor Author

p12tic commented Jan 6, 2025

@xemul Just a friendly ping.

@p12tic
Copy link
Contributor Author

p12tic commented Jan 14, 2025

@xemul Just a friendly ping. As far as I'm aware this PR only needs merging, the review has already been done.

@p12tic p12tic force-pushed the websocket-server-extract-common branch from f044136 to f3becb2 Compare January 14, 2025 00:23
@p12tic
Copy link
Contributor Author

p12tic commented Jan 14, 2025

Rebased on top of latest master.

@xemul
Copy link
Contributor

xemul commented Jan 15, 2025

Still conflicts

@p12tic p12tic force-pushed the websocket-server-extract-common branch 2 times, most recently from 5f5c9ba to 19da320 Compare January 15, 2025 15:30
@p12tic
Copy link
Contributor Author

p12tic commented Jan 15, 2025

Rebased again. The conflict was due to another PR merged today (mentioning just to say I'm not wasting your time :) ).

The tests most likely fail due to 27f834e. I created a PR to revert the commit in case it's useful: #2618.

@p12tic p12tic force-pushed the websocket-server-extract-common branch from 19da320 to 74c8730 Compare January 16, 2025 10:12
@p12tic
Copy link
Contributor Author

p12tic commented Jan 16, 2025

@xemul Rebased again, should be good to merge. Thanks!

This will allow to implement Websocket client that reuses existing code.
Current implementation of websocket connection is specific to the
server. Rename to more appropriate name.
Websocket connection handling is mostly not specific to the server and
thus can be reused on the client easily.
Previous name was useful when it's used locally in translation unit. The
new name is exposed in a header and should be more unique.
This function will be useful on websocket client to construct
Sec-WebSocket-Key header.
@p12tic p12tic force-pushed the websocket-server-extract-common branch from 74c8730 to ba4d044 Compare January 17, 2025 10:01
@p12tic
Copy link
Contributor Author

p12tic commented Jan 17, 2025

Rebased again.

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.

2 participants