Improve user feedback if no ws library installed #926
Conversation
uvicorn/protocols/websockets/auto.py
Outdated
| try: | ||
| import websockets # noqa | ||
| except ImportError: # pragma: no cover | ||
| try: | ||
| import wsproto # noqa | ||
| except ImportError: | ||
| AutoWebSocketsProtocol = None | ||
| logger = logging.getLogger("uvicorn.error") | ||
| logger.warning( |
There was a problem hiding this comment.
Doesn't this warning always trigger when using a minimal install?
I believe the default WS mode is "auto", meaning that we import this "auto protocol", which will be None and trigger this warning by default...
I'm actually not sure if/how we can check for "your app is using WebSocket, but dependencies aren't available" before a WebSocket request comes in.
So what we can do instead is log this warning when the first WebSocket connection request is received. Maybe that means we do need to define the "auto" protocol in this case as a "null protocol" class that logs this warning? I'm not sure how this None protocol is used by asyncio.
There was a problem hiding this comment.
yes you nailed it, the ws flag is on auto by default, so AutoWebSocketsProtocol will be None, and we will get a warning set here if we try a handshake:
uvicorn/uvicorn/protocols/http/h11_impl.py
Line 267 in fa914bc
which sucks.
There was a problem hiding this comment.
So what we can do instead is log this warning when the first WebSocket connection request is received. Maybe that means we do need to define the "auto" protocol in this case as a "null protocol" class that logs this warning? I'm not sure how this None protocol is used by asyncio.
like in the config.load() ? if ws_protocol_class is None > warning ?
There was a problem hiding this comment.
the current scenario is this:
- uvicorn minimal > auto ws > ws=None > auto http > h11 > handshake in h11 >
msg = "Unsupported upgrade request."
The present PR is preemptively raising a warning:
- uvicorn minimal > auto ws > ws=None >
Warning blablabla ws cant be used
We could also add this warning to the existing h11 warning:
- uvicorn minimal > auto ws > ws=None > auto http > h11 > handshake in h11 >
msg = "Unsupported upgrade request. Warning blablabla ws cant be used"
I just find it more deceptive when you want to use ws, but you'll have a useful info in both cases.
It's also true raising a warning for everyone when they dont want to use ws is probably annoying.
I have no preference, I let you choose ! :)
There was a problem hiding this comment.
ok after trying to put the warning in the httptools_impl.py I find it weird.
the scenario is very deceptive for a user, and on top of that for a regular user using ws and having a real upgrade issue he'll have the warning, so I prefer to put it here in the import logic
|
@euri10 I think you put it right in the earlier comments: Between "somewhat deceptive message only when receiving a WS request w/o a WS library" and "raise a message for all minimal users", I think the latter is less acceptable. Should we update the "Unsupported upgrade request" warning instead? Perhaps do a quick runtime check on libraries and add an extra warning hint? |
…thout ws libraries
I had not thought about importing locally the |
| @@ -266,6 +266,11 @@ def handle_upgrade(self, event): | |||
| if upgrade_value != b"websocket" or self.ws_protocol_class is None: | |||
| msg = "Unsupported upgrade request." | |||
| self.logger.warning(msg) | |||
| from uvicorn.protocols.websockets.auto import AutoWebSocketsProtocol | |||
There was a problem hiding this comment.
(Nit) Let's add empty lines around this block so it's clearer that it's related but separate from the rest of the error handling code?
Co-authored-by: Florimond Manca <[email protected]>
Fixes #797