Repository: scrapy/scrapy
Pull request: [fix] wrong reactor installed error
- Error occured when TWISTED_REACTOR in settings.py was set
- remove all top-level imports for twisted.internet.reactor
- add example, reason for top-level imports in TWISTED_REACTOR in `docs/settings.rst`

Fixes #4401 

Problem statement:
Easy to break installing the custom reactor
I created a spider that imports `scrapy.mail` and got "wrong reactor installed" after setting `TWISTED_REACTOR`. This is because `scrapy.mail` imports `twisted.internet.reactor` at the top level and because `scrapy crawl` imports all spider modules.

So I think we should kill all top-level `twisted.internet.reactor` imports (it was partially done when adding the feature, so that importing `CrawlerProcess` doesn't install the reactor) and add documentation that the user code shouldn't do similar top-level imports.

Hints from issue discussion:
I am working on this issue.
True, this is what happened to me in https://github.com/scrapinghub/testspiders/pull/10, I didn't realize there were still conflicting imports within Scrapy itself.
@wRAR I have tried a lot to get "wrong reactor installed" error but cannot get it. Can you provide an example? 
@adityaa30 set `TWISTED_REACTOR = 'twisted.internet.asyncioreactor.AsyncioSelectorReactor'` in settings.py and add `import scrapy.mail` in your spider.
@wRAR  I have found out that, even if don't do `import scrapy.mail` and execute `scrapy crawl quotes` with `TWISTED_REACTOR = 'twisted.internet.asyncioreactor.AsyncioSelectorReactor'` set in my settings.py. I still get the error:

![Screenshot from 2020-03-06 04-02-38](https://user-images.githubusercontent.com/37837542/76005681-6160ee00-5f31-11ea-8114-a733138bb090.png)


I feel this is because `twisted.internet.reactor` does:
```python
from __future__ import division, absolute_import
import sys
del sys.modules['twisted.internet.reactor']
from twisted.internet import default
default.install()
```

I want to know If I am going in correct direction?
Then it looks like some other code imports `twisted.internet.reactor` in your test.
@wRAR Okay thanks! I think I have a good idea now what the exact issue is. I will soon send a pr fixing this issue. Also, shall I add in documentation that the user code shouldn't do similar top-level imports in the same pr? If yes, where in docs should this be added?


The docs should say what can happen, why, and how to prevent it. It's also not just about user code but also about 3rd part libraries that are imported in user code. I think a good place for this is in https://docs.scrapy.org/en/latest/topics/settings.html#std:setting-TWISTED_REACTOR
Okay, working on it.
@wRAR I have sent a pr #4406  fixing this issue.

Commit message: [fix] top-level twisted.internet.reactor imports

Patch to review:
diff --git a/docs/topics/settings.rst b/docs/topics/settings.rst
index a70023efa71..d3db2041bb5 100644
--- a/docs/topics/settings.rst
+++ b/docs/topics/settings.rst
@@ -1473,7 +1473,66 @@ If a reactor is already installed,
 
 :meth:`CrawlerRunner.__init__ <scrapy.crawler.CrawlerRunner.__init__>` raises
 :exc:`Exception` if the installed reactor does not match the
-:setting:`TWISTED_REACTOR` setting.
+:setting:`TWISTED_REACTOR` setting; therfore, having top-level
+:mod:`~twisted.internet.reactor` imports in project files & imported
+3rd party libraries will make Scrapy raise :exc:`Exception` when
+it checks which reactor is installed.
+
+In order to use the reactor installed by scrapy::
+
+    import scrapy
+    from twisted.internet import reactor
+
+
+    class QuotesSpider(scrapy.Spider):
+        name = 'quotes'
+
+        def __init__(self, *args, **kwargs):
+            self.timeout = int(kwargs.pop('timeout', '60'))
+            super(QuotesSpider, self).__init__(*args, **kwargs)
+
+        def start_requests(self):
+            reactor.callLater(self.timeout, self.stop)
+
+            urls = ['http://quotes.toscrape.com/page/1']
+            for url in urls:
+                yield scrapy.Request(url=url, callback=self.parse)
+
+        def parse(self, response):
+            for quote in response.css('div.quote'):
+                yield {'text': quote.css('span.text::text').get()}
+
+        def stop(self):
+            self.crawler.engine.close_spider(self, 'timeout')
+
+
+which raises :exc:`Exception`, becomes::
+
+    import scrapy
+
+
+    class QuotesSpider(scrapy.Spider):
+        name = 'quotes'
+
+        def __init__(self, *args, **kwargs):
+            self.timeout = int(kwargs.pop('timeout', '60'))
+            super(QuotesSpider, self).__init__(*args, **kwargs)
+
+        def start_requests(self):
+            from twisted.internet import reactor
+            reactor.callLater(self.timeout, self.stop)
+
+            urls = ['http://quotes.toscrape.com/page/1']
+            for url in urls:
+                yield scrapy.Request(url=url, callback=self.parse)
+
+        def parse(self, response):
+            for quote in response.css('div.quote'):
+                yield {'text': quote.css('span.text::text').get()}
+
+        def stop(self):
+            self.crawler.engine.close_spider(self, 'timeout')
+
 
 The default value of the :setting:`TWISTED_REACTOR` setting is ``None``, which
 means that Scrapy will not attempt to install any specific reactor, and the
diff --git a/scrapy/core/downloader/__init__.py b/scrapy/core/downloader/__init__.py
index 5a2fdadf53b..644be121f17 100644
--- a/scrapy/core/downloader/__init__.py
+++ b/scrapy/core/downloader/__init__.py
@@ -3,7 +3,7 @@
 from datetime import datetime
 from collections import deque
 
-from twisted.internet import reactor, defer, task
+from twisted.internet import defer, task
 
 from scrapy.utils.defer import mustbe_deferred
 from scrapy.utils.httpobj import urlparse_cached
@@ -133,6 +133,7 @@ def _deactivate(response):
         return deferred
 
     def _process_queue(self, spider, slot):
+        from twisted.internet import reactor
         if slot.latercall and slot.latercall.active():
             return
 
diff --git a/scrapy/core/downloader/handlers/ftp.py b/scrapy/core/downloader/handlers/ftp.py
index 1681c6df8ad..432cb183192 100644
--- a/scrapy/core/downloader/handlers/ftp.py
+++ b/scrapy/core/downloader/handlers/ftp.py
@@ -32,7 +32,6 @@
 from io import BytesIO
 from urllib.parse import unquote
 
-from twisted.internet import reactor
 from twisted.internet.protocol import ClientCreator, Protocol
 from twisted.protocols.ftp import CommandFailed, FTPClient
 
@@ -81,6 +80,7 @@ def from_crawler(cls, crawler):
         return cls(crawler.settings)
 
     def download_request(self, request, spider):
+        from twisted.internet import reactor
         parsed_url = urlparse_cached(request)
         user = request.meta.get("ftp_user", self.default_user)
         password = request.meta.get("ftp_password", self.default_password)
diff --git a/scrapy/core/downloader/handlers/http10.py b/scrapy/core/downloader/handlers/http10.py
index d4aa51bd160..c0146a0a6b8 100644
--- a/scrapy/core/downloader/handlers/http10.py
+++ b/scrapy/core/downloader/handlers/http10.py
@@ -1,7 +1,5 @@
 """Download handlers for http and https schemes
 """
-from twisted.internet import reactor
-
 from scrapy.utils.misc import create_instance, load_object
 from scrapy.utils.python import to_unicode
 
@@ -26,6 +24,7 @@ def download_request(self, request, spider):
         return factory.deferred
 
     def _connect(self, factory):
+        from twisted.internet import reactor
         host, port = to_unicode(factory.host), factory.port
         if factory.scheme == b'https':
             client_context_factory = create_instance(
diff --git a/scrapy/core/downloader/handlers/http11.py b/scrapy/core/downloader/handlers/http11.py
index 93951d3b578..04a8d617a87 100644
--- a/scrapy/core/downloader/handlers/http11.py
+++ b/scrapy/core/downloader/handlers/http11.py
@@ -8,7 +8,7 @@
 from time import time
 from urllib.parse import urldefrag
 
-from twisted.internet import defer, protocol, reactor, ssl
+from twisted.internet import defer, protocol, ssl
 from twisted.internet.endpoints import TCP4ClientEndpoint
 from twisted.internet.error import TimeoutError
 from twisted.web.client import Agent, HTTPConnectionPool, ResponseDone, ResponseFailed, URI
@@ -33,6 +33,7 @@ class HTTP11DownloadHandler:
     lazy = False
 
     def __init__(self, settings, crawler=None):
+        from twisted.internet import reactor
         self._pool = HTTPConnectionPool(reactor, persistent=True)
         self._pool.maxPersistentPerHost = settings.getint('CONCURRENT_REQUESTS_PER_DOMAIN')
         self._pool._factory.noisy = False
@@ -81,6 +82,7 @@ def download_request(self, request, spider):
         return agent.download_request(request)
 
     def close(self):
+        from twisted.internet import reactor
         d = self._pool.closeCachedConnections()
         # closeCachedConnections will hang on network or server issues, so
         # we'll manually timeout the deferred.
@@ -284,6 +286,7 @@ def __init__(self, contextFactory=None, connectTimeout=10, bindAddress=None, poo
         self._txresponse = None
 
     def _get_agent(self, request, timeout):
+        from twisted.internet import reactor
         bindaddress = request.meta.get('bindaddress') or self._bindAddress
         proxy = request.meta.get('proxy')
         if proxy:
@@ -326,6 +329,7 @@ def _get_agent(self, request, timeout):
         )
 
     def download_request(self, request):
+        from twisted.internet import reactor
         timeout = request.meta.get('download_timeout') or self._connectTimeout
         agent = self._get_agent(request, timeout)
 
diff --git a/scrapy/extensions/closespider.py b/scrapy/extensions/closespider.py
index afb2ed049cc..260b2e86eef 100644
--- a/scrapy/extensions/closespider.py
+++ b/scrapy/extensions/closespider.py
@@ -6,8 +6,6 @@
 
 from collections import defaultdict
 
-from twisted.internet import reactor
-
 from scrapy import signals
 from scrapy.exceptions import NotConfigured
 
@@ -54,6 +52,7 @@ def page_count(self, response, request, spider):
             self.crawler.engine.close_spider(spider, 'closespider_pagecount')
 
     def spider_opened(self, spider):
+        from twisted.internet import reactor
         self.task = reactor.callLater(self.close_on['timeout'],
                                       self.crawler.engine.close_spider, spider,
                                       reason='closespider_timeout')
diff --git a/scrapy/mail.py b/scrapy/mail.py
index 9655b811493..b2a24a3db1d 100644
--- a/scrapy/mail.py
+++ b/scrapy/mail.py
@@ -12,7 +12,7 @@
 from email.utils import COMMASPACE, formatdate
 from io import BytesIO
 
-from twisted.internet import defer, reactor, ssl
+from twisted.internet import defer, ssl
 
 from scrapy.utils.misc import arg_to_iter
 from scrapy.utils.python import to_bytes
@@ -28,7 +28,6 @@ def _to_bytes_or_none(text):
 
 
 class MailSender(object):
-
     def __init__(self, smtphost='localhost', mailfrom='scrapy@localhost',
             smtpuser=None, smtppass=None, smtpport=25, smtptls=False, smtpssl=False, debug=False):
         self.smtphost = smtphost
@@ -47,6 +46,7 @@ def from_settings(cls, settings):
             settings.getbool('MAIL_TLS'), settings.getbool('MAIL_SSL'))
 
     def send(self, to, subject, body, cc=None, attachs=(), mimetype='text/plain', charset=None, _callback=None):
+        from twisted.internet import reactor
         if attachs:
             msg = MIMEMultipart()
         else:
@@ -111,6 +111,7 @@ def _sent_failed(self, failure, to, cc, subject, nattachs):
 
     def _sendmail(self, to_addrs, msg):
         # Import twisted.mail here because it is not available in python3
+        from twisted.internet import reactor
         from twisted.mail.smtp import ESMTPSenderFactory
         msg = BytesIO(msg)
         d = defer.Deferred()
diff --git a/scrapy/utils/benchserver.py b/scrapy/utils/benchserver.py
index cdbe21942e5..9d8d64612c6 100644
--- a/scrapy/utils/benchserver.py
+++ b/scrapy/utils/benchserver.py
@@ -3,7 +3,6 @@
 
 from twisted.web.server import Site
 from twisted.web.resource import Resource
-from twisted.internet import reactor
 
 
 class Root(Resource):
@@ -34,6 +33,7 @@ def _getarg(request, name, default=None, type=str):
 
 
 if __name__ == '__main__':
+    from twisted.internet import reactor
     root = Root()
     factory = Site(root)
     httpPort = reactor.listenTCP(8998, Site(root))
diff --git a/scrapy/utils/testproc.py b/scrapy/utils/testproc.py
index 0f15cf60a82..37803b287cd 100644
--- a/scrapy/utils/testproc.py
+++ b/scrapy/utils/testproc.py
@@ -1,7 +1,7 @@
 import sys
 import os
 
-from twisted.internet import reactor, defer, protocol
+from twisted.internet import defer, protocol
 
 
 class ProcessTest(object):
@@ -11,6 +11,7 @@ class ProcessTest(object):
     cwd = os.getcwd()  # trial chdirs to temp dir
 
     def execute(self, args, check_code=True, settings=None):
+        from twisted.internet import reactor
         env = os.environ.copy()
         if settings is not None:
             env['SCRAPY_SETTINGS_MODULE'] = settings
diff --git a/scrapy/utils/testsite.py b/scrapy/utils/testsite.py
index 6f5c21624a2..9e15988059e 100644
--- a/scrapy/utils/testsite.py
+++ b/scrapy/utils/testsite.py
@@ -1,12 +1,12 @@
 from urllib.parse import urljoin
 
-from twisted.internet import reactor
 from twisted.web import server, resource, static, util
 
 
 class SiteTest(object):
 
     def setUp(self):
+        from twisted.internet import reactor
         super(SiteTest, self).setUp()
         self.site = reactor.listenTCP(0, test_site(), interface="127.0.0.1")
         self.baseurl = "http://localhost:%d/" % self.site.getHost().port
@@ -38,6 +38,7 @@ def test_site():
 
 
 if __name__ == '__main__':
+    from twisted.internet import reactor
     port = reactor.listenTCP(0, test_site(), interface="127.0.0.1")
     print("http://localhost:%d/" % port.getHost().port)
     reactor.run()


Code review comments:
diff_hunk: @@ -1473,7 +1473,66 @@ If a reactor is already installed,
 
 :meth:`CrawlerRunner.__init__ <scrapy.crawler.CrawlerRunner.__init__>` raises
 :exc:`Exception` if the installed reactor does not match the
-:setting:`TWISTED_REACTOR` setting.
+:setting:`TWISTED_REACTOR` setting; therfore, having top-level
+:mod:`~twisted.internet.reactor` imports in project files & imported
+3rd party libraries will make Scrapy raise :exc:`Exception` when
path: docs/topics/settings.rst
text: @user1:
```suggestion
third-party libraries will make Scrapy raise :exc:`Exception` when
```