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

Get eventloop from event_loop_policy to avoid deprecation warning. #5689

Conversation

Godson-Gnanaraj
Copy link
Contributor

@Godson-Gnanaraj Godson-Gnanaraj commented Oct 24, 2022

Fixes #5685

Use policy.get_event_loop() instead of asyncio.get_event_loop().
If there is no event loop running, policy will create new_event_loop.

scrapy/utils/reactor.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Oct 24, 2022

Codecov Report

Merging #5689 (78523a3) into master (92be5ba) will increase coverage by 0.16%.
The diff coverage is 76.92%.

@@            Coverage Diff             @@
##           master    #5689      +/-   ##
==========================================
+ Coverage   88.66%   88.83%   +0.16%     
==========================================
  Files         162      162              
  Lines       10995    11003       +8     
  Branches     1799     1800       +1     
==========================================
+ Hits         9749     9774      +25     
+ Misses        966      948      -18     
- Partials      280      281       +1     
Impacted Files Coverage Δ
scrapy/utils/reactor.py 81.57% <62.50%> (+2.12%) ⬆️
scrapy/utils/defer.py 97.33% <100.00%> (+0.03%) ⬆️
scrapy/core/downloader/__init__.py 90.97% <0.00%> (-1.51%) ⬇️
scrapy/core/engine.py 84.01% <0.00%> (+0.05%) ⬆️
scrapy/downloadermiddlewares/httpproxy.py 96.61% <0.00%> (+0.11%) ⬆️
scrapy/robotstxt.py 97.53% <0.00%> (+22.22%) ⬆️

Copy link
Member

@Gallaecio Gallaecio left a comment

Choose a reason for hiding this comment

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

Could you add a test for this change?

@Godson-Gnanaraj
Copy link
Contributor Author

Could you add a test for this change?

I think windows tests should cover these changes. But I couldn't see windows coverage reports being uploaded.

@Gallaecio
Copy link
Member

I meant a test to verify that the warning is no longer triggered, for which I imagine there is no existing test.

Good point about coverage, though. We need to look into that.

@wRAR
Copy link
Member

wRAR commented Oct 26, 2022

tests/test_downloadermiddleware.py: 2 warnings
tests/test_utils_asyncgen.py: 2 warnings
tests/test_utils_defer.py: 4 warnings
tests/test_utils_python.py: 2 warnings
tests/test_utils_signal.py: 2 warnings
  /home/runner/work/scrapy/scrapy/scrapy/utils/defer.py:272: DeprecationWarning: There is no current event loop
    return Deferred.fromFuture(asyncio.ensure_future(o))

@wRAR
Copy link
Member

wRAR commented Oct 26, 2022

Oh right, you said it's a partial fix.

@Godson-Gnanaraj
Copy link
Contributor Author

Godson-Gnanaraj commented Oct 26, 2022

tests/test_downloadermiddleware.py: 2 warnings
tests/test_utils_asyncgen.py: 2 warnings
tests/test_utils_defer.py: 4 warnings
tests/test_utils_python.py: 2 warnings
tests/test_utils_signal.py: 2 warnings
  /home/runner/work/scrapy/scrapy/scrapy/utils/defer.py:272: DeprecationWarning: There is no current event loop
    return Deferred.fromFuture(asyncio.ensure_future(o))

@wRAR Can I move get_event_loop_policy to scrapy/utils/misc.py so that it can be used in both reactor.py and defer.py?

@wRAR
Copy link
Member

wRAR commented Oct 26, 2022

What do you mean by get_event_loop_policy? Some new method?

@Godson-Gnanaraj
Copy link
Contributor Author

Yes.

def get_asycio_event_loop_policy():
    policy = asyncio.get_event_loop_policy()
    if (
        sys.version_info >= (3, 8)
        and sys.platform == "win32"
        and not isinstance(policy, asyncio.WindowsSelectorEventLoopPolicy)
    ):
        policy = asyncio.WindowsSelectorEventLoopPolicy()
        asyncio.set_event_loop_policy(policy)

    return policy

@wRAR
Copy link
Member

wRAR commented Oct 26, 2022

I think it's fine to put it into utils.reactor? utils.defer already imports utils.reactor.

@Godson-Gnanaraj Godson-Gnanaraj force-pushed the 5685-fix-no-event-loop-deprecation-warning branch from c3fb766 to 6bd6161 Compare October 26, 2022 10:16
@Gallaecio Gallaecio added this to the Scrapy 2.7.1 milestone Oct 26, 2022
@wRAR
Copy link
Member

wRAR commented Oct 27, 2022

Should the remaining warning be fixed by passing loop=policy.get_event_loop()) to asyncio.ensure_future(o) in deferred_from_coro()?

Copy link
Member

@wRAR wRAR left a comment

Choose a reason for hiding this comment

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

Perfect!

Copy link
Member

@Gallaecio Gallaecio left a comment

Choose a reason for hiding this comment

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

The code looks good to me.

Can someone manually verify that the new test fails if the other changes are reverted? That’s the only thing I wanted to check manually when I found the time.

@wRAR
Copy link
Member

wRAR commented Oct 27, 2022

@Gallaecio I've changed the test in the master branch and ran it, I got AssertionError: 1 != 0

Copy link
Member

@Gallaecio Gallaecio left a comment

Choose a reason for hiding this comment

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

@Gallaecio Gallaecio merged commit 3a34fa8 into scrapy:master Oct 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DeprecationWarning: There is no current event loop
4 participants