Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add compatibility with websockets 11.0. #2609

Merged
merged 6 commits into from Nov 29, 2022

Conversation

aaugustin
Copy link
Contributor

@aaugustin aaugustin commented Nov 27, 2022

I failed at naming things in the Sans-I/O layer of websockets. I ended up with Connection and Protocol swapped vs. what one would expect: Protocol was managing the network and Connection didn't know about the network.

I am about to rename in python-websockets/websockets#1269. Since Sanic was an early adopter — actually, the first mainstream library to adopt it — it will need an update. I'm sorry.

This PR provides compatibility both with websockets 10.0 (without deprecation warnings) and with the upcoming release websockets 11.0.


sanic currently imports private APIs for which websockets won't provide backwards-compatibility. Specifically, it imports OPEN, CLOSING, CLOSED directly rather than the State enum. To avoid breaking Sanic users, I am planning to keep websockets 11.0 on hold until you release Sanic 22.12, with this PR included.

This is why I have to support both versions of websockets: I cannot simply release websockets 11.0 then update Sanic (bumping the dependency to websockets >= 11.0 to avoid deprecation warnings).


In detail:

  • The first commit switches to the correct import paths for websockets 11.0, with fallback to import paths for websockets 10.x.
  • The second commit renames a variable for consistency with the new name in websockets. It is optional; I can remove it if you don't want it.
  • The third commit ensures that Sanic only uses public APIs of websockets, for which backwards-compatibility is guaranteed a minimum of 5 years.

@aaugustin aaugustin requested a review from a team as a code owner November 27, 2022 17:05
This is consistent with the io_proto instance variable and with the type
of the variable: it's a websockets.server.ServerProtocol.
@aaugustin
Copy link
Contributor Author

Actually, I have to use the websockets 10.x imports and fallback to the websockets 11.0 imports, or else mypy gets confused.

It will get confused again when I release websockets 11.0. Fortunately, at that point, we can remove the fallback and bump the dependency to websockets >= 11.0.

@aaugustin
Copy link
Contributor Author

CI failures look unrelated to my changes. Other PR suffer from the same failures. I am planning to wait until this is cleared and then rebase this PR.

@ashleysommer
Copy link
Member

@aaugustin Thank you very much for your work to adapt this, and for your continued work on the Websockets library. These changes look good to me.

@ahopkins
Copy link
Member

I really have no clue what happened to those TLS tests. They run fine locally. Some package somewhere must have changed. Regardless, I have disabled them for now so we can get the ball rolling again.

@aaugustin as always, your continued support and efforts with websockets and assistance with sanic is greatly appreciated.

Copy link
Member

@ahopkins ahopkins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before we merge this, I want to make one tweak to add a property for connection that will handle any backwards compat. Will push to your branch.

@codecov
Copy link

codecov bot commented Nov 29, 2022

Codecov Report

Base: 88.047% // Head: 87.882% // Decreases project coverage by -0.165% ⚠️

Coverage data is based on head (adaa580) compared to base (beae35f).
Patch coverage: 47.058% of modified lines in pull request are covered.

Additional details and impacted files
@@              Coverage Diff              @@
##              main     #2609       +/-   ##
=============================================
- Coverage   88.047%   87.882%   -0.166%     
=============================================
  Files           71        71               
  Lines         5338      5356       +18     
  Branches       893       893               
=============================================
+ Hits          4700      4707        +7     
- Misses         461       471       +10     
- Partials       177       178        +1     
Impacted Files Coverage Δ
sanic/server/websockets/impl.py 37.788% <42.857%> (-0.274%) ⬇️
sanic/server/protocols/websocket_protocol.py 71.428% <66.666%> (-1.588%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants