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.

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

network: do not retry on 429 and 503 #5727

Merged
merged 1 commit into from
Sep 16, 2024

Conversation

FiveOFive
Copy link
Contributor

@FiveOFive FiveOFive commented Sep 15, 2024

Overview

This is fix for when ZAP is stuck waiting to retry requests after receiving a response with the retry-after header set. See zaproxy/zaproxy#8627

Related Issues

Fix zaproxy/zaproxy#8627.

Checklist

  • Update help
  • Update changelog
  • Run ./gradlew spotlessApply for code formatting
  • Write tests
  • Check code coverage
  • Sign-off commits
  • Squash commits
  • Use a descriptive title

For more details, please refer to the developer rules and guidelines.

Copy link

github-actions bot commented Sep 15, 2024

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@FiveOFive
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@thc202
Copy link
Member

thc202 commented Sep 15, 2024

This should have tests.

@thc202 thc202 changed the title Fix ZAP stuck from 'retry-after' header on 429, 503 network: do not honour 'retry-after' header Sep 15, 2024
@thc202 thc202 changed the title network: do not honour 'retry-after' header network: do not retry on 429 and 503 Sep 15, 2024
@thc202
Copy link
Member

thc202 commented Sep 15, 2024

fwiw, it would have been better if this was added to the rate limiter rather than stopping completely.

addOns/network/CHANGELOG.md Outdated Show resolved Hide resolved
NoRouteToHostException.class,
SSLException.class);

private static final List<Integer> RETRIABLE_CODES = Arrays.asList();
Copy link
Member

Choose a reason for hiding this comment

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

List.of()

Vs Arrays.asList()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@FiveOFive
Copy link
Contributor Author

This should have tests.

I've added a test for both 429 and 503 status codes

@FiveOFive
Copy link
Contributor Author

fwiw, it would have been better if this was added to the rate limiter rather than stopping completely.

Would that be an additional change in combination with this PR or instead of this PR? I didn't see how the rate limiter could help with the behavior of the underlying http client retry strategy, but I'm not that familiar with the networking code here. Let me know if the team prefers a different solution.

@thc202
Copy link
Member

thc202 commented Sep 16, 2024

#5727 (comment)

Looks good but we should verify the behaviour more broadly.

#5727 (comment)

It does not need to be in this PR. It would be an improvement for the rate limiter, this change/PR is still needed either way.

@thc202
Copy link
Member

thc202 commented Sep 16, 2024

Thank you!

@kingthorin kingthorin merged commit 354c81b into zaproxy:main Sep 16, 2024
10 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Sep 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

429 and 503 responses with long "retry-after" causes scans to hang
3 participants