uaiohttp: Close the socket to resolve memory leak and instability

psclafer commented 5 years ago

There is no method to close the Reponse. This leads to a memory leak by filling uasyncio select pool and preventing the GC to clean the sockets and related uasyncio data: Example:

import uaiohttpclient as aiohttp
import uasyncio
from itertools import count

def http_get_coro():
   resp = yield from aiohttp.request("GET","http://perdu.com")
   line = yield from resp.read()
   print(line)

   #yield from resp.aclose()   #Does not exist

def loop_coro():
   for i in count():
      print("------ {:02d} ------".format(i))
      yield from uasyncio.sleep_ms(100)
      yield from http_get_coro()
      print("----------------\n")

l = uasyncio.get_event_loop()
l.create_task(loop_coro())
l.run_forever()

------ 00 ------
b"<html><head><title>Vous Etes Perdu ?</title></head><body><h1>Perdu sur l'Internet ?</h1><h2>Pas de panique, on va vous aider</h2><strong><pre>    * <----- vous &ecirc;tes ici</pre></strong></body></html>\n"
----------------

------ 01 ------
b"<html><head><title>Vous Etes Perdu ?</title></head><body><h1>Perdu sur l'Internet ?</h1><h2>Pas de panique, on va vous aider</h2><strong><pre>    * <----- vous &ecirc;tes ici</pre></strong></body></html>\n"
----------------

------ 02 ------
b"<html><head><title>Vous Etes Perdu ?</title></head><body><h1>Perdu sur l'Internet ?</h1><h2>Pas de panique, on va vous aider</h2><strong><pre>    * <----- vous &ecirc;tes ici</pre></strong></body></html>\n"
--------------b"<html><head><title>Vous Etes Perdu ?</title></head><body><h1>Perdu sur l'Internet ?</h1><h2>Pas de panique, on va vous aider</h2><strong><pre>    * <----- vous &ecirc;tes ici</pre></strong></body></html>\n"
----------------

...
...
...

------ 43 ------
b"<html><head><title>Vous Etes Perdu ?</title></head><body><h1>Perdu sur l'Internet ?</h1><h2>Pas de panique, on va vous aider</h2><strong><pre>    * <----- vous &ecirc;tes ici</pre></strong></body></html>\n"
----------------

------ 44 ------
Traceback (most recent call last):
  File "<stdin>", line 31, in <module>
  File "uasyncio/core.py", line 185, in run_forever
  File "uasyncio/core.py", line 183, in run_forever
  File "uasyncio/core.py", line 181, in run_forever
  File "uasyncio/core.py", line 156, in _run_forever
  File "uasyncio/core.py", line 111, in _run_forever
  File "<stdin>", line 25, in loop_coro
  File "<stdin>", line 14, in http_get_coro
  File "uaiohttpclient.py", line 74, in request
  File "uaiohttpclient.py", line 65, in request_raw
  File "uasyncio/__init__.py", line 171, in awrite
OSError: [Errno 12] ENOMEM
>>> print(l.objmap)
{1073687488: <generator object 'loop_coro' at 3fff2400>, 1073683088: <generator object 'loop_coro' at 3fff2400>, 1073686144: <generator object 'loop_coro' at 3fff2400>, 1073689152: <generator object 'loop_coro' at 3fff2400>, 1073684640: <generator object 'loop_coro' at 3fff2400>, 1073687040: <generator object 'loop_coro' at 3fff2400>, 1073677168: <generator object 'loop_coro' at 3fff2400>, 1073687088: <generator object 'loop_coro' at 3fff2400>, 1073692448: <generator object 'loop_coro' at 3fff2400>, 1073688032: <generator object 'loop_coro' at 3fff2400>, 1073692640: <generator object 'loop_coro' at 3fff2400>, 1073685920: <generator object 'loop_coro' at 3fff2400>, 1073691936: <generator object 'loop_coro' at 3fff2400>, 1073686768: <generator object 'loop_coro' at 3fff2400>, 1073682304: <generator object 'loop_coro' at 3fff2400>, 1073677040: <generator object 'loop_coro' at 3fff2400>, 1073690624: <generator object 'loop_coro' at 3fff2400>, 1073684704: <generator object 'loop_coro' at 3fff2400>, 1073689072: <generator object 'loop_coro' at 3fff2400>, 1073686816: <generator object 'loop_coro' at 3fff2400>, 1073690864: <generator object 'loop_coro' at 3fff2400>, 1073690912: <generator object 'loop_coro' at 3fff2400>, 1073691616: <generator object 'loop_coro' at 3fff2400>, 1073692512: <generator object 'loop_coro' at 3fff2400>, 1073691056: <generator object 'loop_coro' at 3fff2400>, 1073688096: <generator object 'loop_coro' at 3fff2400>, 1073691664: <generator object 'loop_coro' at 3fff2400>, 1073687440: <generator object 'loop_coro' at 3fff2400>, 1073685984: <generator object 'loop_coro' at 3fff2400>, 1073689408: <generator object 'loop_coro' at 3fff2400>, 1073686080: <generator object 'loop_coro' at 3fff2400>, 1073688384: <generator object 'loop_coro' at 3fff2400>, 1073686928: <generator object 'loop_coro' at 3fff2400>, 1073688480: <generator object 'loop_coro' at 3fff2400>, 1073682496: <generator object 'loop_coro' at 3fff2400>, 1073690080: <generator object 'loop_coro' at 3fff2400>, 1073691008: <generator object 'loop_coro' at 3fff2400>, 1073690176: <generator object 'loop_coro' at 3fff2400>, 1073691728: <generator object 'loop_coro' at 3fff2400>, 1073683536: <generator object 'loop_coro' at 3fff2400>, 1073688432: <generator object 'loop_coro' at 3fff2400>, 1073683024: <generator object 'loop_coro' at 3fff2400>, 1073689920: <generator object 'loop_coro' at 3fff2400>, 1073684576: <generator object 'loop_coro' at 3fff2400>,12: <generator object 'loop_coro' at 3fff2400>}

Adding the method aclose() in ClientReponse resolve the issue. (Commit 049c79e)

But, in case of an exceptions during request handling, uaiohttp has a serious issue. A typical example is the usage of wait_for with a slow HTTP server. Here the server is tuned to take 1.0s between -accepting the connection/receiving the request- and -send back the first http header.

import uaiohttpclient as aiohttp
import uasyncio
from itertools import count

def http_get_coro():
   resp = None
   try:
      resp = yield from aiohttp.request("GET","http://192.168.31.15/current")
      line = yield from resp.read()
      print(line)
   except uasyncio.TimeoutError:
      print("RESPONSE TIMEOUT")
   finally:
      if resp:
         yield from resp.aclose()

def loop_coro():
    for i in count():
       print("------ {:02d} ------".format(i))
       yield from uasyncio.sleep_ms(2000)
       yield from uasyncio.wait_for(http_get_coro(),0.8)
       ### NOTE: TEST CASE: Here, this particular server is too slow to answer in 0.8s 

       print("----------------\n")

l = uasyncio.get_event_loop()
l.create_task(loop_coro())
l.run_forever()

In that case, the poller wakes up uasyncio for a canceled request while the next request or the next sleep has been started. uasyncio becomes unstable with an unpredictable behavior leading to a crash of the loop. This use case is absolutely necessary for production systems. The second commit (2a5c8cc) handle theses cases by closing the reader and the socket when an exception occurs during processing.

I'm not sure that theses patchs are prefect, but I think it's a big improvement for this module, to be used for real.

`

pfalcon commented 5 years ago

Thanks for the patch!

Looking at the commit messages:

uaiohttp: Add aclose() method to ClientResponse

While patch actually patches uaiohttpclient.py. What goes into prefix:, should be exact module name (uaiohttpclient), to avoid any confusion.

Speaking of that, how the original uaiohttp deals with that?

Otherwise, I agree with the nature of the patches and argumentation. Similar issues were already handled in other HTTP module(s), e.g. 586ae64cb0f2afa7ef26aec360cb8b09948fd8f5 . But the actual way to do that may require further consideration, will review in more detail when get a chance.

psclafer commented 5 years ago

Speaking of that, how the original uaiohttp deals with that?

The original aiohttp ClientResponse has a close() method. But this can be handled using a context manager.

Example taken from the aiohttp documentation:

async with aiohttp.ClientSession() as session:
    async with session.get('http://httpbin.org/get') as resp:
        print(resp.status)
        print(await resp.text())

pfalcon commented 5 years ago

Thanks for the report, uaiohttpclient.ClientResponse.aclose() method was added (from scratch, to be sure of the correctness of implementation).

pfalcon / pycopy-lib

uaiohttp: Close the socket to resolve memory leak and instability #11