search

python-websockets / websockets

Library for building WebSocket servers and clients in Python
https://websockets.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
5.18k stars 515 forks source link

uvicorn depends on the legacy server implementation #975

Open euri10 opened 3 years ago

euri10 commented 3 years ago

greetings and thanks for the great library !

I'd have a question for you on what you think would be the best way to proceed for us in uvicorn : we recently unpinned websockets from websockets==8.* to websockets>=8.* which went flawlessly in the CI.

However we have a pending PR that aims at being stricter (basically transform warnings into failures) and in that context we'd have to do

- websockets.handshake.check_request(headers)
+ websockets.legacy.handshake.check_request(headers)

So if I understand correctly the 9.0 changelog wording, in particular:

Despite the name

this fix is correct in the sense it is still supported and will be until v10 happens, it this the right way to understand it ? In which case we should probably do websockets>=8.*, <10 ?

Second question, I'm unsure about the following:

The framing, handshake, headers, http, and uri modules in the websockets package are deprecated. These modules provided low-level APIs for reuse by other WebSocket implementations, but that never happened. Keeping these APIs public makes it more difficult to improve websockets for no actual benefit.

so the deprecation warning we have on websockets.handshake.check_request(headers) is very clear when reading the above statement, this said what would be an "equivalent" way to proceed on 9.* without relying on those "not public" APIs ?

merci !

aaugustin commented 3 years ago

I have to give you more context :-)

The backwards compatibility policy says that I intend to maintain backwards compatibility for 5 years after deprecating an API. No worries — we have plenty of time to figure this out! I have kept backwards compatibility shims for longer than 5 years when I suspected that others depended on them.

The fix works and will continue working for several years, certainly until websockets 10, most likely in the following major versions. (SemVer forces me to bump the major version for changes that might break a few users even if they're fine for most... so the major version number climbs quite fast...)

Technically the legacy module is a private API but I know that it will be imported directly so I'll be careful if I ever decide to remove it. I don't have specific plans at this time. Keeping it "foverever" is an option if there are good reasons to do so.

So, here's how we end up there.

When I started websockets in 2013, I designed it in a modular way so that others could reuse bits and documented public APIs. Quickly I realized this aspect of the design didn't work because several projects ended up resorting to hacks for integrating websockets :-/

When Sans-I/O came up (in 2017 I think?) I thought that providing a Sans-I/O layer (similar to wsproto) would be a much better abstraction. So I set out to refactor websockets into a Sans-I/O layer and integration layers.

The Sans-I/O layer is there: https://github.com/aaugustin/websockets/blob/2990cf761177073e6d741e1ef81dc1d6d3c5dba8/src/websockets/server.py#L51 Until I finish at least one integration layer, it could still change, so I didn't document or advertise it yet.

The most advanced integration layer is #885 — not done yet.

During this refactoring, I deprecated all low-level public APIs from the original, failed design. The driver was "let's make it private; no one uses it anyway AFAIK; then I get more freedom to refactor".

I didn't expect anyone to be using this low level APIs. Now I realize uvicorn does. Obviously I'm not going to strand you :-) At least I need to fix the changelog; "that never happened" is wrong.

I released 9.0 half-way through the refactoring effort (i.e. with the current implementation moved to legacy and the future implementation not done yet) because I wanted to release other bugfixes and improvements. This explains the less-than-ideal state of the project for you. (It's OK for typical users; you aren't a typical user.)

Looking forwards, there are two solutions:

  1. I keep the legacy.handshake module until further notice. The interesting part is negotiating extensions; it's a lot more code that one would expect; so I would advise against copy-pasting it in uvicorn.
  2. Once the Sans-I/O layer is ready, you switch to using it. I believe this is hard to assess until it's done.

If you tell me exactly what you use websockets for, perhaps we can find the best way together?

The whole point of this huge refactoring is to make websockets easier to integrate for you. I'd love to know your thoughts!

euri10 commented 3 years ago

I have to give you more context :-)

that was indeed very detailed.

