jupyterhub / kubespawner

Kubernetes spawner for JupyterHub
https://jupyterhub-kubespawner.readthedocs.io
BSD 3-Clause "New" or "Revised" License
549 stars 305 forks source link

`asyncio.ensure_future()` is not always awaited or saved #858

Open manics opened 1 month ago

manics commented 1 month ago

Bug description

There are some calls to asyncio.ensure_future(...) that aren't awaited: https://github.com/jupyterhub/kubespawner/blob/621be0e68d9dea38ac899c6054f966dd68d1d028/kubespawner/spawner.py#L2803-L2805 https://github.com/jupyterhub/kubespawner/blob/621be0e68d9dea38ac899c6054f966dd68d1d028/kubespawner/spawner.py#L3252-L3257 https://github.com/jupyterhub/kubespawner/blob/621be0e68d9dea38ac899c6054f966dd68d1d028/kubespawner/spawner.py#L3379-L3382

The docs for asyncio.ensure_future say the resulting Task will be scheduled, but also says

Save a reference to the result of this function, to avoid a task disappearing mid-execution.

asyncio.create_task() has a longer explanation

Important. Save a reference to the result of this function, to avoid a task disappearing mid-execution. The event loop only keeps weak references to tasks. A task that isn’t referenced elsewhere may get garbage collected at any time, even before it’s done.

I think we should either await those, save a reference to them, or add a comment to the code explaining why it isn't necessary.

manics commented 1 month ago

There's also some calls in proxy.py https://github.com/jupyterhub/kubespawner/blob/7ba57a8c915b190b24d514d91427620c08eb1353/kubespawner/proxy.py#L358-L360

consideRatio commented 1 month ago

@manics thanks for investigating this thoroughly -- I agree on the decision to await things now that I've seen the motivation in this issue!