Refactor room and server tasks cleanup

[martinRenou]

This is partly taken from jupyter_collaboration.

The user API gets broken by this PR (making some APIs async that were sync before).

[davidbrochart]

Awaiting the cancelled task will raise a CancelledError.

Suggested change

await self._broadcast_task

[martinRenou]

It does not seem to raise a CancelledError when I run this, unless it is caught somewhere and silently passing?

How do you make sure the task finished cancelling?

[davidbrochart]

How do you make sure the task finished cancelling?

Then you need to catch CancelledError:

Suggested change

	await self._broadcast_task
	try:
	await self._broadcast_task
	except asyncio.CancelledError:
	pass

[martinRenou]

Isn't the CancelledError the thing that gets triggered in the task code when it is being cancelled?

Sorry if it's a dumb question, I'm new to these things.

[davidbrochart]

Yes CancelledError is raised in the task, so since we're awaiting that task we should catch the exception.
But thinking about it I think we should just use AnyIO, with which all these things come for free.

[martinRenou]

I see, so we'd need a restructure if we go with AnyIO

[martinRenou]

But what happens if you want to cancel a task group from outside the context manager, can you do it?

[davidbrochart]

Yes, I'm going to open a PR to show what it would look like.

[martinRenou]

thanks.

[davidbrochart]

I opened #77.

[fcollonval]

There is a while True loop that eagerly catch all exceptions that needs also to be escaped in case of cancellation:

ypy-websocket/ypy_websocket/websocket_provider.py

Lines 38 to 44 in 8010567

	while True:
	update = await self._update_queue.get()
	message = create_update_message(update)
	try:
	await self._websocket.send(message)
	except Exception:
	pass

In the PR of jupyter_collaboration for improving the clean-up, I miss that point from Python documentation:

In case asyncio.CancelledError is explicitly caught, it should generally be propagated when clean-up is complete.
Source: https://docs.python.org/3/library/asyncio-task.html#task-cancellation

So it was a mistake to wait for the task after calling cancel on them. And the CancelledError should be propagated.

For example, the while loop should become:

 while True: 
     update = await self._update_queue.get() 
     message = create_update_message(update) 
     try: 
         await self._websocket.send(message) 
     except asyncio.CancelledError:
         raise
     except Exception: 
         pass 

[martinRenou]

Thank you for your comment @fcollonval ! I have a try.

[davidbrochart]

We should close this PR in favor of #77.