Asgi Websocket example not working on asgiref>=3.x

twillis commented 4 years ago

I'm guessing there has been some change to asgi since this was written, however I lack the experience to figure it out on my own and submit a pr.

considering the time this was merged my guess would be that the example is not compatible with asgi 3.0. hoping the author @erm might be able to offer some guidance on this.

https://docs.pylonsproject.org/projects/pyramid-cookbook/en/latest/deployment/asgi.html original submission: https://github.com/Pylons/pyramid_cookbook/pull/198

from asgiref.wsgi import WsgiToAsgi

from pyramid.config import Configurator
from pyramid.response import Response

class ExtendedWsgiToAsgi(WsgiToAsgi):

    """Extends the WsgiToAsgi wrapper to include an ASGI consumer protocol router"""

    def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)
        self.protocol_router = {"http": {}, "websocket": {}}

    def __call__(self, scope, **kwargs):
        protocol = scope["type"]
        path = scope["path"]
        try:
            consumer = self.protocol_router[protocol][path]
        except KeyError:
            consumer = None
        if consumer is not None:
            return consumer(scope)
        return super().__call__(scope, **kwargs)

    def route(self, rule, *args, **kwargs):
        try:
            protocol = kwargs["protocol"]
        except KeyError:
            raise Exception("You must define a protocol type for an ASGI handler")

        def _route(func):
            self.protocol_router[protocol][rule] = func

        return _route

HTML_BODY = """<!DOCTYPE html>
<html>
    <head>
        <title>ASGI WebSocket</title>
    </head>
    <body>
        <h1>ASGI WebSocket Demo</h1>
        <form action="" onsubmit="sendMessage(event)">
            <input type="text" id="messageText" autocomplete="off"/>
            <button>Send</button>
        </form>
        <ul id='messages'>
        </ul>
        <script>
            var ws = new WebSocket("ws://127.0.0.1:8000/ws");
            ws.onmessage = function(event) {
                var messages = document.getElementById('messages')
                var message = document.createElement('li')
                var content = document.createTextNode(event.data)
                message.appendChild(content)
                messages.appendChild(message)
            };
            function sendMessage(event) {
                var input = document.getElementById("messageText")
                ws.send(input.value)
                input.value = ''
                event.preventDefault()
            }
        </script>
    </body>
</html>
"""

# Define normal WSGI views
def hello_world(request):
    return Response(HTML_BODY)

# Configure a normal WSGI app then wrap it with WSGI -> ASGI class
with Configurator() as config:
    config.add_route("hello", "/")
    config.add_view(hello_world, route_name="hello")
    wsgi_app = config.make_wsgi_app()

app = ExtendedWsgiToAsgi(wsgi_app)

# Define ASGI consumers
@app.route("/ws", protocol="websocket")
def hello_websocket(scope):

    async def asgi_instance(receive, send):
        while True:
            message = await receive()
            if message["type"] == "websocket.connect":
                await send({"type": "websocket.accept"})
            if message["type"] == "websocket.receive":
                text = message.get("text")
                if text:
                    await send({"type": "websocket.send", "text": text})
                else:
                    await send({"type": "websocket.send", "bytes": message.get("bytes")})

    return asgi_instance

tom@tom-ThinkPad-P1:~/projects/navigator8_env$ pipenv run uvicorn websocket:app
Courtesy Notice: Pipenv found itself running within a virtual environment, so it will automatically use 
that environment, instead of creating its own for any project. You can set PIPENV_IGNORE_VIRTUALENVS=1 t
o force pipenv to ignore that environment and create its own instead. You can set PIPENV_VERBOSITY=-1 to
 suppress this warning.                                                                                
INFO:     Started server process [28427]
INFO:     Waiting for application startup.
INFO:     ASGI 'lifespan' protocol appears unsupported.
INFO:     Application startup complete.
INFO:     Uvicorn running on http://127.0.0.1:8000 (Press CTRL+C to quit)
ERROR:    Exception in ASGI application
Traceback (most recent call last):
  File "/home/tom/projects/navigator8_env/.venv/lib/python3.7/site-packages/uvicorn/protocols/http/httpt
