Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
manics opened this issue Oct 8, 2024 · 2 comments
Open

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

manics opened this issue Oct 8, 2024 · 2 comments
Labels

Comments

@manics
Copy link
Member

manics commented Oct 8, 2024

Bug description

There are some calls to asyncio.ensure_future(...) that aren't awaited:

if previous_reflector:
# we replaced the reflector, stop the old one
asyncio.ensure_future(previous_reflector.stop())

self.log.error(
"Pod %s never showed up in reflector, restarting pod reflector",
ref_key,
)
self.log.error("Pods: %s", sorted(self.pod_reflector.pods.keys()))
asyncio.ensure_future(self._start_watching_pods(replace=True))

self.log.error(
"Pod %s did not disappear, restarting pod reflector", ref_key
)
asyncio.ensure_future(self._start_watching_pods(replace=True))

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 manics added the bug label Oct 8, 2024
@manics manics changed the title Should we always await asyncio.ensure_future()? asyncio.ensure_future() is not always awaited or saved Oct 8, 2024
@manics
Copy link
Member Author

manics commented Oct 9, 2024

There's also some calls in proxy.py

asyncio.ensure_future(self.ingress_reflector.start())
asyncio.ensure_future(self.service_reflector.start())
asyncio.ensure_future(self.endpoint_reflector.start())

@consideRatio
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants