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

Drop use of Monitor._ws.open and Connection._ws.closed #1208

Closed
wants to merge 1 commit into from

Conversation

freyes
Copy link
Contributor

@freyes freyes commented Nov 22, 2024

The websockets library has dropped the properties "open" and "closed" since websockets-13.0[0], this patch makes the changes needed to check for the connection's state as suggested by the documentation.

[0] python-websockets/websockets@7c8e0b9

Fixes #1207

The websockets library has dropped the properties "open" and "closed"
since websockets-13.0[0], this patch makes the changes needed to check
for the connection's state as suggested by the documentation.

[0] python-websockets/websockets@7c8e0b9
@dimaqq
Copy link
Contributor

dimaqq commented Nov 23, 2024

I imagine that would allow us to upgrade to websockets 14 or remove the dependency upper bound, wouldn’t it?

@dimaqq
Copy link
Contributor

dimaqq commented Nov 23, 2024

Pls reword commit messages to follow conventional commits

@dimaqq
Copy link
Contributor

dimaqq commented Nov 25, 2024

https://github.com/python-websockets/websockets/blob/59d4dcf779fe7d2b0302083b072d8b03adce2f61/src/websockets/protocol.py#L57-L60

The websockets library defines 4 values for State.

This PR references only two.

I wonder what about the other two.

@@ -357,7 +358,7 @@ async def close(self, to_reconnect: bool = False):
tasks_need_to_be_gathered.append(self._debug_log_task)
self._debug_log_task.cancel()

if self._ws and not self._ws.closed:
if self._ws and self._ws.state is not State.CLOSED:
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking through websockets code, the library already handles the corner cases and we should close the connection unconditionally.

@dimaqq dimaqq mentioned this pull request Nov 25, 2024
freyes added a commit to freyes/zaza that referenced this pull request Nov 25, 2024
Set upper bound for websockets until libjuju is compatible with newer
versions.

See juju/python-libjuju#1208
@dimaqq
Copy link
Contributor

dimaqq commented Nov 26, 2024

Continued in #1216

@dimaqq dimaqq closed this Nov 26, 2024
javacruft pushed a commit to openstack-charmers/zaza that referenced this pull request Nov 26, 2024
* Pin websockets library

Set upper bound for websockets until libjuju is compatible with newer
versions.

See juju/python-libjuju#1208

* Migrate to actions/upload-artifact@v4
freyes added a commit to freyes/zaza that referenced this pull request Nov 26, 2024
* Pin websockets library

Set upper bound for websockets until libjuju is compatible with newer
versions.

See juju/python-libjuju#1208

* Migrate to actions/upload-artifact@v4

(cherry picked from commit d758d6d)
freyes added a commit to freyes/zaza that referenced this pull request Nov 26, 2024
* Pin websockets library

Set upper bound for websockets until libjuju is compatible with newer
versions.

See juju/python-libjuju#1208

* Migrate to actions/upload-artifact@v4

(cherry picked from commit d758d6d)
freyes added a commit to freyes/zaza that referenced this pull request Nov 26, 2024
* Pin websockets library

Set upper bound for websockets until libjuju is compatible with newer
versions.

See juju/python-libjuju#1208

* Migrate to actions/upload-artifact@v4

(cherry picked from commit d758d6d)
(cherry picked from commit e8d72f4)
freyes added a commit to freyes/zaza that referenced this pull request Nov 26, 2024
* Pin websockets library

Set upper bound for websockets until libjuju is compatible with newer
versions.

See juju/python-libjuju#1208

* Migrate to actions/upload-artifact@v4

(cherry picked from commit d758d6d)
(cherry picked from commit e8d72f4)
(cherry picked from commit 9b5291c)
freyes added a commit to freyes/zaza that referenced this pull request Nov 26, 2024
* Pin websockets library

Set upper bound for websockets until libjuju is compatible with newer
versions.

See juju/python-libjuju#1208

* Migrate to actions/upload-artifact@v4

(cherry picked from commit d758d6d)
(cherry picked from commit e8d72f4)
(cherry picked from commit 9b5291c)
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.

AttributeError: 'ClientConnection' object has no attribute 'open'
2 participants