ools_impl.py", line 385, in run_asgi                                                                   
    result = await app(self.scope, self.receive, self.send)
  File "/home/tom/projects/navigator8_env/.venv/lib/python3.7/site-packages/uvicorn/middleware/proxy_hea
ders.py", line 45, in __call__                                                                         
    return await self.app(scope, receive, send)
  File "/home/tom/projects/navigator8_env/.venv/lib/python3.7/site-packages/uvicorn/middleware/asgi2.py"
, line 6, in __call__                                                                                  
    instance = self.app(scope)
  File "./websocket.py", line 24, in __call__
    return super().__call__(scope, **kwargs)
TypeError: __call__() missing 2 required positional arguments: 'receive' and 'send'
INFO:     127.0.0.1:59952 - "GET / HTTP/1.1" 500 Internal Server Error

pinning asgiref==2.3.2 fixes things.

IftachSadeh commented 4 years ago

In addition to the issue of the asgiref upgrade, there is also an infinite loop upon disconnect:

    async def asgi_instance(receive, send):
        while True:
            message = await receive()
            if message["type"] == "websocket.connect":
                ...
            if message["type"] == "websocket.receive":
                ...

should be changed to:

    async def asgi_instance(receive, send):
        while True:
            message = await receive()
            if message["type"] == "websocket.connect":
                ...
            elif message["type"] == "websocket.receive":
                ...
            elif message["type"] == "websocket.disconnect":
                break

stevepiercy commented 4 years ago

Apologies for missing @twillis's PR. @IftachSadeh thank you for the review! I made the change, and pushed it to twillis's branch. Please let me know if the updated PR looks good to y'all and I will merge it.

IftachSadeh commented 4 years ago

Thanks @stevepiercy . The example runs well now. One small caveat - on leaving the page, one gets an exception:

ERROR:    Exception in ASGI application
Traceback (most recent call last):
  File "venv/lib/python3.7/site-packages/uvicorn/protocols/websockets/websockets_impl.py", line 153, in run_asgi
    result = await self.app(self.scope, self.asgi_receive, self.asgi_send)
  File "venv/lib/python3.7/site-packages/uvicorn/middleware/proxy_headers.py", line 45, in __call__
    return await self.app(scope, receive, send)
......
  File "venv/lib/python3.7/site-packages/asgiref/wsgi.py", line 35, in __call__
    raise ValueError("WSGI wrapper received a non-HTTP scope")
ValueError: WSGI wrapper received a non-HTTP scope

Is this the expected behaviour? If so, you can change to:

    async def __call__(self, scope, *args, **kwargs):
        protocol = scope["type"]
        path = scope["path"]
        try:
            consumer = self.protocol_router[protocol][path]
        except KeyError:
            consumer = None
        if consumer is not None:
            await consumer(scope, *args, **kwargs)
        try:
            await super().__call__(scope, *args, **kwargs)
        except ValueError as e:
            pass
        except Exception as e:
            raise e

stevepiercy commented 4 years ago

I don't know whether that behavior is expected or not, but putting a pass for an exception does not feel right. I'd look to the source code for the error message, then determine what would be more useful to return to the user as an error message than the default, specifically scope["type"]. Perhaps that gives you some more information on how to improve this bit?

IftachSadeh commented 4 years ago

On leaving the page the scope['type'] is websocket. Do you know why this is?

I'm not sure if there's any sense in taking some action. Maybe the following fix would be ok:

        try:
            if scope['type'] == 'http':
                await super().__call__(scope, *args, **kwargs)
        except Exception as e:
            raise e

stevepiercy commented 4 years ago

I am still new to ASGI, so I don't have much to offer as an explanation. I can read docs about ASGI and websocket but that doesn't mean I understand them. I think many of us are still trying to figure out best practices with ASGI. "Perfection is the enemy of progress", and this PR makes good progress. Therefore I'll push a comment into the PR, referring to this discussion, and merge it as is. That will give the recipe better public exposure and potential for continued improvement.

As a cookbook recipe, the chef may follow the essential instructions and add code to taste. If someone in the future comes up with a definitive improvement, then we can update the recipe.

stevepiercy commented 4 years ago

Closed by #226

Pylons / pyramid_cookbook

Asgi Websocket example not working on asgiref>=3.x #225