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

WebsocketConnection does not send subprotocol in accept #1878

Closed
darkvariantdivine opened this issue Jun 24, 2020 · 2 comments
Closed

WebsocketConnection does not send subprotocol in accept #1878

darkvariantdivine opened this issue Jun 24, 2020 · 2 comments

Comments

@darkvariantdivine
Copy link
Contributor

darkvariantdivine commented Jun 24, 2020

WebsocketConnection does not send subprotocols during accept
Client sent graphql-ws subprotocol during websocket handshake, Sanic replied with an empty string instead of populating data extracted from scope

Code snippet

class WebSocketConnection:

    # TODO
    # - Implement ping/pong

    def __init__(
        self,
        send: Callable[[ASIMessage], Awaitable[None]],
        receive: Callable[[], Awaitable[ASIMessage]],
    ) -> None:
        self._send = send
        self._receive = receive

    ...

    async def accept(self) -> None:
        await self._send({"type": "websocket.accept", "subprotocol": ""})

In the accept function, the subprotocol field is hardcoded as an empty string

Expected behavior
Allow the subprotocol field to be populated from an ASGI client or external scope

Environment (please complete the following information):

  • OS: Fedora 30
  • Python: 3.7.7
  • Sanic: 19.12.2, 20.3.0

Additional context
Sanic is a simple and elegant framework, I would love to help out with a PR but will need some direction. :)

@ahopkins
Copy link
Member

ahopkins commented Jun 28, 2020

You are running ASGI server? Websockets in ASGI mode do still need some work. I believe when I added ASGI support, they were not yet supported in the ASGI spec. It looks like they are there now, so it should be a relatively small PR if you would like to give it a shot.

Without looking too deeply at this right now, I believe you need to grab scope['subprotocols'] in here:

https://github.com/huge-success/sanic/blob/master/sanic/asgi.py#L236

and then ultimately pass that to WebSocketConnection(..., subprotocols=subprotocols) and then update the referenced code you pasted above accordingly.

@darkvariantdivine
Copy link
Contributor Author

@ahopkins thanks for the guidance, I'll be following up with a PR on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants