async HTTP component #3914

balloob · 2016-10-17T04:36:38Z

I have been working on porting over the HTTP component to use aiohttp instead of CherryPy WSGI server + Werkzeug. Once this is done, adding a websocket server is pretty much free too 👍

I had to enhance some internal aiohttp classes to get gzip / md5 fingerprinting / caching working. I am planning to see if I can get support for this merged upstream.

Features to add back:

CORS - docs - Ugh: Class based views are not supported aio-libs/aiohttp-cors#41
SSL docs
Static resource serving (temp workaround withoug gzip support is in place)
Supporting the cache busting MD5 fingerprints in static resources
Fix API Stream
Fix event forwarder
Fix API error log

Components to fix:

mention-bot · 2016-10-17T04:36:40Z

@balloob, thanks for your PR! By analyzing the history of the files in this pull request, we identified @fabaff, @robbiet480 and @JshWright to be potential reviewers.

balloobbot · 2016-10-17T04:37:18Z

homeassistant/components/api.py



 class APIEntityStateView(HomeAssistantView):
    """View to handle EntityState requests."""

-    url = "/api/states/<entity(exist=False):entity_id>"
+    url = "/api/states/{entity_id}"  # TODO validation <entity(exist=False):entity_id>


farcy v1.1

80: E501 line too long (86 > 79 characters)

balloobbot · 2016-10-17T04:37:18Z

homeassistant/components/http.py

-    def stop_wsgi_server(event):
-        """Stop the WSGI server."""
-        server.stop()
+    def start_server(event):


farcy v1.1

5: F811 redefinition of unused 'start_server' from line 138

balloobbot · 2016-10-17T04:37:19Z

homeassistant/components/http.py

+    #         if cors_check:
+    #             response.headers[HTTP_HEADER_ACCESS_CONTROL_ALLOW_ORIGIN] = \
+    #                 environ.get('HTTP_ORIGIN')
+    #             response.headers[HTTP_HEADER_ACCESS_CONTROL_ALLOW_HEADERS] = \


farcy v1.1

80: E501 line too long (80 > 79 characters)

balloobbot · 2016-10-17T04:37:19Z

homeassistant/components/http.py

+        # TODO CORS ?
+        return web.Response('', status=200)
+
+    def __call__(self, request):


farcy v1.1

D102: Missing docstring in public method

balloob · 2016-10-17T04:37:23Z

I am planning on adding aiohttp to the core dependencies, just like requests is.

balloobbot · 2016-10-17T04:39:24Z

homeassistant/components/http.py

-    def stop_wsgi_server(event):
-        """Stop the WSGI server."""
-        server.stop()
+    def start_server(event):


farcy v1.1

5: F811 redefinition of unused 'start_server' from line 137

balloobbot · 2016-10-17T07:43:30Z

homeassistant/components/http.py


+    @asyncio.coroutine


farcy v1.1

5: E303 too many blank lines (2)

balloobbot · 2016-10-17T08:27:03Z

homeassistant/components/http.py


+from homeassistant.core import callback, is_callback


farcy v1.1

1: F401 'callback' imported but unused

balloobbot · 2016-10-18T22:11:55Z

homeassistant/components/ffmpeg.py


    if FFMPEG_CONFIG.get(CONF_RUN_TEST):
        # if in cache
        if input_source in FFMPEG_TEST_CACHE:
            return FFMPEG_TEST_CACHE[input_source]

        # run test
-        test = Test(get_binary())
-        if not test.run_test(input_source):
+        ffmpeg_test = Test(get_binary())


farcy v1.1

23: F821 undefined name 'Test'

balloobbot · 2016-10-19T06:19:52Z

homeassistant/components/http.py

-                raise BadRequest('Unable to read JSON request')
-
-    return Request
+class GzipFileSender(FileSender):


farcy v1.1

D204: 1 blank line required after class docstring (found 0)

balloob · 2016-10-19T06:20:25Z

Besides the camera view, this is ready for review.

@bbangert, when you have time, could you take a look. Thanks.

balloobbot · 2016-10-19T13:07:54Z

homeassistant/components/camera/__init__.py

+                    break
+                response.write(data)
+            # pylint: disable=broad-except
+            except Exception as err:


farcy v1.1

13: F841 local variable 'err' is assigned to but never used

balloobbot · 2016-10-19T13:07:54Z

homeassistant/components/camera/__init__.py

+                response.write(data)
+            # pylint: disable=broad-except
+            except Exception as err:
+                _LOGGER.debug("Exception on stream sending.", exc_info=True)


farcy v1.1

17: F821 undefined name '_LOGGER'

balloobbot · 2016-10-19T14:34:22Z

homeassistant/components/camera/__init__.py

+        """Stream images as mjpeg stream."""
+        img_bytes = yield from self.entity.async_camera_image()
+
+        if img_bytes is not None and img_bytes != last_image:


farcy v1.1

51: F821 undefined name 'last_image'

balloobbot · 2016-10-19T14:34:22Z

