Retry request from spider callback #3685

kasun · 2019-03-15T07:16:46Z

New approach for #3590

This,

provides a method retry_request() in response class that will return a request instance that is marked as to be retried
A spidermiddleware (RetryMiddleware) will check whether this request can be retried, do the necessary modifications and retry the request

Resolves #3590

codecov · 2019-03-15T07:43:08Z

Codecov Report

Merging #3685 into master will increase coverage by 0.14%.
The diff coverage is 97.67%.

@@            Coverage Diff             @@
##           master    #3685      +/-   ##
==========================================
+ Coverage   84.52%   84.67%   +0.14%     
==========================================
  Files         167      169       +2     
  Lines        9410     9566     +156     
  Branches     1397     1425      +28     
==========================================
+ Hits         7954     8100     +146     
- Misses       1199     1206       +7     
- Partials      257      260       +3

Impacted Files	Coverage Δ
scrapy/settings/default_settings.py	`98.63% <ø> (ø)`
scrapy/http/response/__init__.py	`90.90% <60.00%> (-2.54%)`	⬇️
scrapy/downloadermiddlewares/retry.py	`94.59% <100.00%> (-1.24%)`	⬇️
scrapy/http/request/__init__.py	`100.00% <100.00%> (ø)`
scrapy/spidermiddlewares/retry.py	`100.00% <100.00%> (ø)`
scrapy/utils/retry.py	`100.00% <100.00%> (ø)`
scrapy/crawler.py	`91.60% <0.00%> (-0.48%)`	⬇️
scrapy/linkextractors/sgml.py	`96.80% <0.00%> (ø)`
scrapy/linkextractors/__init__.py	`100.00% <0.00%> (ø)`
... and 3 more

ejulio

hey @kasun .
Great work here 👏
I also left some comments on things I think can be improved, but feel free to give your opinion as well.

ejulio · 2019-03-15T11:21:51Z

scrapy/spidermiddlewares/retry.py

+        for x in result:
+            if isinstance(x, Request):
+                if x.is_marked_for_retry():
+                    if not self.enabled:


This could be the first if statement of the method.
Maybe, it would be even better to raise NotConfigured in __init__ if RETRY_ENABLED is False

This was mostly done for the debug message. Just in case if a user disabled request retrying globally and then tries to retry a request.

ejulio · 2019-03-15T11:23:31Z

scrapy/utils/retry.py

+        self.max_retry_times = spider.settings.getint('RETRY_TIMES')
+        self.priority_adjust = spider.settings.getint('RETRY_PRIORITY_ADJUST')
+
+    def is_exhausted(self):


Maybe a property.

ejulio · 2019-03-15T11:25:00Z

scrapy/http/request/__init__.py

+        self.meta.pop('_retry_requested', None)
+        self.meta.pop('_retry_request_reason', None)
+
+    def is_marked_for_retry(self):


Maybe a property here and in the following ones.

ejulio · 2019-03-15T11:26:15Z

scrapy/http/request/__init__.py

@@ -96,3 +96,31 @@ def replace(self, *args, **kwargs):
            kwargs.setdefault(x, getattr(self, x))
        cls = kwargs.pop('cls', self.__class__)
        return cls(*args, **kwargs)
+
+    def mark_for_retry(self, reason=None, **kwargs):


I think that return req.retry() reads better than req.mark_for_retry().
Besides that, mark, for me, reads as something that is changing the instance and not creating a new one.

Besides that, I don't know if this behavior should be here.
Should a request know about the configurations other modules need to retry it?
Or should the retrying module set this configurations in the request?

I agree with req.retry(), which is similar to req.replace().

ejulio · 2019-03-15T11:27:31Z

scrapy/http/response/__init__.py

