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

Encourage creation of aiohttp public objects inside a coroutine #3333

Closed
wants to merge 17 commits into from
Closed

Encourage creation of aiohttp public objects inside a coroutine #3333

wants to merge 17 commits into from

Conversation

zhangyunhao116
Copy link
Contributor

@zhangyunhao116 zhangyunhao116 commented Oct 8, 2018

Replace asyncio.get_event_loop() with get_running_loop()

Nope

Nope

#3331

  • 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 news fragment into the CHANGES folder
    • name it <issue_id>.<type> for example (588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the pr
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

@zhangyunhao116
Copy link
Contributor Author

May be i should rebase those two commits for test ? @asvetlov

@gyermolenko
Copy link
Contributor

@ZYunH towncrier fails on python 3.6, asyncio.get_event_loop() was added in 3.7. Maybe that's the case.
Also you can run flake8 aiohttp to resolve build failure for linter.

@zhangyunhao116
Copy link
Contributor Author

@gyermolenko thx! i will fix this case

@@ -10,7 +11,7 @@ class BaseProtocol(asyncio.Protocol):

def __init__(self, loop: Optional[asyncio.AbstractEventLoop]=None) -> None:
if loop is None:
self._loop = asyncio.get_event_loop()
self._loop = get_running_loop()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is replacing the entire if loop is None block with self._loop = get_running_loop(loop).
The change doesn't only remove several lines but always check a loop (explicit or implicit) for running.

Please apply it everywhere.

Copy link
Contributor Author

@zhangyunhao116 zhangyunhao116 Oct 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got you.I will do this.
BTW,is there any plan to use asyncio.get_running_loop() which is a new api in python3.7 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eventually yes, but we should support Python 3.5 and 3.6 for a long time

@zhangyunhao116
Copy link
Contributor Author

I think it is simple at first, but it is wrong.There is still a lot of work to be done for test infra.Is there any easy way to make the test infra(more than 200 DeprecationWarning in test suites)?
And also, get event loop form another module make something wrong, i guess it create loop from other threads(i.e test.connector.test_ctor_loop()) or might I got that wrong. @asvetlov

@asvetlov
Copy link
Member

asvetlov commented Oct 9, 2018

Test suite disables default event loop, this is why you see RuntimeError: There is no current event loop in thread 'MainThread'.

In general converting def test_xxx() into async def test_xxx() can solve the issue.
Sometimes it is not easy.

Please fix trivial cases and let me know, I'll pick up the PR and finish it if needed.

@zhangyunhao116
Copy link
Contributor Author

zhangyunhao116 commented Oct 9, 2018

@asvetlov Thanks for reply! Now these commits only do

  1. Add get_running_loop() in helpers.py
  2. Replace asyncio.get_event_loop() with get_running_loop() in aiottp/*.py
    except two files:
    abc.py -> Replace will cause circular import error
    web.py -> run_app() is not coroutine function, and will always make warning

No unit tests for the changes

@asvetlov
Copy link
Member

asvetlov commented Oct 9, 2018

  1. Extract AccessLog into a separate module #3336 should help with circular imports.
  2. You are right, run_app() should keep using get_event_loop()

@zhangyunhao116
Copy link
Contributor Author

Now i can't access this PR, because previous fork is deleted for next fix.Pick up this PR if you need.
Thanks for all the work, and very nice product!

@asvetlov
Copy link
Member

asvetlov commented Nov 7, 2018

Done in a separate commit

@asvetlov asvetlov closed this Nov 7, 2018
gusutabopb added a commit to gusutabopb/aioinflux that referenced this pull request Feb 1, 2019
@lock
Copy link

lock bot commented Nov 7, 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 Nov 7, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Nov 7, 2019
@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Nov 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bot:chronographer:provided There is a change note present in this PR outdated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants