Add warning for creation a session outside of coroutine

[asvetlov]

For preventing many stupid mistakes

[codecov-io]

Current coverage is 98.83% (diff: 100%)

Merging #1468 into master will increase coverage by <.01%

@@             master      #1468   diff @@
==========================================
  Files            30         30          
  Lines          6955       6961     +6   
  Methods           0          0          
  Messages          0          0          
  Branches       1152       1154     +2   
==========================================
+ Hits           6874       6880     +6   
  Misses           40         40          
  Partials         41         41          

Powered by Codecov. Last update b89a462...625de7c

[Rapptz]

Can this change possibly be reverted or maybe thought out a little more?

I have HTTP Clients in my library where I create a session object in __init__, e.g. something similar to this:

class HTTP:
  def __init__(self):
    self.session = aiohttp.ClientSession(...)

and then close it at some later point in time when necessary for clean-up.

With this new warning my users are now getting a noisy warning and reporting bugs to me about it when there is nothing actually wrong.

At best the current way to fix this is to recommend my users to shut off warnings via the filters (or do it myself programmatically) but I don't really think that's a good idea.

[asvetlov]

How do you create your HTTP class instance?
Asyncio code instantiation outside of coroutine is actually wrong idea, you always could get rid of it easy.

[Rapptz]

I create an HTTP instance inside a different class. I don't really know how to explain it without detailing the entire library's architecture. The session is maintained by an HTTP client. The HTTP client is maintained by a different Client class. This HTTP client is shared amongst many different models, e.g. Server/ConnectionState/etc (at least, in the internal rewrite stash of my library). I have no good way to lazy init the session and have the architecture I have now.

I should note that sessions are properly cleaned up. So all this warning does is bother my users and cause annoying bug reports.

I already lock the aiohttp version to <1.1.0 but people without virtualenv will still update anyway and I've received reports from those people. As much as I wish I could just pip freeze this problem away, replying with "I only support aiohttp 1.1.0" can only get me so far.

[AraHaan]

Well you could always make an Helper coroutine for that class like so:

class HTTP:
  def __init__(self, loop=None, *args, **kwargs):
    self.loop = asyncio.get_event_loop() if loop is None else loop
    self.session = self.loop.run_until_complete(self._create_session(*args, **kwargs))

  async def _create_session(self, *args, **kwargs):
    """creates a ClientSession inside of a coroutine."""
    session = aiohttp.ClientSession(*args, **kwargs)
    return session

It might need modified to fit your library needs so. At least it is an idea.

Looking at the actual class that does it it could be simple just do the following changes to the above:

class HTTPClient:
    def __init__(self, connector=None, *, loop=None):
        self.loop = asyncio.get_event_loop() if loop is None else loop
        self.connector = connector
        self.session = self.loop.run_until_complete(self._create_session())
        # other library specific stuff.

    @asyncio.coroutine
    def _create_session(self):
        """creates a ClientSession inside of a coroutine."""
        session = aiohttp.ClientSession(connector=self.connector, loop=self.loop)
        return session

    # recreate function change:

    def recreate(self):
        self.session = self.loop.run_until_complete(self._create_session())

This way you get the following:

The client Session.

The warning removed.

I do not like the fact that there is no recreate method in ClientSession however causing you to have to recall the class. The issue with that is when a class is initialized there is not destructing it until the Python Interpreter closes. I think a better solution is to add a copy of __init__ into a new function named recreate in aiohttp.ClientSession() so that way the class does not need reinitialized to recreate the client Session after being closed.

[PaulReiber]

...wow! Nice work, AraHaan. Kind regards, *Paul Reiber*http://about.me/reiber

…

On Fri, Dec 23, 2016 at 2:30 PM, AraHaan ***@***.***> wrote: Well you could always make an Helper coroutine for that class like so: class HTTP: def __init__(self): self.session = asyncio.create_task(self._create_session(), loop=loop) async def _create_session(self, *args, **kwargs): """creates an ClientSession inside of a coroutine.""" session = await aiohttp.ClientSession(*args, **kwargs) return session — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1468 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADlQBjXwXB-oOwHjLBAH2wKKjIneJtWks5rLEufgaJpZM4LI7Ww> .

[Rapptz]

@AraHaan So what you're suggesting is that I have to create an asyncio.Task that has an aiohttp.ClientSession as its pending result to bypass this warning. Something that is completely non-deterministic and an extra layer of indirection that I have to account for in every model.

To actually make use of your session, I would have to either call self.session.result() at some point in the future (and block the main thread in doing so, not that bad all things considered) or await/yield from it. Then go through most things that use the "session" and update the references pointed to it.

Honestly this is overall a really poor workaround. I also can't comprehend what the 'recreate' part is all about.

[AraHaan]

The recreate part is to basically recreate the client session just like your recreate method does without having to reinitialize the class which can cause in most cases a Resource or Memory leak because the old instance is not getting destroyed.

Also edited it above to instead use run_until_complete.

[AraHaan]

@Rapptz just so you know on your HTTP client class on the recreate method of yours you did not delete the old client session instance before reinitializing it after it has been closed thereby it can gradually create a memory leak. you might need to consider deleting it before creating the new session instance on that method.

[asvetlov]

Well, a warning might be omitted if loop is passed to ClientSession explicitly (ClientSession(loop=loop)).

[AraHaan]

hmm oddly they are passing in a loop that they use when they call their HTTP client but still the warning shows for some reason. Or at least I think they are passing the loop to their http class.

Yep I have verified by looking inside Rapptz's client.py the warning still persists and inside their Client class they do this:

self.loop = asyncio.get_event_loop() if loop is None else loop
self.http = HTTPClient(connector, loop=self.loop)

and then HTTPClient lazy inits ClientSession like so:

    def __init__(self, connector=None, *, loop=None):
        self.loop = asyncio.get_event_loop() if loop is None else loop
        self.connector = connector
        self.session = aiohttp.ClientSession(connector=connector, loop=self.loop)

[asvetlov]

Sorry, I was not clean.
Currently ClientSession throws a warning even with explicit loop. The code should be modified to raise the warning only if implicit loop is used and the loop is not running.
The same behaior should be applied to connector's logic.

show_warning is not an option.

The main intention for adding the warning is prevention silly error-prone code like the following:

    class A:
         client = aiohttp.ClientSession()

This code is just not testable.

[AraHaan]

Ok then, modified #1507 to reflect what you explained @asvetlov now only file that needs changed in the PR are the CHANGES.rst file if desired for a different wording otherwise I dont see why not merge it all it does is fix one line in client.py and does not change coverage at all now.

[Rapptz]