+        :param reason: Reason for retrying. Optional, defaults to None.
+        :type reason: str
+        """
+        request = self.request


Not sure if the variable is required.

ejulio · 2019-03-15T11:32:57Z

scrapy/spidermiddlewares/retry.py

+                        retry_handler.record_retry_failure(reason)
+                    else:
+                        new_req = retry_handler.make_retry_request()
+                        retry_handler.record_retry(new_req, reason)


I think this should be called inside make_retry_request.
Otherwise, I could create a retry without recording it.

ejulio · 2019-03-15T11:39:21Z

scrapy/spidermiddlewares/retry.py

+
+                    reason = x.get_retry_reason() or 'Retry Requested'
+                    retry_handler = RetryHandler(spider, x)
+                    if retry_handler.is_exhausted():


Maybe make_retry_request should return None if it is exhausted.
Then you can remove this if statement from here.

ejulio · 2019-03-15T11:40:22Z

scrapy/utils/retry.py

+class RetryHandler(object):
+    """utilities to handle retries"""
+
+    def __init__(self, spider, request):


For me, it seems that the most important dependency is the request.
So, I would change the precedence to put the request first.

ejulio · 2019-03-15T11:41:05Z

scrapy/utils/retry.py

+    def __init__(self, spider, request):
+        self.spider = spider
+        self.request = request
+        self.max_retry_times = spider.settings.getint('RETRY_TIMES')


Maybe setting default values, or raising an specific exception if these settings aren't available.

It should be possible to override max_retry_times per request.

ejulio · 2019-03-15T11:44:12Z

scrapy/utils/retry.py

+from scrapy.utils.python import global_object_name
+
+
+class RetryHandler(object):


This handler does not required any state, so it could be a set of functions.
At least, it could be instantiated only once to store the settings and then the requests could be arguments to methods.

apalala · 2019-03-17T00:14:24Z

scrapy/http/request/__init__.py

@@ -96,3 +96,31 @@ def replace(self, *args, **kwargs):
            kwargs.setdefault(x, getattr(self, x))
        cls = kwargs.pop('cls', self.__class__)
        return cls(*args, **kwargs)
+
+    def mark_for_retry(self, reason=None, **kwargs):


I agree with req.retry(), which is similar to req.replace().

apalala · 2019-03-17T00:15:37Z

scrapy/http/response/__init__.py

@@ -133,3 +133,23 @@ def follow(self, url, callback=None, method='GET', headers=None, body=None,
                       priority=priority,
                       dont_filter=dont_filter,
                       errback=errback)
+
+    def retry_request(self, reason=None, **kwargs):


Here too, there's Response.follow() which returns a Request. Why not Response.retry()?

apalala · 2019-03-17T00:17:03Z

scrapy/utils/retry.py

+    def __init__(self, spider, request):
+        self.spider = spider
+        self.request = request
+        self.max_retry_times = spider.settings.getint('RETRY_TIMES')


It should be possible to override max_retry_times per request.

ShanikaEdiriweera · 2020-03-01T09:03:00Z

Any update?

fellipegpbotelho · 2020-07-16T14:26:23Z

Any updates?

kasun added 2 commits March 15, 2019 10:18

spidermiddleware to retry requests from spider callbacks

b044d45

retry spidermiddleware documentation

04117e0

This was referenced Mar 15, 2019

Retry exception #3590 #3591

Closed

Ability to retry a request from inside a spider callback #3590

Closed

ejulio suggested changes Mar 15, 2019

View reviewed changes

apalala reviewed Mar 17, 2019

View reviewed changes

Gallaecio mentioned this pull request Nov 23, 2020

Implement a retry request function #4902

Merged

3 tasks

kmike closed this in #4902 Apr 1, 2021

Retry request from spider callback #3685

Retry request from spider callback #3685

kasun commented Mar 15, 2019 •

edited by Gallaecio

codecov bot commented Mar 15, 2019 •

edited

ejulio left a comment

ejulio Mar 15, 2019

kasun Mar 21, 2019

ejulio Mar 15, 2019

ejulio Mar 15, 2019

ejulio Mar 15, 2019

ejulio Mar 15, 2019

apalala Mar 17, 2019

ejulio Mar 15, 2019

ejulio Mar 15, 2019

ejulio Mar 15, 2019

ejulio Mar 15, 2019

ejulio Mar 15, 2019

apalala Mar 17, 2019

ejulio Mar 15, 2019

apalala Mar 17, 2019

apalala Mar 17, 2019

apalala Mar 17, 2019

ShanikaEdiriweera commented Mar 1, 2020

fellipegpbotelho commented Jul 16, 2020

		from scrapy.utils.python import global_object_name


		class RetryHandler(object):

Retry request from spider callback #3685

Retry request from spider callback #3685

Conversation

kasun commented Mar 15, 2019 • edited by Gallaecio

codecov bot commented Mar 15, 2019 • edited

Codecov Report

ejulio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShanikaEdiriweera commented Mar 1, 2020

fellipegpbotelho commented Jul 16, 2020

kasun commented Mar 15, 2019 •

edited by Gallaecio

codecov bot commented Mar 15, 2019 •

edited