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.

Sign up for GitHub

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

Jump to bottom

(Bug in client/connection.py) "raise websockets.exceptions.ConnectionClosed" needs a 'Close' object, not the code and reason directly. #989

Closed
boltex opened this issue Dec 3, 2023 · 4 comments
Closed

(Bug in client/connection.py) "raise websockets.exceptions.ConnectionClosed" needs a 'Close' object, not the code and reason directly. #989

boltex opened this issue Dec 3, 2023 · 4 comments
Labels
hint/2.9 going on 2.9 branch state/incomplete need more information

Comments

@boltex
Copy link

boltex commented Dec 3, 2023

As per https://websockets.readthedocs.io/en/stable/reference/exceptions.html#websockets.exceptions.ConnectionClosed The parameters need to be 'Close' object, not a number code and reason string as seen on line 483.

python-libjuju/juju/client/connection.py

Line 483 in 2581b0c

raise websockets.exceptions.ConnectionClosed(0, 'websocket closed')

@cderici
Copy link
Contributor

cderici commented Dec 4, 2023

Thanks for catching this, @boltex! I think you're right 👍

@cderici cderici added the hint/2.9 going on 2.9 branch label Dec 4, 2023
cderici added a commit to cderici/python-libjuju that referenced this issue Dec 4, 2023
@cderici
Pass websocket Close frame into ConnectionClosed
93cd118 
Fixes juju#989
@cderici cderici mentioned this issue Dec 4, 2023
Merged
@boltex
Copy link
Author

boltex commented Dec 4, 2023

@cderici No problem!
Note: The few code bases that create a websockets.exceptions.ConnectionClosed on github have this bug. So chatGPT and copilot propose that same error! ...that's how I got it wrong in my project also and stumbled on your project when trying to see some examples!

jujubot added a commit that referenced this issue Dec 5, 2023
@jujubot
Merge pull request #990 from cderici/websocket-connection-closed
5711ade 
#990

#### Description

This fixes a bug in connection where we pass close code and reason directly into the `websockets.exception.ConnectionClosed`, where it needs a Close frame that contains those.

Fixes #989


#### QA Steps

This doesn't affect any normal behavior, as this is sort of a safeguard against races. The only two times we raise this explicit exception is when the `MONITOR` is closed but either the receiver task is running still or someone makes an rpc call, which shouldn't happen anyways. So manually checking the arguments by the [doc](https://websockets.readthedocs.io/en/stable/reference/exceptions.html#websockets.exceptions.ConnectionClosed) is sufficient.

All CI tests need to pass.

#### Notes & Discussion

Needs to be forward ported into 3.x.
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 4, 2024

This issue is marked as incomplete because it has been open 30 days with no activity. Please remove incomplete label or comment or this will be closed in 5 days.

@github-actions github-actions bot added the state/incomplete need more information label Jan 4, 2024
@cderici
Copy link
Contributor

cderici commented Jan 8, 2024

Closed by #990

@cderici cderici closed this as completed Jan 8, 2024
cderici added a commit to cderici/python-libjuju that referenced this issue Feb 7, 2024
@cderici
Pass websocket Close frame into ConnectionClosed
7b7579e 
Fixes juju#989
@cderici cderici mentioned this issue Feb 8, 2024
Merged
jujubot added a commit that referenced this issue Feb 8, 2024
@jujubot
Merge pull request #1022 from cderici/forward-ports-from-29
dd93886 
#1022

#### Description

This brings onto the 3.x track some of the latest fixes from to the 2.9 track. Here're the details:

* Fix for #989 from #990
* Fix for #1001 from #1002
* Fix for #998 from #1003


#### QA Steps

No QA needed for #990. For 1002 and 1003 please refer to their QA steps. Though they are very related so I'd expect the QA for both of them can be done in one fell swoop.

* #1002 
* #1003 

All CI tests need to pass (there's still some known intermittent ones in there).
mthaddon pushed a commit to mthaddon/python-libjuju that referenced this issue Mar 21, 2024
Backport fix for juju#989
ebf7e17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
hint/2.9 going on 2.9 branch state/incomplete need more information
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@cderici @boltex