@asvetlov removing the warning if an explicit loop is given is good enough for me (and it's what people should be doing anyway).

[AraHaan]

Ok, @Rapptz #1507 had a merge conflict so I had to remake the pull request into #1518. Feel free to review it if you need to. Also @asvetlov you could merge 1518 right now if you like since it should not change coverate at all. It just makes the warning go away if loop is not None.

[Rapptz]

I was thinking of PRing the changes myself since this issue is becoming worse and worse on my end with the support questions.

However I stumbled across a roadblock while doing so. The logic for detecting the loop can be initialised in two different branches and it's not really clear if something like this should raise a warning (and if so, how):

s = aiohttp.ClientSession(connector=aiohttp.TCPConnector())

There's technically no explicit loop passed, so I wasn't sure how to PR the changes with this bikeshed in my mind. I was thinking of just a menial check of loop is None since IMO that should be enough, but there are cases where you can do something like this:

s = aiohttp.ClientSession(connector=aiohttp.TCPConnector(loop=some_loop))

And I'm not sure if that counts as "explicit loop passing" under the criteria of shutting off the warnings.

Any help would be appreciated.

[WGH-]

Care to explain how is it any more dangerous than creating any other "asyncio object" outside of the coroutine, like asyncio.Lock, asyncio.Queue, asyncio.Whatever or anything from third-party libraries?

[fafhrd91]

When you create session or TCPConnector objects, they capture current loop. If for some reason you set new loop, those objects would stop work. Sure, you can make it work, but you need to be more careful. asyncio.Lock,Queue,Semaphore suffer from same problem

…

On Feb 22, 2017, at 11:30 AM, WGH ***@***.***> wrote: Care to explain how is it any more dangerous than creating any other "asyncio object" outside of the coroutine, like asyncio.Lock, asyncio.Queue, asyncio.Whatever or anything from third-party libraries? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1468 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAkjzl7owWk2tGIjU1H0gtF-3temUj-Cks5rfIzDgaJpZM4LI7Ww>.

[cglacet]

Isn't the best solution to simply reproduce what aiohttp.ClientSession does? You just need to turn your HTTP objects into asynchronous context managers (using magic methods __aenter__ and __aexit__)?

class HTTP:
    def __init__(self):
        pass # replace by any classic __init__ method
    async def __aenter__(self, *your_args, **your_kwargs):
        self.session = await aiohttp.ClientSession().__aenter__()
        return self
    async def __aexit__(self, *args, **kwargs):
        await self.session.__aexit__(*args, **kwargs)

Then you can just do:

async with HTTP() as http:
    http.get("http://api.mathjs.org/v4/", params={'expr' : '4^2'})

I don't know if there is a clean way to "force" users to use with syntax, but I think you can have a solution that works even if they construct the object in a synchronous way:

class HTTP:
    def __init__(self):
        self.session = None
    async def __aenter__(self, *your_args, **your_kwargs):
        self.session = await aiohttp.ClientSession().__aenter__()
        return self
    async def __aexit__(self, *args, **kwargs):
        await self.session.__aexit__(*args, **kwargs)

    # Check if there is an active session, if not use a single-request session:
    async def get(self, *args, **kwargs):
        if self.session is None:
            async with aiohttp.ClientSession() as session:
                print("One shot session.")
                return await self._get_with_session(session, *args, **kwargs)
        else:
            print("Long lasting session.")
            return await self._get_with_session(self.session, *args, **kwargs)
    async def _get_with_session(self, session, url, params):
        async with session.get(url, params=params) as response:
            return await response.text()

That way, both the following piece of code would work:

http = HTTP()
await http.get("http://api.mathjs.org/v4/", params={'expr' : '3^2'}))
await http.get("http://api.mathjs.org/v4/", params={'expr' : '4^2'})

async with HTTP() as http:
    await http.get("http://api.mathjs.org/v4/", params={'expr' : '3^2'})
    await http.get("http://api.mathjs.org/v4/", params={'expr' : '4^2'})

I feel like this is a good solution as it requires no coding effort and is probably easy to maintain as you just delegate everything to the session object.

EDIT If you want to add the active session automatically if it exists or feed a new one if it doesn't for several services (get/post/put/etc), then you should probably use a decorator to do so. If you plan on having this behaviour in other classes than HTTP, you may want to extend an abstract base class that would implement all the basic features (creating/closing the session), in the end, your classes would look like this:

import asyncio, aiohttp
import functools
from abc import ABCMeta, abstractmethod

def attach_session(method):
    @functools.wraps(method)
    async def method_with_session(self, *args, **kwargs):
        if self.session is None:
            async with aiohttp.ClientSession() as session:
                print("{} using a one shot session.".format(method.__name__))
                return await method(self, *args, session=session, **kwargs)
        else:
            print("{} using the long lasting session {}.".format(method.__name__, self.session))
            return await method(self, *args, session=self.session, **kwargs)
    return method_with_session

