aiohttp much slower and far more errors than grequests

[mcarans]

Long story short

I have two versions of my code, one grequests and one aiohttp. I think I have set them up the same, but aiohttp is very slow and connections fail far more often. I can only think that I must have something wrong for the results to be so different.

Expected behaviour

aiohttp performance and request success/failure to be on a par with grequests

Actual behaviour

aiohttp is very slow and connections fail far more often than grequests
grequests:
Run 1:
INFO - 2016-12-01 09:53:21 - __main__ - Execution time: 20.83988046646118 seconds
INFO - 2016-12-01 09:53:21 - __main__ - Have Last-Modified: 87, Hashed: 9, Number Failed: 4
Run 2:
INFO - 2016-12-01 09:59:17 - __main__ - Execution time: 15.96270489692688 seconds
INFO - 2016-12-01 09:59:17 - __main__ - Have Last-Modified: 87, Hashed: 10, Number Failed: 3
Run 3 (switched round which one runs first):
INFO - 2016-12-01 10:10:32 - __main__ - Execution time: 17.060651779174805 seconds
INFO - 2016-12-01 10:10:32 - __main__ - Have Last-Modified: 87, Hashed: 8, Number Failed: 5

aiohttp:
Run 1:
INFO - 2016-12-01 09:58:21 - __main__ - Execution time: 300.0312280654907 seconds
INFO - 2016-12-01 09:58:21 - __main__ - Have Last-Modified: 55, Hashed: 8, Number Failed: 37
Run 2:
INFO - 2016-12-01 10:04:17 - __main__ - Execution time: 300.0241482257843 seconds
INFO - 2016-12-01 10:04:17 - __main__ - Have Last-Modified: 74, Hashed: 11, Number Failed: 15
Run 3 (switched round which one runs first):
INFO - 2016-12-01 10:10:15 - __main__ - Execution time: 300.03467059135437 seconds
INFO - 2016-12-01 10:10:15 - __main__ - Have Last-Modified: 59, Hashed: 12, Number Failed: 29

Steps to reproduce

I have made a cut down example here: https://github.com/mcarans/aiohttpvsgrequests

Your environment

aiohttp 1.1.6
grequests 0.3.0
Linux Mint 18

[tumb1er]

Chunk iteration with size 1024 is very inefficient, may be difference will be less with greater values?

Also, example is not reproducible:

FileNotFoundError: [Errno 2] No such file or directory: '/home/tumbler/.hdxkey'

Also, for asyncio example uses semaphore. Why?

asyncio.Timeout(300) may be the cause of high execution time, what happens with timeout=10?

[asvetlov]

The code is not equal.
You spawn new tasks in aiohttp case but wait for requests execution by imap in grequests code.
Please switch to asyncio.gather() or create new greenlets by Greenlet.spawn/gevent.Thread explicitly.

[mcarans]

@asvetlov Forgive my stupidity but I am not clear what you are suggesting I do. Are you suggesting I change the grequests code or the aiohttp code? If grequests, then that does not solve my problem of performance and failures in aiohttp. If aiohttp, then my code already uses asyncio.gather(*tasks) and I do not know how to add greenlets to aiohttp.

I am using the two asynchronous libraries aiohttp and grequests as described in their documentation. Is it possible to make changes to my aiohttp code so that it runs as fast as the grequests code and has similar numbers of failures?

@tumb1er Sorry about that, I have updated the code and supporting library so as not to require the file you mentioned. asyncio.Timeout(10) produces many more failures. The semaphore is to limit the number of connections.

[asvetlov]

Drop asyncio.ensure_future() call here: https://github.com/mcarans/aiohttpvsgrequests/blob/master/run.py#L56
It should look like

        for metadata in last_modified_check:
            task = bound_fetch(sem, metadata, session)
            tasks.append(task)
        return await asyncio.gather(*tasks)

[asvetlov]

And, obviously, you don't need a semaphore

[klen]

And, obviously, you don't need a semaphore

He uses the semaphore to limit aiohttp up to 100 parallel connections. Because for grequests he uses a pool with size 100.

[asvetlov]

limit=100 in aiohttp.Connectior should be enough.

[mcarans]

@asvetlov I thought limit=100 in aiohttp.Connector was for connections to the same host rather than any host, whereas the semaphore would be like the grequests pool as @klen mentioned?

[asvetlov]

Yes, you are right.
Sorry for misleading.

[mcarans]

@asvetlov Thanks for your help so far. Much appreciated. I have dropped ensure_future (and checked into GitHub), but unfortunately the result is broadly the same:
INFO - 2016-12-01 15:02:19 - __main__ - Execution time: 300.05031394958496 seconds
INFO - 2016-12-01 15:02:19 - __main__ - Have Last-Modified: 75, Hashed: 9, Number Failed: 16

[asvetlov]

Ok. I'll try to clone your repo and experiment with it a bit later.

[mcarans]

Thanks a lot. I have spent several days trying to figure it out unsuccessfully. My hope is to drop grequests and use aiohttp going forwards as it is the "official" way to do things. BTW, it would be great if aiohttp added retry logic as in unixsurfer's response in this ticket which I made into a more general function for my own use.

[mcarans]

Connection timeouts may be the problem - I think that they may not be working in aiohttp. I get performance similar to grequests when using aiohttp.Timeout(10, loop=session.loop), but then the number of errors is higher which is no good.

I think that these timeouts: conn_timeout = 10 on TCPConnector and session.get(url, timeout=10) may not be working. I commented out the response reading part ie this part: async for chunk in response.content.iter_chunked(1024) ... so that there is just connection time - only changing aiohttp.Timeout(10, loop=session.loop) made any difference to the performance.

[asvetlov]

Hmm. I have reproduced your issue and it's very very weird.
Single request is much faster than executed in batch.
Let me dig into deeper.

[kserhii]

I added some logs and found the problem.

Function fetch works correct, but when we try to go out it's stuck.
This is because code async with session await for response.release(). Since there is no way to break data transfer, except close the socket, release function reads the whole body.

If you add response.close() after if last_modified (as you do in grequests_check_resources_for_last_modified) the execution times will be almost equal.

[mcarans]

@dodge-ksv Good spot! Thanks.

I thought that being in the with statement, on exit from with all would be closed. Is the current behaviour of not closing and requiring I write an explicit close as identified by @dodge-ksv correct?

[jettify]

@mcarans here is relevant part: https://github.com/KeepSafe/aiohttp/blob/af6dd82b455d4f5a5934cc59326f9fc18072bbbe/aiohttp/client_reqrep.py#L776-L781

as @dodge-ksv mentioned, connection is not closed but released, leftover data drained her
https://github.com/KeepSafe/aiohttp/blob/af6dd82b455d4f5a5934cc59326f9fc18072bbbe/aiohttp/client_reqrep.py#L668-L669
so connection can be reused later.

[mcarans]

@jettify Thanks for this.

What would happen if the connection was released, not drained and then reused? Would the content from the previous connection be downloaded when trying to download the content from the new connection?

[jettify]

Correct. From same connection new chunk of data is not available unless you
fetch old ones.

On Sat, 3 Dec 2016 at 22:19 Mike [e-mail] wrote:

@jettify https://github.com/jettify Thanks for this.

What would happen if the connection was released, not drained and then
reused? Would the content from the previous connection be downloaded when
trying to download the content from the new connection?

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
https://github.com/KeepSafe/aiohttp/issues/1438#issuecomment-264662982,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AANoZ2scvWFoBV4nfCLqv_bjjetNoBjcks5rEc7SgaJpZM4LBJzG
.

[asvetlov]

This is very interesting problem.
We might change .release() code in the following way:

1. Read everything from resp.context non-blocking
2. If EOF is not reached -- close the connection.

@tailhook proposed it about a year ago :)
We could do reading by blocking call with relative small timeout but it doesn't make sense IMHO.

[mcarans]

@asvetlov Is the discussion with @tailhook this issue?

For me, the "with" statement in Python gives me the impression that it handles closing things safely and so I would expect "async with" to handle the case where I choose not to read the response body.

If the behaviour is to remain as now, then it must be very clearly documented that an explicit close is necessary if the response body is not read when using "async with".

@jettify Thanks for your response.

[mwfrojdman]

What if the response is an infinite stream? Wouldn't https://github.com/KeepSafe/aiohttp/blob/af6dd82b455d4f5a5934cc59326f9fc18072bbbe/aiohttp/client_reqrep.py#L668-L669 end up in an infinite loop?

Maybe a sensible compromise would be to close the connection if chunking is used, or Content-Length implies there's more than $threshold bytes to read. In case there's eg. gigabytes of data to transfer over the network and read, opening a new connection is probably cheaper anyway.

[fafhrd91]

should be fixed in master

[fafhrd91]

@mcarans could you test with aiohttp from master

[mcarans]

@fafhrd91

It works and not only that master seems to be faster than 1.1.6 overall:

aiohttp timings:
1.1.6 - No response close: 300.0320360660553 seconds
1.1.6 - With response close: 15.828491926193237 seconds
Master - No response close: 10.035323143005371 seconds

Thanks so much all who worked on this!

[fafhrd91]

Great. Thanks!

[lock[bot]]

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].