fix: create websockets ConnectionClosed error correctly

[james-garner-canonical]

Description

Fixes: #1252

QA Steps

Test should keep passing.

Notes & Discussion

I wonder if we need to bump our minimum websockets version.

[dimaqq]

There seems to be 3 cases of raise ConnectionClose*** in the codebase.

Pls check if those other invocations also need a fix or could use a simplification.

[james-garner-canonical]

The two in juju/client/connection.py seem like they should be fine.

On line 383 we have raise websockets.exceptions.ConnectionClosedOK(None, None)

On line 571 we have raise websockets.exceptions.ConnectionClosed(...) with a single argument, which should also be fine with the logic that causes #1252

I've updated the instance in tests/unit/test_connection.py to hopefully be more future proof. I guess our tests are using older websockets where this isn't a problem, so it's nice that things are passing there, but should we be using the new websockets version?

[dimaqq]

This repo CI would ideally run against oldest and newest supported versions of dependencies.
Then again, i'm hesitant to invest time in that up front.
So, let's do a manual spot check and call it a day :)

[james-garner-canonical]

Sounds good. What would a manual spot check entail in this case?

[dimaqq]

Raising these exceptions using oldest/newest versions of websockets that we support.
I think that should do it.

[dimaqq]

Another possibility is to look if the code should be raising ConnectionClosedOK or ConnectionClosedError and not the base class.

[dimaqq]

See https://websockets.readthedocs.io/en/stable/reference/exceptions.html#websockets.exceptions.ConnectionClosed

The signature was changed in websockets 10.0, but there's no direct guidance how to "update your code".

My take is that we should either not supply any arguments, or supply the minimal arguments that are required.

[dimaqq]

P.S. ugh websockets==13.1 release was not made form the 13.x maintenance branch 🙈

[dimaqq]

Let's update every single place this package raises a websockets exception:

• raise ConnectionClosedOK() whenever it seems that the situation is OK
  • current code has OK
  • or current code has code 1000 or 1001
• raise ConnectionClosedError() otherwise

As far as I can see, websockets versions that are new enough don't require arguments for these exception classes.

Edit: as James rightfully points out below, raise ConnectionClosedOK|Error(None, None).

[james-garner-canonical]

It seems that for the oldest version of websockets that we support (13.0.1) and for the latest release (15.0.1) the ConnectionClosed exceptions require both rcvd and sent arguments. These should be websockets.frames.Closed objects or None. If both are Closed objects, then the third argument is also required, to specify which order they occurred in.

The instance that caused our users problems here did raise ConnectionClosed(int_code, str_reason), which may be an older API, or may just be wrong -- it's using the arguments expected for a Closed object. ConnectionClosed only checks if the arguments are None, so it doesn't complain -- except that it now requires the third argument if they're both non-None, which is what we're hitting here.

I'm not sure what's expected typically, but in the cases where we raise a ConnectionClosed manually, I don't see an obvious thing to put in sent and rcvd other than None, so I've gone with that. This gives the message 'no close frame received or sent', which I guess is fine here.

For a little bit more user facing information, I've changed them to ConnectionClosedOK in connection.py since they seem to be for cases where the socket already closed cleanly, and to ConnectionClosedError in model/__init__.py -- but maybe that should be ConnectionClosedOK too?

Oh and the exception types are exposed on websockets directly, so I've removed the use of websockets.exceptions just to clean up.

[james-garner-canonical]

Test coverage is clearly not great here, as it didn't catch the issue our users encountered, nor did it catch my original broken 'fix' of raising with no arguments (two arguments are required), but I've manually verified on oldest (13.0.1) and newest (15.0.1) versions of websockets that the current exceptions raise cleanly.

[dimaqq]

I suspect that bundle errors are unrelated.
I've made #1256 to test that theory.

[dimaqq]

I think the latest anbox bundle is broken: https://bugs.launchpad.net/anbox-cloud/+bug/2103485
And the dummy PR fails in the very same way as this.

[dimaqq]

/build

[dimaqq]

/merge