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

Refactor WebSocket support into separate sync/async implementations #206

Draft
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

dolfies
Copy link
Contributor

@dolfies dolfies commented Jan 4, 2024

Addresses #202 WebSocket requests

  • Separates async support into AsyncWebSocket
  • Removes Session.ws_connect() and allows initialization via WebSocket() constructor directly
    • This is because WebSockets need their own curl handle, as making a request will break the CurlOpt.CONNECT_ONLY value, so the session becomes useless if a WebSocket is created from it
    • This is not an issue with AsyncSession.connect() as it already maintains multiple curl handles
  • Changes AsyncWebSocket API to more closely match aiohttp
    • Removes sync-style callback handlers
    • Adds send/recv_str/json helper methods
  • Adds on_data callback to WebSocket
  • Adds autoclose and skip_utf8_validation parameters, and close_code and close_reason attributes
  • Adds iteration support

TODO:

  • Test and refine
  • Figure out why concurrent curl_ws_send() calls break unfortunately seems unsupported :(
  • Verify which curl opts are useful for WebSocket connections
  • Figure out how autoclose should behave with errors
  • Implement reconnection logic to run_forever()
  • Possibly wait for server to close connection before closing

@perklet
Copy link
Collaborator

perklet commented Jan 4, 2024

Removes Session.ws_connect() and allows initialization via WebSocket() constructor directly

aiohttp uses ws_connect.

@dolfies
Copy link
Contributor Author

dolfies commented Jan 4, 2024

aiohttp uses ws_connect.

AsyncSession still has ws_connect().
I removed it from sync Session since websocket-client uses the constructor and the curl handle can't be shared anyway.

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