-
Notifications
You must be signed in to change notification settings - Fork 41
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
Client close function #112
Conversation
…ers to close the client connection from within a async function such as async main(). This is useful to allow users to use the `asyncio.run()` wrapper properly
def close_loops(self) -> None: | ||
"""Closes the HTTP connection | ||
""" | ||
LOG.info("Clash of Clans client logging out...") | ||
self.dispatch("on_client_close") | ||
self.loop.run_until_complete(self.http.close()) | ||
self.loop.close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def close_loops(self) -> None: | |
"""Closes the HTTP connection | |
""" | |
LOG.info("Clash of Clans client logging out...") | |
self.dispatch("on_client_close") | |
self.loop.run_until_complete(self.http.close()) | |
self.loop.close() | |
def close_loops(self) -> None: | |
"""Closes the HTTP connection""" | |
self.loop.run_until_complete(self.http.close()) | |
self.loop.close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you omitting the dispatch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you omitting the dispatch?
because this is actually closing the asyncio loop
and not doing what the dispatch name says. The thing that dispatch name says is actually being one by the close_client
method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I think you've found the proper way to fix all these loop cleanup issues :)
await client.close_client() | ||
|
||
if __name__ == '__main__': | ||
try: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line should probably be indented
@@ -278,14 +278,20 @@ def login_with_keys(self, *keys: str) -> None: | |||
|
|||
LOG.debug("HTTP connection created. Client is ready for use.") | |||
|
|||
def close(self) -> None: | |||
def close_loops(self) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only little thing I'm worried about is making sure we don't break stuff in a non-major version, ie. I think (please correct me if I'm wrong) but client.close() now doesn't exist.
Long-term, should we always be creating and closing clients with await stuff and letting asyncio handle running async stuff in non-async places? I wonder whether that would solve all our issues, and whether I'm trying to do something that isn't really my job.
So change .close()
to be an async function and either make coc.login()
an async function or remove it completely.
Like you showed, it's easier (and better if it doesn't have all those loop cleanup issues) to leave it to asyncio.run() to run it all. Thoughts? I'm kinda just rambling. Either way, I think we can add this stuff with different names now, but need to keep the existing functions until v3 (maybe adding a depreciated warning)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
asyncio.run() requieres python 3.7 or newer, we would need to add that somewhere too.
The big downside with asyncio.run() is most likely the usage of a coc client with another aiohttp session, like a discord bot. We should definitely test that before we discussing this solution further.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely, and very true re. python 3.7
I guess the asyncio.run
stuff would only be for people running it as a standalone script thing - else the current coc.login
is fine (and doesn't have the loop cleanup issues / people don't care as much because there's a hundred other warnings when you close a bot). I think the real issue is client.close()
, not login so much. .close()
isn't really a thing for bots, and never works for non-bots as intended.
I don't think people generally have ever had many issues with login in it's current form, so maybe it's best to leave that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you actually mentioned it I personally do not think it is on your library to handle the cleanup because it will create a race condition with other libraries that also depend on sessions to be cleaned out.
This is the best way I have found to handle cleanup of both coc.py sessions and asyncpg and that's to drop into "manual" loops instead of the abstracted run only to be able to properly clean up after myself. The asyncio.run does do the generator cleanup but it will not properly clean up sessions. Which may or may not be an issue.
https://github.com/majordoobie/PantherLily/blob/main/main.py#L117
the example code with the modifications above produces still an error.
According to this aiohttp issue, this aiohttp issue and this httpx issue the problem is the underlaying SSL transport. Currently the only way to avoid this error message is to give the SSL transport time to shutdown by adding a The example bot is also affected by the same error but not as you may think. The discord bot itself has the same problem when using |
changes described [here](mathsman5133#112 (comment))
as soon as @majordoobie merges the pr we should be good to merge this one too |
Moved to #116 |
#111
[Added coc.Client.close_client coroutine] Added coroutine to allow us…ers to close the client connection from within a async function such as async main(). This is useful to allow users to use the
asyncio.run()
wrapper properly