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

Base support for asyncio #4010

Merged
merged 34 commits into from Dec 29, 2019
Merged

Base support for asyncio #4010

merged 34 commits into from Dec 29, 2019

Conversation

wRAR
Copy link
Contributor

@wRAR wRAR commented Sep 12, 2019

This is #3972, rebased against the non-py2 branch. I can copy my comments from there if needed.

scrapy/utils/defer.py Outdated Show resolved Hide resolved
@wRAR wRAR force-pushed the asyncio-base branch 2 times, most recently from eb2dce1 to 63ea95d Compare Nov 14, 2019
@wRAR wRAR changed the base branch from remove-py2-base to master Nov 14, 2019
@wRAR wRAR closed this Nov 14, 2019
@wRAR wRAR reopened this Nov 14, 2019
@codecov
Copy link

codecov bot commented Nov 14, 2019

Codecov Report

Merging #4010 into master will increase coverage by 0.27%.
The diff coverage is 89.02%.

@@            Coverage Diff             @@
##           master    #4010      +/-   ##
==========================================
+ Coverage   83.77%   84.04%   +0.27%     
==========================================
  Files         166      166              
  Lines        9668     9709      +41     
  Branches     1448     1450       +2     
==========================================
+ Hits         8099     8160      +61     
+ Misses       1322     1296      -26     
- Partials      247      253       +6
Impacted Files Coverage Δ
scrapy/utils/markup.py 100% <ø> (ø) ⬆️
scrapy/utils/defer.py 96.61% <ø> (+15.03%) ⬆️
scrapy/http/response/text.py 100% <ø> (ø) ⬆️
scrapy/utils/multipart.py 100% <ø> (ø) ⬆️
scrapy/exporters.py 100% <ø> (ø) ⬆️
scrapy/linkextractors/lxmlhtml.py 92.3% <ø> (ø) ⬆️
scrapy/logformatter.py 92% <ø> (ø) ⬆️
scrapy/http/response/__init__.py 93.65% <ø> (ø) ⬆️
scrapy/core/scraper.py 89.67% <ø> (+0.2%) ⬆️
scrapy/middleware.py 90.38% <0%> (ø) ⬆️
... and 27 more

@wRAR
Copy link
Contributor Author

wRAR commented Dec 4, 2019

I've added the ASYNCIO_SUPPORT option as discussed recently. I've wrote https://github.com/scrapy/scrapy/wiki/Asyncio-use-cases that helped me to understand possible real-world use cases and what should we do with them.

scrapy/utils/asyncio.py Outdated Show resolved Hide resolved
docs/topics/settings.rst Outdated Show resolved Hide resolved
scrapy/utils/asyncio.py Outdated Show resolved Hide resolved
scrapy/utils/asyncio.py Outdated Show resolved Hide resolved
scrapy/utils/defer.py Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
@wRAR
Copy link
Contributor Author

wRAR commented Dec 5, 2019

I've found that I once again forgot to write the ASYNCIO_ENABLED handling for CrawlerProcess. So making a note here.

Looks like we'll need a new test that runs a new process with CrawlerProcess inside.

scrapy/utils/defer.py Outdated Show resolved Hide resolved
@wRAR
Copy link
Contributor Author

wRAR commented Dec 16, 2019

@Gallaecio can you please explain how to investigate the coverage?

@Gallaecio
Copy link
Member

Gallaecio commented Dec 17, 2019

@wRAR I’m not sure at the moment, but I’ve created #4230 to deal with that separately, I don’t think coverage data should block these changes I’ll look into it.

@Gallaecio
Copy link
Member

Gallaecio commented Dec 17, 2019

@wRAR The first thing I noticed is that deferred_from_coro is defined but never used, which would explain why it is not included in the coverage data.

@Gallaecio
Copy link
Member

Gallaecio commented Dec 23, 2019

I bet it’s far from trivial, but would it be possible to write tests for those import errors?

@wRAR
Copy link
Contributor Author

wRAR commented Dec 23, 2019

@Gallaecio hmmm.

The requirements have changed since that code was first written, asyncio is now available in all supported Python versions and twisted.internet.asyncioreactor.AsyncioSelectorReactor is available in our minimal required Twisted version (17.9.0). I think these checks can be removed.

I also wonder if we should test asyncio in the "pinned" env, maybe use py35-asyncio one for that. I remember some features being only available in newer Twisted.

tox.ini Outdated Show resolved Hide resolved
@kmike kmike merged commit bb991cd into master Dec 29, 2019
@kmike
Copy link
Member

kmike commented Dec 29, 2019

Looks good. Thanks @wRAR for the feature, and @Gallaecio and @elacuesta for the reviews!

@CarterPape
Copy link

CarterPape commented Mar 29, 2020

@wRAR This feature has been on my mind since I started using this package. I am so happy that you and the Scrapinghub team have initiated the support for asyncio.

@wRAR wRAR added the asyncio label Aug 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants