mental32 / spotify.py

🌐 API wrapper for Spotify 🎶
https://spotifypy.readthedocs.io/en/latest/
MIT License
150 stars 38 forks source link

Issue with session closing #45

Closed AdrienPensart closed 4 years ago

AdrienPensart commented 4 years ago

Version: master Python: 3.7.5 aiohttp (3.6.2)

I tried both modes, async and sync, same result :

2019-12-07 10:39:21 asyncio[3439466] ERROR Unclosed client session client_session: <aiohttp.client.ClientSession object at 0x7fe47f168810> 2019-12-07 10:39:21 asyncio[3439466] ERROR Unclosed connector connections: ['[(<aiohttp.client_proto.ResponseHandler object at 0x7fe47f1514b0>, 1495824.926715675)]'] connector: <aiohttp.connector.TCPConnector object at 0x7fe47f168850>

It seems I have to manually close it : await user.http.close()

Moreover, in your README, there is : client.loop.run_until_complete(main())

Should it be "asyncio.run(main())" ?

mental32 commented 4 years ago

Every spotify.Client and http enabledspotify.User objects have an aiohttp.ClientSession open underneath them.

aiohttp will emit errors/warnings for when the sessions are inappropriately destroyed, the solution is to properly close the sessions. spotify.Client has a close method which will close the underlying client session, spotify.User objects have a http attribute that have a close method for their respective session. You must call these methods when you are discarding the objects.

The library provides an asynchronous context manager for spotify.Client so you don't have to worry about the Client at least:

async with Client(...) as client:
    await stuff

There is currently no such implementation for User objects, since they're intended to be created by other models, the client itself or via one of the User constructors such as from_code or from_token. The solution there is to call user.http.close() yourself.

I'm unable to recall the extent that spotify.sync supports the close calls, so if there are issues there let me know :)

mental32 commented 4 years ago

PS: the close methods are coroutine functions, they must be awaited.

mental32 commented 4 years ago

Although... with hindsight. The rationale behind the lack of an async context manager for User objects seems a little nonsensical, the following situation could be possible:

async with User.from_token(client, token) as user:
    ...
# Or alternatively
async with client.user_from_token(token) as user:
    ...

I think I'll add this and roll a new release by the end of today to address this lopsided design.