-
Notifications
You must be signed in to change notification settings - Fork 572
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
[runner.py] don't call get_application #411
Conversation
this is because get_application launches default Twisted reactor when called from runner, and we want to respect Reactor settings from Scrapy.
fixes #377 |
waiting for this: #412 to be merged, after that I'll add unit test here |
Sounds good to me. I've done a quick review of #412. Let me know if anything else you need. |
Codecov Report
@@ Coverage Diff @@
## master #411 +/- ##
==========================================
+ Coverage 79.63% 79.67% +0.03%
==========================================
Files 23 23
Lines 1051 1053 +2
==========================================
+ Hits 837 839 +2
Misses 214 214
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
removed windows, created issue for investigating this |
@jpmckinney so I'll integrate this PR with current master and merge. Then we can release 1.3 version, there is a lot of fresh code available. Next step in 2022 we can release 2.0 with this #421, possibly also this #304? |
cebbc36
to
388953d
Compare
Sounds good to me! |
This is because get_application launches default Twisted reactor when called from runner, and we want to respect reactor settings from Scrapy. I also think there is some weird circularity in current code.
Fixes #377
How code in master branch works now
To fix this:
on point 5) instead of calling get_application we just get Eggstorage, without application object. I think there is no need for an application object in runner which is called from Application already as subprocess. If root process is creating subprocess I do not expect it to create another object that manages root process. So now it won't call application
TODO