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

Avoid creating TimerContext when there is no timeout #1817

Merged
merged 1 commit into from
Apr 16, 2017
Merged

Avoid creating TimerContext when there is no timeout #1817

merged 1 commit into from
Apr 16, 2017

Conversation

jcugat
Copy link
Contributor

@jcugat jcugat commented Apr 15, 2017

This will allow to use aiohttp with ioloops that don't implement
asyncio.Task.current_task, like Tornado's couroutine runner.

More info: #1180

What do these changes do?

This will allow to use aiohttp with ioloops that don't implement asyncio.Task.current_task, like Tornado's couroutine runner.

Are there changes in behavior for the user?

This will restore previous functionality that was available in v1.3.3, which was possible to run with Tornado when setting timeout=None. This was lost in this refactor: 38360f9

Related issue number

More information is available in the original issue: #1180

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt
    • The format is <Name> <Surname>.
    • Please keep alphabetical order, the file is sorted by names.
  • Add a new entry to CHANGES.rst
    • Choose any open position to avoid merge conflicts with other PRs.
    • Add a link to the issue you are fixing (if any) using #issue_number format at the end of changelog message. Use Pull Request number if there are no issues for PR or PR covers the issue only partially.

@codecov-io
Copy link

codecov-io commented Apr 15, 2017

Codecov Report

Merging #1817 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1817      +/-   ##
==========================================
+ Coverage   97.18%   97.18%   +<.01%     
==========================================
  Files          37       37              
  Lines        7467     7469       +2     
  Branches     1297     1298       +1     
==========================================
+ Hits         7257     7259       +2     
  Misses         89       89              
  Partials      121      121
Impacted Files Coverage Δ
aiohttp/helpers.py 97.59% <100%> (+0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cd8f4bf...3e48c62. Read the comment docs.

This will allow to use aiohttp with ioloops that don't implement
asyncio.Task.current_task, like Tornado's couroutine runner.

More info: #1180
@asvetlov
Copy link
Member

I believe Tornado should fix its issue with asyncio compatibility instead of trying to invent workarounds with tornado in every async library.

@jcugat
Copy link
Contributor Author

jcugat commented Apr 16, 2017

Yes, I agree than the best outcome would be to implement asyncio.Task.current_task in Tornado, but since this behaviour was previously possible (in aiohttp v1.3.3) but it's broken now, I'd treat this as an actual bug.

@fafhrd91 fafhrd91 merged commit 2248325 into aio-libs:master Apr 16, 2017
@fafhrd91
Copy link
Member

thanks

@fafhrd91
Copy link
Member

I am implementing alternative event loop at the moment, and asyncio.Task.current_task api is so bad and ugly.

@lock
Copy link

lock bot commented Oct 29, 2019

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.

@lock lock bot added the outdated label Oct 29, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants