generated async code and tests, with 100% async test coverage. #49

Merged
merged 5 commits into from Apr 18, 2016

Projects

None yet

4 participants

@escapewindow
Contributor

@jhford , @jonasfj , @djmitche , anyone else interested:

This PR adds async support, with generated code+tests.
I'm going to guess this will be another lengthy review cycle, so I'll be working on the generic python worker lib, based on this async client, in the meantime.

The async tests test single calls, as well as multiple async calls to verify the asynchronicity of the calls. All the async code has 100% test coverage.

The pattern I used for the multi-async tests was to have two calls. The first one fails and retries; the second call should be unblocked and happen before the first retries. This works well, and I mock.patched utils.calculateSleepTime to minimize any wait time.

Here are some comments about various files' changes. A lot has changed, so I may not have covered everything. Let me know if you have any questions.

  • Makefile: I have lint running for both py3 and py2, with py2 skipping the async files. make lint now runs on both py35 and py27 without errors after make clean.
  • genCode.py: we now generate code+tests for sync and async.
  • setup.py: Added nose-exclude to skip async tests in py2; coverage < 4.1 dies on async code https://bitbucket.org/ned/coveragepy/issues/434/indexerror-in-python-35
  • taskcluster/async/[A-Z]*: generated async classes. These all use async def and await syntax, so they will not be drop-in replacements for the synchronous classes.
  • taskcluster/baseclient.py: I took the requests-specific calls out of BaseClient and moved them to SyncClient.
  • taskcluster/sync/syncclient.py: the requests-, synchronous, blocking methods are here. RuntimeClient inherits SyncClient.
  • taskcluster/async/asyncclient.py: This defines the async methods with aiohttp and asyncio instead of requests. I shared as much code as I could. There's some makeHttpRequest code that's very similar, but I couldn't have a shared makeHttpRequest calling a retryHttpRequest because one needed to be a def, returning the retryHttpRequest, and the other an async def, return awaiting the retryHttpRequest.
  • taskcluster/runtimeclient.py: I removed a duplicate createTemporaryCredentials
  • taskcluster/asyncutils.py: This defines the async methods with aiohttp and asyncio instead of requests. This file was going to live under taskcluster/async, but because taskcluster.async imports some taskcluster.async.* objects which import asyncutils, that created a circular dependency. I was also thinking about pulling the synchronous code out of taskcluster.utils into taskcluster.syncutils, but that may break existing code, so I left it.
  • test/async/test_async_integration.py: I added integration tests for the async Auth class. The httmock no_creds_needed needed aiohttpifying (httmock is requests-specific), and I added a multi-async call test.
  • test/async/test_asyncclient.py and test/async/test_asyncutils.py: I wanted to get to 100% coverage on the async code; these helped get there.
  • test/base/__init__.py and test/async/__init__.py: I fleshed these out further. I made BaseAuthentication async-override-friendly.
  • test/test_integration.py: I wanted to split this out of test_client.py earlier, and did so this pass.
@escapewindow
Contributor

https://github.com/escapewindow/taskcluster-client.py/commits/async-dev has the unedited, un-squashed commits if that helps at all.

@djmitche djmitche commented on the diff Apr 1, 2016
genCode.py
+ "codeDir": os.path.join(BASE_DIR, 'taskcluster', "sync"),
+ "testDir": os.path.join(BASE_DIR, 'test'),
+ "testTemplate": "test.template",
+ "defStatement": "def",
+ "returnStatement": "return",
+ },
+ "async": {
+ "clientModule": "taskcluster.async.asyncclient",
+ "clientClass": "AsyncClient",
+ "codeDir": os.path.join(BASE_DIR, 'taskcluster', "async"),
+ "testDir": os.path.join(BASE_DIR, 'test', "async"),
+ "testTemplate": "async_test.template",
+ "returnStatement": "return await",
+ "defStatement": "async def",
+ },
+}
@djmitche
djmitche Apr 1, 2016 Contributor

This is really cool

@djmitche
Contributor
djmitche commented Apr 1, 2016

As far as I'm concerned, this looks great!

@jonasfj jonasfj commented on the diff Apr 1, 2016
taskcluster/async/asyncclient.py
+import taskcluster.baseclient as baseclient
+import taskcluster.utils as utils
+import taskcluster.asyncutils as asyncutils
+
+log = logging.getLogger(__name__)
+
+
+class AsyncClient(baseclient.BaseClient):
+ """Async class for API Client Classes. Each individual Client class
+ needs to set up its own methods for REST endpoints and Topic Exchange
+ routing key patterns.
+ """
+
+ @contextmanager
+ def contextSession(self, *args, **kwargs):
+ yield self.createSession(*args, **kwargs)
@jonasfj
jonasfj Apr 1, 2016 Member

So this defaults to one global session by default right? No matter how many client objects I create they all share the same session?
I suspect so, I just want to be sure :)

Maybe a nice to have would be a constructor for AsyncClient that accepts a session... But maybe that's a thing we add if it's ever needed..

@escapewindow
escapewindow Apr 1, 2016 Contributor

It does accept a session, then by default uses that session throughout.
The makeHttpRequest method allows you to override that session.

@jonasfj jonasfj commented on the diff Apr 1, 2016
taskcluster/async/asyncclient.py
+import taskcluster.asyncutils as asyncutils
+
+log = logging.getLogger(__name__)
+
+
+class AsyncClient(baseclient.BaseClient):
+ """Async class for API Client Classes. Each individual Client class
+ needs to set up its own methods for REST endpoints and Topic Exchange
+ routing key patterns.
+ """
+
+ @contextmanager
+ def contextSession(self, *args, **kwargs):
+ yield self.createSession(*args, **kwargs)
+
+ def createSession(self, session=None, args=(), kwargs=None):
@jonasfj
jonasfj Apr 1, 2016 Member

Nit, but wouldn't it be preferable to prefix these methods with an underscore, just so it's clear that these are intended to be used internally. Ideally, it would also make docs better...
(I'm not python expert, so I defer to you)

@escapewindow
escapewindow Apr 1, 2016 Contributor

When using pylint (we don't here), it throws an error whenever an external object calls a single underscore prefixed method, because those are "private". This causes enough headaches for tests that I default to open unless there's a reason for making something private.

Since we don't use pylint here, I don't have a strong opinion, but still default to open.

@jhford
jhford Apr 8, 2016 Contributor

since we don't use pylint in this tool, i think we shouldn't concern ourselves with placating it.

@jonasfj
Member
jonasfj commented Apr 1, 2016

From my quick skim this seems solid! :)

@escapewindow
Contributor

@catlee pointed out that I can simplify most of my asyncio.run_until_complete() calls (I don't always need a helper async function). Neatened up in the latest commit.

@escapewindow
Contributor

@jhford , do you have any questions so far?

@jhford
Contributor
jhford commented Apr 8, 2016

I don't have a ton of experience with async python, but I can't find anything major wrong with the async portions. @djmitche do you know async python well? If so, and you think this looks good, I'm happy to merge. For the remaining portions of the client, I think the changes look good. I like that the CODE_DEF thing that lets you share the template.

@djmitche
Contributor
djmitche commented Apr 8, 2016

I am not particularly familiar with async python, no (well, not the new kind.. I know Twisted Deferreds far better than I'd like!)

I don't see any issues that would result from shipping this as-is.

@escapewindow
Contributor

@rail ported https://github.com/mozilla/tctalker to use the new async code, and found that async auth calls fail with a bad hash. I'm not sure why they fail when the integration tests work.

We're trying to debug. One theory is that when aiohttp changes headers [1] [2] [3] after we run makeHeaders, that breaks the hash, but I'm not 100% sure that's it. We've debated dropping the hash; the java client already does this https://github.com/taskcluster/taskcluster-client-java/blob/6e6a11f5d391b907d4630957b217d9fd84649841/src/main/java/org/mozilla/taskcluster/client/TaskClusterRequestHandler.java#L111-L114

Dropping Authorization from the headers completely passes auth, but with no scopes. Dropping the hash from the Authorization text block results in 'bad mac'. I think I need to dig to find precisely what's breaking here.

I currently suspect https://github.com/KeepSafe/aiohttp/blob/master/aiohttp/client_reqrep.py#L265-274 .

[1] https://github.com/KeepSafe/aiohttp/blob/master/aiohttp/client_reqrep.py#L89-96
[2] https://github.com/KeepSafe/aiohttp/blob/master/aiohttp/client_reqrep.py#L185-L210
[3] https://github.com/KeepSafe/aiohttp/blob/master/aiohttp/client_reqrep.py#L261-338

@escapewindow
Contributor

We got auth working when I prevented aiohttp from changing the content-type and from using compression (which can then lead to chunking).

I also fixed the debug logging of the response text.

@escapewindow
Contributor

Are we waiting on anything here before merge?

@djmitche
Contributor

All of the comments seem positive :)

@jhford jhford merged commit 0b48b36 into taskcluster:master Apr 18, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment