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.

Sign up for GitHub

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

Jump to bottom

feat(rule): add maxRetryTime option #136

Merged
merged 5 commits into from Nov 8, 2021
Merged

feat(rule): add maxRetryTime option #136

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
be5353a
feat(rule): add `maxRetryTime` option
chick-p Oct 31, 2021
457506a
docs: add `maxRetryTime`
chick-p Oct 31, 2021
32fcbb8
fix: change default value of maxRetryTime 0 -> 10
chick-p Nov 3, 2021
a90dec5
docs: change default value of maxRetryTime
chick-p Nov 5, 2021
701bd5a
fix: prefer to use After-Retry value
chick-p Nov 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 6 additions & 1 deletion ReadMe.md
View file
Open in desktop
Expand Up @@ -66,7 +66,8 @@ The default options are:
"preferGET": [],
"ignoreRedirects": false,
"retry": 3,
"userAgent": "textlint-rule-no-dead-link/1.0"
"userAgent": "textlint-rule-no-dead-link/1.0",
"maxRetryTime": 0
}
}
}
Expand Down Expand Up @@ -151,6 +152,10 @@ The default max retry count is `3`.

Customize `User-Agent` http header.

### maxRetryTime

The max of allow waiting time [second] for retry, if response header has `Retry-After`.

## Tests

```
Expand Down
14 changes: 10 additions & 4 deletions src/no-dead-link.js
View file
Open in desktop
Expand Up @@ -20,7 +20,8 @@ const DEFAULT_OPTIONS = {
interval: 500, // The length of time in milliseconds before the interval count resets. Must be finite. [Experimental]
intervalCap: 8, // The max number of runs in the given interval of time. [Experimental]
keepAlive: false, // {boolean} if it is true, use keepAlive for checking request [Experimental]
userAgent: 'textlint-rule-no-dead-link/1.0' // {String} a UserAgent
userAgent: 'textlint-rule-no-dead-link/1.0', // {String} a UserAgent,
maxRetryTime: 0, // (number) The max of waiting seconds for retry, if response returns `After-Retry` header.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This default value means that we will ignore Retry-After by default.
I think that we should consider Retry-After header if the server response it.

Increase maxRetryTime to 10 or 30?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've set it to 10.

I've set 0 to not impact those who are already using it.
But I understand that After-Retry value should be used.

};

// Adopted from http://stackoverflow.com/a/3809435/951517
Expand Down Expand Up @@ -186,9 +187,14 @@ const createCheckAliveURL = (ruleOptions) => {

// try to fetch again if not reach max retry count
if (currentRetryCount < maxRetryCount) {
// exponential retry
// 0ms -> 100ms -> 200ms -> 400ms -> 800ms ...
await waitTimeMs((currentRetryCount ** 2) * 100);
const retrySeconds = res.headers.get('Retry-After');
Copy link
Member

@azu azu Oct 31, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that we need to prefer Retry-After if the response header includes Retry-After.

Edit: This concern come from default value of maxRetryTime https://github.com/textlint-rule/textlint-rule-no-dead-link/pull/136/files#r739803949
So, we not need to change the logics if we change the default value.

probably, next code. It may be subtle.

const retrySeconds = res.headers.get('Retry-After');
// If the response has `Retry-After` header, prefer it
// else exponential retry: 0ms -> 100ms -> 200ms -> 400ms -> 800ms ...
const retryWaitTimeMs = retrySeconds !== null ? retrySeconds * 1000 : currentRetryCount ** 2 * 100;
const maxRetryTimeMs = ruleOptions.maxRetryTime * 1000;
if (retryWaitTime <= maxRetryTimeMs) {
  await waitTimeMs(currentRetryCount ** 2 * 100);
}
return isAliveURI(uri, 'GET', maxRetryCount, currentRetryCount + 1);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@azu
Thank you for your comment.
I rewrite the code as you said.

if (retrySeconds && retrySeconds <= ruleOptions.maxRetryTime) {
await waitTimeMs(retrySeconds * 1000);
} else {
// exponential retry
// 0ms -> 100ms -> 200ms -> 400ms -> 800ms ...
await waitTimeMs(currentRetryCount ** 2 * 100);
}
return isAliveURI(uri, 'GET', maxRetryCount, currentRetryCount + 1);
}
return {
Expand Down