Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement support for asyncio. #251

Merged
merged 4 commits into from Nov 24, 2016

Conversation

@mvantellingen
Copy link
Owner

commented Nov 6, 2016

This adds an optional AsyncTransport class which only works for
Python 3.5. Code is heavily inspired by the pull-request from
chrisimcevoy (see #207)

@mvantellingen

This comment has been minimized.

Copy link
Owner Author

commented Nov 6, 2016

@chrisimcevoy let me know if you have feedback!

@codecov-io

This comment has been minimized.

Copy link

commented Nov 6, 2016

Current coverage is 93.32% (diff: 71.42%)

Merging #251 into master will decrease coverage by 0.17%

@@             master       #251   diff @@
==========================================
  Files            39         39          
  Lines          3786       3775    -11   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits           3540       3523    -17   
- Misses          246        252     +6   
  Partials          0          0          
Diff Coverage File Path
•••••• 66% src/zeep/wsdl/wsdl.py
•••••••••• 100% src/zeep/transports.py

Powered by Codecov. Last update 773a68c...e70f5ce

@chrisimcevoy

This comment has been minimized.

Copy link

commented Nov 7, 2016

Hey @mvantellingen - Good stuff! I wasn't expecting to see this so soon! Your implementation is a lot cleaner in the way you separated the asyncio stuff into its own module. I like the example too - the direct comparison between sync and async is striking.

I haven't had a lot of time to play with this PR yet, but at first glance I have two comments if you don't mind?

i) AsyncTransport._load_remote_data() uses a synchronous GET via requests.Session(), the same as the regular zeep.Transport()... It seems undesirable to have a blocking operation there if the user expects concurrency? In other words, while you can call operations asynchronously once the Client is instantiated, it isn't possible for the user to instantiate multiple Clients/AsyncTransports concurrently with this code; You would have to wait for each AsyncTransport to fetch it's WSDL one after the other, which sounds counter-intuitive.

In #207 this was possible via the Client.create() classmethod/coroutine (although that classmethod maybe looks a little garish in hindsight), for example:

clients = []

def append_clients(fut):
    for client in fut.result():
        clients.append(client)

wsdls = [
    # list of wsdl uri or path
]

tasks = []

for wsdl in wsdls:
     tasks.append(client.create(wsdl))

fut = asyncio.gather(tasks)
fut.add_done_callback(append_clients)

loop = asyncio.get_event_loop()
loop.run_until_complete(fut)
loop.close()

ii) I don't think that requests.auth.HTTPBasicAuth() and aiohttp.BasicAuth() are interchangeable. As far as I can tell, passing an http_auth kwarg to this PR's AsyncTransport will result in one of two errors because the same value is used for both the AsyncTransport._load_session (requests) and the AsyncTransport.session (aiohttp). I did an implicit conversion on this in #207 inside of AsyncTransport's init() to avoid confusion for end users who are accustomed to zeep's synchronous, requests-based API.

Those points aside, very excited to see this going somewhere!

@mvantellingen

This comment has been minimized.

Copy link
Owner Author

commented Nov 7, 2016

Hey @mvantellingen - Good stuff! I wasn't expecting to see this so soon! Your implementation is a lot cleaner in the way you separated the asyncio stuff into its own module. I like the example too - the direct comparison between sync and async is striking.

Thanks, wanted to play a bit with the async io stuff after a Django under the hood presentation last friday. :-)

I haven't had a lot of time to play with this PR yet, but at first glance I have two comments if you don't mind?

I actually really appreciate the feedback!

i) AsyncTransport._load_remote_data() uses a synchronous GET via requests.Session(), the same as the regular zeep.Transport()... It seems undesirable to have a blocking operation there if the user expects concurrency? In other words, while you can call operations asynchronously once the Client is instantiated, it isn't possible for the user to instantiate multiple Clients/AsyncTransports concurrently with this code; You would have to wait for each AsyncTransport to fetch it's WSDL one after the other, which sounds counter-intuitive.

In #207 this was possible via the Client.create() classmethod/coroutine (although that classmethod maybe looks a little garish in hindsight), for example:

clients = []

def append_clients(fut):
for client in fut.result():
clients.append(client)

wsdls = [
# list of wsdl uri or path
]

tasks = []

for wsdl in wsdls:
tasks.append(client.create(wsdl))

fut = asyncio.gather(tasks)
fut.add_done_callback(append_clients)

loop = asyncio.get_event_loop()
loop.run_until_complete(fut)
loop.close()

So my idea was to first only offer async during regular operations. This minimises the impact on the codebase as a whole and still offers a number of benefits. The client should be thread safe so creating the client once and reusing it in coroutines shouldn't be an problem (as far as I can tell now, my experience with asyncio is very limited).

However that said, I will go through your code again to see if making the load() operations async aware is possible. Otherwise this might be a good step as long as the limitiations are documented clearly.

ii) I don't think that requests.auth.HTTPBasicAuth() and aiohttp.BasicAuth() are interchangeable. As far as I can tell, passing an http_auth kwarg to this PR's AsyncTransport will result in one of two errors because the same value is used for both the AsyncTransport._load_session (requests) and the AsyncTransport.session (aiohttp). I did an implicit conversion on this in #207 inside of AsyncTransport's init() to avoid confusion for end users who are accustomed to zeep's synchronous, requests-based API.

Ok, yeah makes sense. The http_auth is now used for both aiohttp and requests. So maybe the best solution is to use the aiohttp for synchronous requests (see point 1)? Is this even possible? I'll try to get a better grasp of the module.

Those points aside, very excited to see this going somewhere!

Yeah, I think it's quite cool and allows you to easily fanout multiple requests

mvantellingen added some commits Nov 6, 2016

Implement support for asyncio.
This adds an optional AsyncTransport class which only works for
Python 3.5. Code is heavily inspired by the pull-request from
chrisimcevoy (see #207)
Use aiohttp to fetch external wsdl/xsd files.
This is for now a blocking call to make the integration easier.

@mvantellingen mvantellingen merged commit 80ba6aa into master Nov 24, 2016

1 of 5 checks passed

continuous-integration/appveyor/branch Waiting for AppVeyor build to complete
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
dependency-ci Dependencies checked
Details

@mvantellingen mvantellingen deleted the asyncio branch Nov 24, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.