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

Reflectors watch: close after stop #859

Merged
merged 1 commit into from
Oct 9, 2024

Conversation

manics
Copy link
Member

@manics manics commented Oct 9, 2024

I'm trying to track down the

Unclosed client session
client_session: <aiohttp.client.ClientSession object at 0x7fcfe6bbc690>

warning in lsc-sde/lsc-sde#93 (comment)

I don't know if this is the cause, but based on the context manager code in
https://github.com/tomplus/kubernetes_asyncio/blob/v21.7.1/kubernetes_asyncio/watch/watch.py#L212-L216
we should call close() anyway.

Copy link
Member

@consideRatio consideRatio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is correct and suitable to do, but I struggle to become confident about it.

Do you want to merge or ask for another set of eyes?

@consideRatio
Copy link
Member

consideRatio commented Oct 9, 2024

By awaiting close, we'll have a delay of finalizing the _watch_and_update function call.

The _watch_and_update is called via self.watch_task = asyncio.create_task(self._watch_and_update()), and then self.watch_task is checked to be .done() or not at several locations.

The thing I'm not getting confident about, is the consequences of now waitinging for that to finalize. I figure at worst we get a performance hit, but I think we'll end up with something more robust. Due to that, I'm leaning towards we go for a merge.

@manics
Copy link
Member Author

manics commented Oct 9, 2024

Thanks for reviewing, let's merge then!

I want to look into #858 , maybe we can have a beta-3 after that

@manics manics merged commit 7ba57a8 into jupyterhub:main Oct 9, 2024
9 checks passed
@manics manics deleted the reflector-watch-close branch October 9, 2024 12:54
@consideRatio
Copy link
Member

Thank you for working this @manics!!

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

Successfully merging this pull request may close these issues.

2 participants