-
Notifications
You must be signed in to change notification settings - Fork 10.4k
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
Handling CloseSpider exception if it has been raised in start_requests() #6148
base: master
Are you sure you want to change the base?
Handling CloseSpider exception if it has been raised in start_requests() #6148
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #6148 +/- ##
==========================================
+ Coverage 88.55% 88.63% +0.07%
==========================================
Files 159 159
Lines 11582 11593 +11
Branches 1885 1887 +2
==========================================
+ Hits 10257 10276 +19
+ Misses 996 990 -6
+ Partials 329 327 -2
|
Still working on it, got failed tests after some changes. |
@Gallaecio @wRAR could you please take a look if have time? |
if self.slot is not None: | ||
raise RuntimeError("Engine slot is already assigned. Use self.close_spider") | ||
|
||
self.start() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't look correct because start() returns a Deferred, but also is it OK to call it here at all if we are closing anyway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wRAR
I didn't place it at beginning. But to common close_spider
method has several checks for engine being started. Not sure we need to have use it though, just think that the code inside it is useful.
I've tried to avoid it, but neither of approaches I've considered allowed to get the proper result. The only other option I see so far is to simply set close reason to spider and leave it in piece.
How do you think to do it better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In any case if you call it you need to wait until it completes.
Proposal for issue #3463.
I assume it's not the cleanest approach, so I'm opened for change requests after review.
I was trying to modify the
ExecutionEngine.open_spider
in a way that thestart_requests
's exception would be called inside it. But it looked that it would take too much changes, so a lot of stuff can be broken.Still, I'm opened to any ideas of how to make this change better.