Convert Sanic Request to WebSocket Request in websocket_handshake

[ChihweiLHBird]

Description

This PR is a (potentially naive) solution to fix an issue that missed WebSocket Upgrade request header during a WebSocket handshaking process causing 500 server error being returned.

After debugging, I found that the error stems from Sanic's Request object being passed into ws_proto.accept(...) method, which expects a request to be a WebSocket Request object. The issue is occurred when the websockets package is trying to assign an _exception value to the request object, resulting the attribute not existent error will be raised, which causes a 500 server error.

My solution is to convert Sanic Request object to be a WebSocket Request object in websocket_handshake method. I feel like this is a naive solution and am stilling digging into how a WebSocket requests is processed in Sanic's code, and I am also working on test cases.

May I have some feedback and comments?

Related issue

closes #2743

Code snippets for reproducing the issue

server

import sanic

from sanic import Request
from sanic.server.websockets.impl import WebsocketImplProtocol

app = sanic.Sanic("MySanic")

@app.websocket("/ws-echo")
async def ws_echo(request: Request, ws: WebsocketImplProtocol):
    message = "Start"
    while True:
        await ws.send(message)
        message = await ws.recv()


if __name__ == "__main__":
    app.run(auto_reload=True)

client

import http.client
import base64
import secrets
from urllib.parse import urlparse


def connect_websocket(url: str, message: str) -> None:
    parsed_url = urlparse(url)
    conn = http.client.HTTPConnection(parsed_url.netloc)

    websocket_key = base64.b64encode(secrets.token_bytes(16)).decode('utf-8')
    headers = {
        "Upgrade": "websocket",
        #"Connection": "Upgrade",
        "Sec-WebSocket-Key": websocket_key,
        "Sec-WebSocket-Version": "13"
    }

    conn.putrequest("GET", parsed_url.path)
    for header, value in headers.items():
        conn.putheader(header, value)
    conn.endheaders()

    response = conn.getresponse()
    if response.status == 101 and response.getheader('Upgrade', '').lower() == 'websocket':
        print("WebSocket handshake successful")
        conn.sock.sendall(message.encode())
    else:
        print(f"WebSocket handshake failed: {response.status} {response.reason}")

    conn.close()

websocket_url = "ws://localhost:8000/ws-echo"
connect_websocket(websocket_url, "Hello, WebSocket Server!")

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (0663f11) 88.188% compared to head (5c79652) 88.254%.

Additional details and impacted files

@@              Coverage Diff              @@
##              main     #2858       +/-   ##
=============================================
+ Coverage   88.188%   88.254%   +0.066%     
=============================================
  Files           93        93               
  Lines         7298      7305        +7     
  Branches      1254      1254               
=============================================
+ Hits          6436      6447       +11     
+ Misses         601       599        -2     
+ Partials       261       259        -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ChihweiLHBird]

Echo my thought in the issue thread:

Another way I am thinking about is to build WebSocket request object instead of Sanic Request in http protocol if the WebSocket handler is being used. This seems to come with a better performance, but much more complicated and might not be very feasible since we assume request is a Sanic Request object everywhere in the code base. I am still exploring. I would also like to hear feedback and comments about this idea.

[ahopkins]

since we assume request is a Sanic Request object everywhere in the code base.

I do not think this is something we can change. Not only do we make this assumption, it is a contract we've established and changing that would break a lot of apps.