fix: upgrade is not websocket and dependencies are installed, should not warning

[vvanglro]

Summary

When I upgraded uvicorn to standard, I found that I encountered a lot of Unsupported upgrade request. prompts in the logs, so I debugged locally and found that h11 and httptools handle upgrades with different logic. I think I should keep the same logic.
If the upgrade field is not equal to websocket in h11, the self._should_upgrade_to_ws() method will not run, while httptools is different.

Checklist

• I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
• I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
• I've updated the documentation accordingly.

[vvanglro]

@Kludex Hey, would you look at that?

[Kludex]

Can you create a table of expected behavior you see?

Also, while checking this... I've noticed that this is printed multiple times with httptools:

WARNING:  Unsupported upgrade request.
WARNING:  No supported WebSocket library detected. Please use "pip install 'uvicorn[standard]'", or install 'websockets' or 'wsproto' manually.
WARNING:  Unsupported upgrade request.
WARNING:  No supported WebSocket library detected. Please use "pip install 'uvicorn[standard]'", or install 'websockets' or 'wsproto' manually.
WARNING:  Unsupported upgrade request.
WARNING:  No supported WebSocket library detected. Please use "pip install 'uvicorn[standard]'", or install 'websockets' or 'wsproto' manually.

Does this also happens for you?

[vvanglro]

I have created a table to describe the expected behavior and the actual behavior I observed:

Protocol	Expected Behavior	Actual Behavior
h11	Ignore upgrade requests not equal to websocket	Works as expected, no warnings
httptools	Ignore upgrade requests not equal to websocket	Multiple warnings: "Unsupported upgrade request"

yes, I also encountered the warnings multiple times with httptools.

[Kludex]

Is this PR fixing the multiple warnings? I tried it, and it didn't change that. I may have done something wrong?

[vvanglro]

How did you test it?
Run the code below without this PR, then you will see multiple warnings when httptools is selected.

from fastapi import FastAPI

app = FastAPI()


@app.get("/")
async def root():
    return {"message": "Hello, World!"}


if __name__ == '__main__':
    import uvicorn
    # uvicorn.run(app, http="h11")
    uvicorn.run(app, http="httptools")

import httpx


async def a():
    headers = {"Connection": "upgrade", "Upgrade": "123"}
    async with httpx.AsyncClient() as client:
        response = await client.get('http://0.0.0.0:8000', headers=headers)
        print(response.text)


import asyncio
asyncio.run(a())

[Kludex]

I'll get back to you in some hours. I'm away.

[Kludex]

I'm using wscat.

I'd like to see this working against:

from starlette.applications import Starlette
from starlette.routing import WebSocketRoute
from starlette.websockets import WebSocket


async def route(websocket: WebSocket) -> None:
    await websocket.accept()
    await websocket.send_text("Hello, world!")
    await websocket.close()


app = Starlette(routes=[WebSocketRoute("/ws", route)])

Running with uvicorn main:app --http httptools.

[Kludex]

Ah, got it.

The _should_upgrade is called multiple times in the httptools one.

[vvanglro]

If you are using the standard ws client to request it, no warning will be generated.
If you use http and bring the upgrade upgrade protocol field in the header. Then a warning will be generated.

[vvanglro]

The biggest difference between h11 and httptools is that h11 strictly determines that upgrade is equal to websocket, whereas httptools does not, which leads to warnings when upgrade is not a websocket.

[Kludex]

Are you saying that your PR just doesn't solve the multiple warnings on the scenario which should really warn?

[vvanglro]

There seems to be two issues now, there should only be one warning prompt if a user receives a request to upgrade ws when they have not chosen to install a websocket dependency.

When a user installs a websocket dependency and receives a request with an incorrect value in the upgrade field, multiple warnings are generated, which is incorrect because I already have the dependency installed, this PR is to fix that.

[vvanglro]

Only one warning is generated now.

And the warnings are friendly, if the dependency is installed and the upgrade is not supported, then only prompted the upgrade is not supported. If the dependency is not installed and the upgrade is not supported, then both are prompted.

[urec56]

Hey. What the solution of this problem now, guys? I have it on my fastapi app for some time.

And it starts like

CMD ["gunicorn", "app.main:app", "--workers", "1", "--worker-class", "uvicorn.workers.UvicornWorker", \
 	"--bind=0.0.0.0:8000", "--max-requests", "100", "--max-requests-jitter", "50"]

My libs:

h11==0.14.0
httptools==0.6.1
uvicorn==0.26.0
uvloop==0.19.0
websockets==12.0

[vvanglro]

@urec56 This comment is the solution to this PR: #2360 (comment)

If you can reproduce the problem, you can try installing this PR to verify that it solves the problem.

pip install git+https://github.com/vvanglro/uvicorn.git

The expected result is that you will only receive: [WARNING] Unsupported upgrade request.

[urec56]

Ok, I'll try it with this fix and reply here in a few days

[urec56]

So, I tried this fix and it looks like it solved the problem. Thank you, guys

[Kludex]

I'll check this over the weekend. Sorry the delay.

[vvanglro]

@Kludex Maybe there should be a timeout for the test action?

jobs:
  tests:
    name: "Python ${{ matrix.python-version }} ${{ matrix.os }}"
    runs-on: "${{ matrix.os }}"
+   timeout-minutes: 30

[Kludex]

Yes. Some process is hanging. Do you want to create a PR for it?

[vvanglro]

Yes. Some process is hanging. Do you want to create a PR for it?

I'll do.

[Kludex]

Thanks @vvanglro 🙏

[urec56]

In what release of uvicorn we can download this fix?

[Kludex]

In what release of uvicorn we can download this fix?

0.30.6

[urec56]

Thank you