At least I need to fix the changelog; "that never happened" is wrong.

nah as you said it's probably not the typical use, I can bear with it of course !!

Looking forwards, there are two solutions:

1 is perfectly fine I think for the time being, assessing 2 in due course will be interesting

If you tell me exactly what you use websockets for, perhaps we can find the best way together?

I don't know how intimate you are with the asgi spec so the following may sound ovious, but uvicorn is an asgi webserver (it implements that specification, you can think of it as an async version of the wsgi spec), and for the ws part of that spec we use your library by sublassing WebSocketServerProtocol I think the original author @tomchristie could definitly be a more interesting person than me to talk about the design though !

aaugustin commented 3 years ago

Yes I'm familiar with ASGI. (I'm a Django committer; in 2013, I investigated what it would take to make Django async and failed; then Andrew picked up this flag and eventually came up with Django Channels and ASGI.)

I talked with Tom a few years back. I just reviewed how uvicorn integrates websockets, and indeed, there's quite a bit of surgery involved to integrate it. It works but it depends rather heavily on websockets internals.

Likely you could build something cleaner on top of the Sans-I/O layer. I'll keep this issue open and let you know when it's ready.

aaugustin commented 3 years ago

Sanic has the same issue: https://github.com/sanic-org/sanic/pull/2154

However they're actively moving to the Sans-I/O implementation which will remove the dependency on the handshake module.

aaugustin commented 3 years ago

Hello!

The Sans-I/O layer I was discussing earlier is now a public API in websockets 10.0.

Here's how to integrate it:

Here's Sanic adopting it:

aaugustin commented 2 months ago

In case this helps, #1332 integrate the Sans-I/O layer in an asyncio implementation.

Kludex commented 2 months ago

@aaugustin I just don't know the right thing to do here.

In Uvicorn, we are using the WebSockets legacy, but it's working just fine - and I've fixed a lot of issues in the Uvicorn implementation over the last two years.

We have support for 2 websocket implementations. Uvicorn uses websockets legacy if websockets is installed, and uses the wsproto one if the latter is installed.

What do you recommend us to do? Create a new implementation with the WebSockets Sans-IO, and deprecate the old one? Are you going to not accept changes on the legacy implementation if we find issues?

aaugustin commented 2 months ago

Create a new implementation with the WebSockets Sans-IO, and deprecate the old one?

Ideally, yes. This should result in a cleaner, more reliable integration, also more similar to the wsproto integration.

By "more reliable", I mean "less vulnerable to issues like https://github.com/python-websockets/websockets/issues/1396#issuecomment-2088140120".

Are you going to not accept changes on the legacy implementation if we find issues?

It's so stable at this point that I'm not really worried about bugs — and I'm still going to fix them for several years.

It's just annoying to keep it compatible with new Python versions, mypy updates, etc. This implementation was born in the very early days of asyncio, when it was still called tulip, and there are home-grown bits (like my own version of unittest.IsolatedAsyncioTestCase, etc.)

I'm less motivated to keep it alive now that the new asyncio implementation — with a better design, more reliable tests, etc. — has landed in the main branch.

If we're pragmatic, options are:

  1. an uvicorn contributor is motivated to reimplement the websockets integration in uvicorn
  2. I am motivated to do it
  3. nothing changes; I have to keep maintaining the legacy implementation to avoid stranding you on a version that doesn't get security updates

If 1. doesn't happen and 3. becomes too painful, then it will have to mean 2.

Kludex commented 2 months ago

There's already one implementation in a close PR, I can't reopen, and ask your review. If that's okay.

But the thing is... We'll have 3 implementations of websockets, and if we are doing things by the book, the legacy will be the default when you have websockets installed, and optionally allow the new implementation to be selected. What I worry is that no one will select the new one...

I'll reopen the old PR soon - when I get to the computer.

aaugustin commented 2 months ago

IMO you should remove the old websockets implementation (using private APIs) relatively quickly and keep only the new implementation (using the Sans-I/O layer & public API) relatively quickly.

Rationale: