requests-threads icon indicating copy to clipboard operation
requests-threads copied to clipboard

readme example not async?

Open juliusbierk opened this issue 7 years ago • 5 comments

The example in the readme file reads:

from requests_threads import AsyncSession

session = AsyncSession(n=100)

async def _main():
    rs = []
    for _ in range(100):
        rs.append(await session.get('http://httpbin.org/get'))
    print(rs)

if __name__ == '__main__':
    session.run(_main)

shouldn't it be

from requests_threads import AsyncSession

session = AsyncSession(n=100)

async def _main():
    rs = []
    for _ in range(100):
        rs.append(session.get('http://httpbin.org/get'))
    for i in range(100):
        rs[i] = await rs[i]
    print(rs)

if __name__ == '__main__':
    session.run(_main)

to be run async?

juliusbierk avatar Jun 06 '18 14:06 juliusbierk

Had to try, and comment above is right. The original example is not async, or rather it runs asynch synchronously (though I could not for the life of me be able to find this out in a code review).

time python run01.py
>real	0m19.468s
>user	0m0.677s
>sys	0m0.062s

# vs
time python run02.py
>real	0m0.698s
>user	0m0.592s
>sys	0m0.238s

Additionally the example also caused DNS errors to appear after running it a few times. This is because the AsyncSession doesn't close properly (it just exists the program). Solved with sudo systemctl restart systemd-resolved.

Used versions:

  • requests-threads 0.1.1 pypi_0 pypi
  • twisted 19.2.0 py37h516909a_1 conda-forge

lautjy avatar May 13 '19 10:05 lautjy

I must partially agree and partially disagree with both comments. Neither one is asynchronous. The first example is not asynchronous for obvious reasons. The second example is still not asynchronous because in order to get from task #n to task #n+1, you have to wait for task #n-1 to finish and then wait for task #n to finish as well. While there would be some reduced wait time due to some asynchronicity, a properly asynchornous example would be one where the time to resolve all of the "promises" is no more than the time to resolve the longest-taking task, like JavaScript's Promise.allSettled().

While we are at it, a pull request open for 2 1/2 years? To fix the tiniest bit of documentation? Seriously??? I'm thinking the maintainer of the requests library either doesn't care or doesn't know what they are doing.

FrankDMartinez avatar Feb 12 '21 14:02 FrankDMartinez

@FrankDMartinez Yeah, I opened this issue years ago. I don't really care about it anymore.

However, I comment because I disagree with what you say: as soon as you call session.get, the thread starts. Calling await sequentially seems strange, but in terms of efficiency it's fine. If the task # 1 is the slowest of them all, it is true that you have to wait for this finish before you get the results of the other ones, but as soon as you get this result, you will immediately get the rest of the results. So this is truly asynchronous.

If you prefer a method more akin to allSettled() you can use asyncio.gather.

juliusbierk avatar Feb 12 '21 21:02 juliusbierk

@juliusbierk Sorry, I didn't mean you don't care. I meant the library maintainers don't care. I don't take seriously software which the maintainers don't care enough to keep up to date.

FrankDMartinez avatar Feb 13 '21 01:02 FrankDMartinez

Haha no offence taken! Well it happens to software all the time. :)

juliusbierk avatar Feb 13 '21 06:02 juliusbierk