fix: clean up background tasks on close

[agoose77]

This PR attempts to close #503 and fix #161 by cleaning up background tasks upon closure of the websocket. I'm not sure if this fits into the wider design space for resource management, but it does fix "hang on shutdown" on my system.

[github-actions]

👈 Launch a Binder on branch agoose77/jupyter-collaboration/fix-close-background-tasks

[davidbrochart]

That would prevent cleaning up the room that was launched here, right?

[agoose77]

I don't think so? That is launched by asyncio's create_task, rather than the class helper.

[davidbrochart]

Ah right.
So it looks like you are also cancelling the WebSocket server (launched here), while you only want to cancel this particular handler)?

[krassowski]

I edited description as I closed #503 as a duplicate of #161

[agoose77]

It looks as though there's already some consideration for cleanup in

jupyter-collaboration/projects/jupyter-server-ydoc/jupyter_server_ydoc/websocketserver.py

Lines 62 to 76 in 5db09a6

	# FIXME some clean up should be upstreamed and the following does not
	# prevent hanging stop process - it also requires some thinking about
	# should the ystore write action be cancelled; I guess not as it could
	# results in corrupted data.
	# room_tasks = list()
	# for name, room in list(self.rooms.items()):
	# for task in room.background_tasks:
	# task.cancel() # FIXME should be upstreamed
	# room_tasks.append(task)
	# if room_tasks:
	# _, pending = await asyncio.wait(room_tasks, timeout=3)
	# if pending:
	# msg = f"{len(pending)} room task(s) are pending."
	# self.log.warning(msg)
	# self.log.debug("Pending tasks: %r", pending)

Do we know why this was committed but commented out?

[krassowski]

It looks like tests did not run because pre-commit failed:

check builtin type constructor use.......................................Failed
- hook id: check-builtin-literals
- exit code: 1

projects/jupyter-server-ydoc/jupyter_server_ydoc/websocketserver.py:66:21: replace list() with []

trim trailing whitespace.................................................Passed
ruff.....................................................................Passed
ruff-format..............................................................Passed
doc8.................................................(no files to check)Skipped
mypy.....................................................................Failed
- hook id: mypy
- exit code: 1

projects/jupyter-server-ydoc/jupyter_server_ydoc/websocketserver.py: note: In member "clean" of class "JupyterWebsocketServer":
projects/jupyter-server-ydoc/jupyter_server_ydoc/websocketserver.py:68: error:
"YRoom" has no attribute "background_tasks"  [attr-defined]
                for task in room.background_tasks:
                            ^~~~~~~~~~~~~~~~~~~~~
Found 1 error in 1 file (checked 28 source files)

[ianhi]

Hi,

does this fix the core issue, beyond the pre-commit check (and then maybe tests) any more to do here?

[ianhi]

does this fix the core issue,

seemingly no :(

I did find that adding a client._message_queue.put_nowait(b"") here:

https://github.com/ianhi/jupyter-collaboration/blob/dec4789d7b58f90ea653b48aa72595d09376ecf3/projects/jupyter-server-ydoc/jupyter_server_ydoc/websocketserver.py#L59-L69

im looking into a cleaner way (maybe moving that to client) shutdown

[ianhi]

actually that works somtimes, but not always. very mysterious. i guess there is a race happening somewhere. will keeping digging.

[ianhi]

I found the race. will make a pr

[ianhi]

fix that works locally for me in #546