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.

Sign up for GitHub

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

Jump to bottom

readme example not async? #8

Open
juliusbierk opened this issue Jun 6, 2018 · 5 comments · May be fixed by #10
Open

readme example not async? #8

juliusbierk opened this issue Jun 6, 2018 · 5 comments · May be fixed by #10

Comments

@juliusbierk
Copy link

juliusbierk commented Jun 6, 2018 •
edited

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?
@acrogenesis acrogenesis linked a pull request Apr 26, 2019 that will close this issue
Open
@lautjy
Copy link

lautjy commented May 13, 2019 •
edited

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

@FrankDMartinez
Copy link

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.

@juliusbierk
Copy link
Author

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

@FrankDMartinez
Copy link

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

@juliusbierk
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

update readme to real async get acrogenesis/requests-threads
3 participants
@FrankDMartinez @juliusbierk @lautjy