Add retries to link checking by saswatamcode · Pull Request #40 · bwplotka/mdox

saswatamcode · 2021-06-11T08:45:58Z

This PR adds retries to link checking based on status code. For status code 429, it uses the Retry-After header to wait before retrying and retries normally for status code 301, 307 and 503. Can try a maximum of three times before returning error. This can increase the time of processing docs if rate limit is encountered but not too excessively.
Fixes #11.

Some questions:

Should this logic be put behind a --retry flag so that user can en/disable and set number of retries?
The Retry-After header works well for sites like GitHub which returns 429 with Retry-After set to 60s. But other sites might make value excessive, so should a limit be placed?

Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>

bwplotka

Nice, some comments.

pkg/mdformatter/linktransformer/link.go

bwplotka · 2021-06-11T12:19:44Z

pkg/mdformatter/linktransformer/link.go

+				response.Ctx.Put(retryKey, strconv.Itoa(retries+1))
+				retryAfter, convErr := strconv.Atoi(response.Headers.Get("Retry-After"))
+				if convErr == nil {
+					time.Sleep(time.Duration(retryAfter) * time.Second)


hmmmmm I think we need to have global retry. You see that will not work if others for same host will try in the same time 🤔

Oh! I didn't think of this before! Will try to implement global retry!

I thought about this and tested it a bit. I think we never make requests to the same host again or at the same time due to our present caching layer and retries can only occur after request! So this should work. 🙂

I challenge this statement (:

I think it's fine for now, let's iterate (global retry will be hard to implement), but I think it's needed, paths are cached not hosts

pkg/mdformatter/linktransformer/link.go

bwplotka · 2021-06-11T12:22:58Z

pkg/mdformatter/linktransformer/link.go

+					time.Sleep(time.Duration(retryAfter) * time.Second)
+				}
+
+				retryErr := response.Request.Retry()


I might think we need default back of maybe?

Should this be in default?

well default if no retry-after is set? maybe something like 1s

Oh okay! Will try to implement this!

Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>

bwplotka

Some review during our 1:1

bwplotka · 2021-06-15T10:56:26Z

pkg/mdformatter/linktransformer/link.go

+		switch response.StatusCode {
+		case http.StatusTooManyRequests:
+			if retries <= 0 {
+				var retryAfter int


Suggested change

var retryAfter int

var retryAfterSeconds int

If it's int not time.Duration we need to mention unit in var name

bwplotka · 2021-06-15T10:57:19Z

pkg/mdformatter/linktransformer/link.go

+				if convErr != nil {
+					retryAfter = 1
+				}
+				time.Sleep(time.Duration(retryAfter) * time.Second)


Suggested change

time.Sleep(time.Duration(retryAfter) * time.Second)

select {

case <- time.After(time.Duration(retryAfter) * time.Second):

case <- response.Context.Done():

return

}

Let's also mock time.After so we can inject different times

Colly allows us to pass in context and use that, but it isn't released yet. So pinning to needed commit for now! 🙂https://pkg.go.dev/github.com/gocolly/colly/v2@v2.1.1-0.20201013153555-8252c346cfb0#Collector

bwplotka · 2021-06-15T10:59:53Z

pkg/mdformatter/linktransformer/link.go

+				v.remoteLinks[response.Ctx.Get(originalURLKey)] = errors.Wrapf(err, "%q rate limited even after retry; status code %v", response.Request.URL.String(), response.StatusCode)
+			}
+		case http.StatusMovedPermanently, http.StatusTemporaryRedirect, http.StatusServiceUnavailable:
+			if retries <= 0 {


Let's make more important block being left indendented

bwplotka · 2021-06-15T11:00:52Z

pkg/mdformatter/linktransformer/link.go

+		retries, _ := strconv.Atoi(retriesStr)
+		switch response.StatusCode {
+		case http.StatusTooManyRequests:
+			if retries <= 0 {


ditto (indent)

Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>

bwplotka

I think retrying will be annoying if we cannot interrupt it. Do you mind hooking the context properly. It will also show if we put it in right place... (:

bwplotka · 2021-06-17T08:11:07Z

pkg/mdformatter/linktransformer/link.go

 // MustNewValidator returns mdformatter.LinkTransformer that crawls all links.
 func MustNewValidator(logger log.Logger, linksValidateConfig []byte, anchorDir string) mdformatter.LinkTransformer {
-	v, err := NewValidator(logger, linksValidateConfig, anchorDir)
+	v, err := NewValidator(context.TODO(), logger, linksValidateConfig, anchorDir)


does not sound useful ;p

We can currently interrupt retry. For instance, testing a file with envoyproxy link and setting retryAfterSeconds to 10, gives the following results. First is when I let it run and retry, second is when I interrupt,

time mdox fmt -l test.md error: test.md: "https://envoyproxy.io" not accessible even after retry; status code 301: Moved Permanently mdox fmt -l test.md 0.14s user 0.06s system 1% cpu 14.995 total ime mdox fmt -l test.md ^Cinfo: caught signal. Exiting.: interrupt mdox fmt -l test.md 0.12s user 0.05s system 2% cpu 6.762 total

So we can cancel retry without having to wait. But I'm not sure what would be the correct way to hook context?

Looks like it works but we don't know why, right? That's not the best case too ;p Anyway, let's merge and think later (: I think we pass context to colly somewhere else?

Yes, we pass it here! That's why we can cancel without waiting. Also passing it is sort of weird as colly also has a context package.

Add retries to link check

1f07814

Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>

saswatamcode requested a review from bwplotka June 11, 2021 08:45

saswatamcode self-assigned this Jun 11, 2021

bwplotka reviewed Jun 11, 2021

View reviewed changes

Implement suggestions

45dc956

Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>

saswatamcode requested a review from bwplotka June 13, 2021 15:34

bwplotka reviewed Jun 15, 2021

View reviewed changes

Add ctx for clean cancellation

d7e0655

Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>

saswatamcode requested a review from bwplotka June 16, 2021 11:13

bwplotka approved these changes Jun 17, 2021

View reviewed changes

bwplotka merged commit ffae630 into bwplotka:main Jun 17, 2021

-				time.Sleep(time.Duration(retryAfter) * time.Second)
+select {
+case <- time.After(time.Duration(retryAfter) * time.Second):
+case <- response.Context.Done():
+     return
+}

Conversation

saswatamcode commented Jun 11, 2021

Uh oh!

bwplotka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bwplotka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bwplotka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saswatamcode Jun 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

saswatamcode Jun 17, 2021 •

edited

Loading