Sanic no longer raises ConnectionClosed on Python ≥ 3.8

[jamesstidard]

Hi,

I've been using this library via Sanic and I've noticed that the connection errors that use to be raised in easier versions are no longer being thrown on calling awaiting websockets.protocol.WebScoketCommonProtocol.recv.

I've not been able to figure out exactly why, as it all "looks correct" from the time I spent looking at it. I'm also not sure if this is an issue from Sanic's side or from websockets, so I've cross posted here - hope that's OK.

Here's a description of problem: sanic-org/sanic#1871

[aaugustin]

This looks like a ramification of sanic-org/sanic#1214, which could get solved in a maintainable way once #676 is done.

[jamesstidard]

Hey @aaugustin,

Thanks for the response. Yeah, I can see getting sanic-org/sanic#1214 done first makes sense.

I guess I wasn't sure if this issue was actually related to how sanic integrates websockets or something that could be reproduced in just websockets itself.

That it only happens in Python 3.8, and not Python 3.7, is strange. Though if it is a kinda async race condition, then maybe the order of when the coroutines are yielded to changes between 3.7 and 3.8. That is to say that, maybe the integration in is the problem but it happens to work by chance in 3.7.

I'll maybe see if I can better understand what's going on over the weekend.

Thanks for your work on the library.

[aaugustin]

Clearly, you're onto something, but I'm not sure what yet ;-)

I'm not aware of any changes between Python 3.7 and 3.8 that may explain this.

Based on your reports here and in the sanic repository, I can't tell on which side the bug is.

I would expect that kind of bug to be caught by the test suite of websockets, but who knows!

Needs further investigation.

[cjerdonek]

Could this be caused by CancelledError being changed in 3.8 not to inherit from Exception? https://bugs.python.org/issue32528 If you look at one of the referenced issues in this report, it mentions CancelledError being raised in 3.8 when it wasn’t before. On the surface, such a behavior change could potentially be explained by CancelledError now being able to bubble up out of an "except Exception" clause where previously it would have been caught.

[cjerdonek]

@aaugustin Independent of this issue, I would recommend auditing your code for places where except Exception is used without separately handling CancelledError. I found such occurrences e.g. in server.py and protocol.py. Those spots will behave differently in 3.7 and 3.8 due to the inheritance change I mentioned above, so it would be good to be explicit in how you want the code to behave in those places.

[aaugustin]

Thanks you very much Chris!

There's a very good chance this will explain the problem we're seeing here: a CancelledError catched and re-raised as ConnectionClosed on Python 3.7, while it bubbles up on Python 3.8.

Cancellation in asyncio is a gift that keeps giving!

[aaugustin]

Looking at this from the perspective of the target behavior, where CancelledError is a "special" exception (inheriting BaseException rather than Exception, it seems to me that:

try:
    ...
except Exception:
    ...
    raise

is fine as CancelledError will be re-raised eventually.

I went through all except Exception clauses and didn't find any location where the exception wouldn't get re-raised.

So... 🤔

[aaugustin]

There's a good chance this will go away with sanic-org/sanic#2000.