class AbstractSessionContainer(metaclass=ABCMeta):
    @abstractmethod
    def __init__(self):
        self.session = None
    async def __aenter__(self):
        self.session = await aiohttp.ClientSession().__aenter__()
        return self
    async def __aexit__(self, *args, **kwargs):
        await self.session.__aexit__(*args, **kwargs)

class HTTP(AbstractSessionContainer):
    def __init__(self):
        pass # If you don't have an init method I think you wont be able to create an HTTP instance
    @attach_session
    async def get(self, url, params, session=None):
        async with session.get(url, params=params) as response:
            return await response.text()
    @attach_session
    async def post(self, url, json_data, session=None):
        async with session.post(url, json=json_data) as response:
            return await response.text()

If anyone sees any improvement (even a minor one like namings) to this I would be happy to get some feedback (as I wrote these for myself too :p).

[asvetlov]

You can use sess = aiohttp.ClientSession() but should call it from async function.

[cglacet]

Do you suggest to add methods in the AbstractSessionContainer to allow users to open/close a session?

If so then here is the modified code (in the mean time I also modified to code to handle parameters on the context manager creation):

import asyncio, aiohttp
import functools
from abc import ABCMeta, abstractmethod

def attach_session(method):
    @functools.wraps(method)
    async def method_with_session(self, *args, **kwargs):
        if self.session is None:
            async with aiohttp.ClientSession() as session:
                print("{} using a one shot session.".format(method.__name__))
                return await method(self, *args, session=session, **kwargs)
        else:
            print("{} using the long lasting session {}.".format(method.__name__, self.session))
            return await method(self, *args, session=self.session, **kwargs)
    return method_with_session

class AbstractSessionContainer(metaclass=ABCMeta):
    @abstractmethod
    def __init__(self, *args, **kwargs):
        self.session = None
        self.args = args
        self.kwargs = kwargs
    async def __aenter__(self):
        self.session = await aiohttp.ClientSession(*self.args, **self.kwargs).__aenter__()
        return self
    async def __aexit__(self, *args, **kwargs):
        await self.session.__aexit__(*args, **kwargs)
    async def start_session(self, *args, **kwargs):
        self.session = aiohttp.ClientSession(*args, **kwargs)
    async def close_session(self):
        if (self.session is not None) and (not self.session.closed):
            await self.session.close()
        self.session = None

class HTTP(AbstractSessionContainer):
    def __init__(self):
        super().__init__(raise_for_status=True) # add any 'aiohttp.ClientSession' parameters here 
    @attach_session
    async def get(self, url, params, session=None):
        async with session.get(url, params=params) as response:
            return await response.text()
    @attach_session
    async def post(self, url, json_data, session=None):
        async with session.post(url, json=json_data) as response:
            return await response.text() 

This allow to have 3 different ways of working with HTTP objects, just as before the first one will use two different sessions for the two GET requests:

http = HTTP()
await http.get("http://api.mathjs.org/v4/", params={'expr' : '3^2'})
await http.get("http://api.mathjs.org/v4/", params={'expr' : '4^2'})

This doesn't change either, a single session is used for both requests:

async with HTTP() as http:
    await http.get("http://api.mathjs.org/v4/", params={'expr' : '3^2'})
    await http.get("http://api.mathjs.org/v4/", params={'expr' : '4^2'})

You can also mimic the async with statement by calling start_session and close_session yourself (here a single session is used too):

http = HTTP()
await http.start_session()
await http.get("http://api.mathjs.org/v4/", params={'expr' : '3^2'})
await http.get("http://api.mathjs.org/v4/", params={'expr' : '4^2'})
await http.close_session()

[asvetlov]

@cglacet the answer depends on requirements.
You snippet looks complicated for me but maybe you have a reason to support all three ways in parallel.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a [new issue] for related bugs.
If you feel like there's important points made in this discussion, please include those exceprts into that [new issue].
[new issue]: https://github.com/aio-libs/aiohttp/issues/new