homeassistant/components/camera/__init__.py

+        img_bytes = yield from self.entity.async_camera_image()
+
+        if img_bytes is not None and img_bytes != last_image:
+            img_bytes =  bytes(


farcy v1.1

24: E222 multiple spaces after operator

balloobbot · 2016-10-19T14:55:51Z

homeassistant/components/camera/__init__.py

+                response.write(data)
+            # pylint: disable=broad-except
+            except Exception:
+                _LOGGER.debug("Exception on stream sending.", exc_info=True)


farcy v1.1

17: F821 undefined name '_LOGGER'

balloobbot · 2016-10-19T20:17:55Z

homeassistant/components/camera/mjpeg.py

+        # connect to stream
+        try:
+            with async_timeout.timeout(10, loop=self.hass.loop):
+                stream = yield from session.get(self._mjpeg_url)


farcy v1.1

37: F821 undefined name 'session'

balloobbot · 2016-10-19T21:38:13Z

homeassistant/components/camera/generic.py

        else:
            self._auth = None

        self._last_url = None
        self._last_image = None
+        self._session = aiohttp.ClientSession(loop=hass.loop, auth=auth)


farcy v1.1

68: F821 undefined name 'auth'

balloobbot · 2016-10-19T21:38:14Z

homeassistant/components/camera/generic.py


    def camera_image(self):
+        """Return bytes of camera image."""
+        return run_coroutine_threadsafe(


farcy v1.1

16: F821 undefined name 'run_coroutine_threadsafe'

balloobbot · 2016-10-19T21:58:57Z

homeassistant/components/camera/generic.py

@@ -4,10 +4,14 @@
 For more details about this platform, please refer to the documentation at
 https://home-assistant.io/components/camera.generic/
 """
+import asyncio
+import functools as ft


farcy v1.1

1: F401 'ft' imported but unused

pvizeli · 2016-10-19T22:11:43Z

So my work on camera component and his platform is done 💯

balloobbot · 2016-10-23T19:27:46Z

tests/conftest.py

+        return self._requests[key]
+
+
+class AiohttpClientMockResponse:


farcy v1.1

D204: 1 blank line required after class docstring (found 0)

balloobbot · 2016-10-23T19:27:46Z

tests/conftest.py

+
+class AiohttpClientMockResponse:
+    """Mock Aiohttp client response."""
+    def __init__(self, method, url, status, response):


farcy v1.1

D102: Missing docstring in public method

balloobbot · 2016-10-23T19:27:46Z

tests/conftest.py

+
+
+@pytest.fixture
+def aioclient_mock():


farcy v1.1

D202: No blank lines allowed after function docstring (found 1)

bbangert

Looks good overall. Left a few questions that may or may not be actual issues, hard to hold all the context at once. It might be nice to clarify whether async_ means its async safe, or is actually a @callback.

bbangert · 2016-10-24T03:15:49Z

homeassistant/components/api.py


        return self.json_message("Event forwarding setup.")

+    @asyncio.coroutine
    def delete(self, request):
        """Remove event forwarer."""


forwarder perhaps?

bbangert · 2016-10-24T03:17:00Z

homeassistant/components/api.py

@@ -364,6 +398,7 @@ class APIErrorLogView(HomeAssistantView):
    url = URL_API_ERROR_LOG
    name = "api:error-log"

+    @asyncio.coroutine
    def get(self, request):
        """Serve error log."""
        return self.file(request, self.hass.config.path(ERROR_LOG_FILENAME))


A file request that doesn't block?

bbangert · 2016-10-24T03:26:52Z

homeassistant/components/camera/__init__.py

+
+        def write(img_bytes):
+            """Write image to stream."""
+            response.write(bytes(


Nonblocking call?

It's nonblocking: http://aiohttp.readthedocs.io/en/stable/web_reference.html#aiohttp.web.StreamResponse.write

Instead, we yield from response.drain()

Just read 2 comments below 👍 we're all good

bbangert · 2016-10-24T03:27:17Z

homeassistant/components/camera/__init__.py

+                    break
+
+                if img_bytes is not None and img_bytes != last_image:
+                    write(img_bytes)


I would've expected a yield to be able to write to a socket under async.

bbangert · 2016-10-24T03:28:01Z

homeassistant/components/camera/__init__.py

+                        write(img_bytes)
+
+                    last_image = img_bytes
+                    yield from response.drain()


If this is the call that waits for a write buffer to empty, ignore the above comments about waiting on the write.

I should have read all the comments before commenting 🎉

bbangert · 2016-10-24T04:24:39Z

homeassistant/components/frontend/__init__.py

@@ -193,9 +201,10 @@ def __init__(self, hass, extra_urls):
            )
        )

+    @asyncio.coroutine


A coroutine but nothing needs to yield?

bbangert · 2016-10-24T04:25:11Z

homeassistant/components/frontend/__init__.py

@@ -230,7 +239,7 @@ def get(self, request, entity_id=None):
            icons_url=icons_url, icons=FINGERPRINTS['mdi.html'],


async_render?

This is a jinja2 template instance, not a HASS instance.

bbangert · 2016-10-24T04:45:59Z

homeassistant/components/http.py


-            return parsed
+        finally:
+            resp.set_tcp_nodelay(True)


Where is it set to False?

This code is a direct copy of the aiohttp code. I am planning on contributing it back to them. For now this is a direct copy of their code.

set_tcp_nodelay will reverse the set_tcp_cork call a few lines above. Docs.

bbangert · 2016-10-24T04:53:19Z

homeassistant/components/sensor/torque.py

@@ -100,14 +102,14 @@ def get(self, request):
            elif is_value:
                pid = convert_pid(is_value.group(1))
                if pid in self.sensors:
-                    self.sensors[pid].on_update(data[key])
+                    self.sensors[pid].async_on_update(data[key])


Guaranteed to not be a coroutine?

I've added a callback annotation to the method for clarity

bbangert · 2016-10-24T04:55:26Z

homeassistant/remote.py


-    def disconnect(self, api):
+    def async_disconnect(self, api):


Are these callbacks or safe from to call from any thread?

Callbacks. Added annotation.

They are only called from the APi when setting up/deleting event forwarding.

balloob · 2016-10-24T06:42:59Z

@bbangert you found more than I would have expected. It's all fixed now 👯‍♂️

balloob added the async label Oct 17, 2016

balloobbot reviewed Oct 17, 2016

View reviewed changes

homeassistant/components/http.py

@asyncio.coroutine

Copy link

balloobbot Oct 17, 2016

Choose a reason for hiding this comment

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

farcy v1.1

5: E303 too many blank lines (2)

balloobbot reviewed Oct 17, 2016

View reviewed changes

homeassistant/components/http.py

from homeassistant.core import callback, is_callback

Copy link

balloobbot Oct 17, 2016

Choose a reason for hiding this comment

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

farcy v1.1

1: F401 'callback' imported but unused

robbiet480 added the Hacktoberfest label Oct 17, 2016

balloob force-pushed the aiohttp branch from 238ff2a to 8f26311 Compare October 18, 2016 02:49

balloob mentioned this pull request Oct 18, 2016

Fix starting of remote slaves. #3681

Closed

8 tasks

balloobbot reviewed Oct 18, 2016

View reviewed changes

balloob force-pushed the aiohttp branch from b14c9a6 to 2c8e3ba Compare October 19, 2016 01:47

balloob changed the title ~~WIP: async HTTP component~~ async HTTP component Oct 19, 2016

balloobbot reviewed Oct 19, 2016

View reviewed changes

balloob assigned bbangert Oct 19, 2016

balloobbot reviewed Oct 19, 2016

View reviewed changes

balloob force-pushed the aiohttp branch from cbe1db7 to af4d2d3 Compare October 21, 2016 06:57

balloob force-pushed the aiohttp branch from af4d2d3 to c552336 Compare October 23, 2016 13:07

balloobbot reviewed Oct 23, 2016

View reviewed changes

tests/conftest.py

@pytest.fixture

def aioclient_mock():

Copy link

balloobbot Oct 23, 2016

Choose a reason for hiding this comment

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

farcy v1.1

D202: No blank lines allowed after function docstring (found 1)

balloob force-pushed the aiohttp branch from 6db9901 to db9b7e8 Compare October 23, 2016 21:35

bbangert approved these changes Oct 24, 2016

View reviewed changes

balloob added 11 commits October 23, 2016 23:30

Migrate WSGI to asyncio

b561569

Rename wsgi -> http

940c1a8

Python 3.4 compat

dbc8ad3

Move linting to Python 3.4

005c40b

lint

3baef16

Lint

3874392

Fix Python 3.4 mock_open + binary data

87bf054

Surpress logging aiohttp.access

756c491

Spelling

9e0c9ba

Sending files is a coroutine

633cf5b

More callback annotations and naming fixes

2fb88eb

balloob force-pushed the aiohttp branch from 7e4f6a1 to 2fb88eb Compare October 24, 2016 06:31

Fix ios

b279190

balloob merged commit 519d9f2 into dev Oct 24, 2016

balloob deleted the aiohttp branch October 24, 2016 06:48

Chris-V mentioned this pull request Nov 11, 2016

Log file not updating on dev-info page #4308

Closed

home-assistant locked and limited conversation to collaborators Mar 17, 2017

		@@ -230,7 +239,7 @@ def get(self, request, entity_id=None):
		icons_url=icons_url, icons=FINGERPRINTS['mdi.html'],

async HTTP component #3914

async HTTP component #3914

Conversation

balloob commented Oct 17, 2016 • edited by pvizeli Loading

mention-bot commented Oct 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

balloob commented Oct 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

balloob commented Oct 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvizeli commented Oct 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbangert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

balloob commented Oct 24, 2016

balloob commented Oct 17, 2016 •

edited by pvizeli

Loading