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

downloader.webclient: make reactor import local #5357

Merged
merged 2 commits into from
Dec 31, 2021

Conversation

pawelmhm
Copy link
Contributor

@pawelmhm pawelmhm commented Dec 30, 2021

All reactor imports must be local not top level because twisted.internet.reactor import installs default reactor, and if you want to use non-default reactor like asyncio top level import will break things for you.

In my case I had a middleware imported in spider

in spider

from project.middlewares.retry import CustomRetryMiddleware

this was importing

from scrapy.downloadermiddlewares import retry

then retry was importing

from scrapy.core.downloader.handlers.http11 import TunnelError

and http11 was importing

from scrapy.core.downloader.webclient import _parse

and webclient was importing and installing reactor. I wonder how this was not detected earlier? It should break things for people trying to be using asyncio reactor in more complex projects.

This import of reactor may be annoying for many projects because it is easy to forget and do import reactor somewhere in your scrapy middleware or somewhere else. Then you'll get an error when using asyncio, and some people will be confused they may not know where to look for source of the problem.

@codecov
Copy link

codecov bot commented Dec 30, 2021

Codecov Report

Merging #5357 (80e2521) into master (7380888) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #5357   +/-   ##
=======================================
  Coverage   88.59%   88.59%           
=======================================
  Files         163      163           
  Lines       10618    10619    +1     
  Branches     1807     1807           
=======================================
+ Hits         9407     9408    +1     
  Misses        936      936           
  Partials      275      275           
Impacted Files Coverage Δ
scrapy/core/downloader/webclient.py 94.65% <100.00%> (+0.04%) ⬆️

@Gallaecio Gallaecio merged commit e4bdd1c into scrapy:master Dec 31, 2021
@Gallaecio
Copy link
Member

Nice catch!

This pull request was closed.
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.

3 participants