slackapi / java-slack-sdk

Slack Developer Kit (including Bolt for Java) for any JVM language
https://slack.dev/java-slack-sdk/
MIT License
570 stars 209 forks source link

Fix #1273 AsyncMethodsRateLimiter does not handle ratelimitted errors properly #1274

Closed seratch closed 7 months ago

seratch commented 7 months ago

This pull request resolves #1273 ; Only the AsyncMethodsClient has been having this bug, so we don't need to make similar changes to SCIM/SCIMv2/Audit Logs API clients.

Category (place an x in each of the [ ])

Requirements

Please read the Contributing guidelines and Code of Conduct before creating this issue or pull request. By submitting, you agree to those rules.

codecov[bot] commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (84723a5) 74.34% compared to head (70ad2f8) 74.49%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1274 +/- ## ============================================ + Coverage 74.34% 74.49% +0.14% - Complexity 4130 4135 +5 ============================================ Files 444 444 Lines 13102 13108 +6 Branches 1323 1324 +1 ============================================ + Hits 9741 9765 +24 + Misses 2588 2570 -18 Partials 773 773 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

gunrein commented 7 months ago

Thanks for making this fix @seratch !

I'm probably missing something, but in this block in AsyncRateLimitExecutore.enqueueThenRun:

if (e.getResponse().code() == 429) {

does setRateLimitedMethodRetryEpochMillis need to called similarly to what is in MethodsClientImpl.postFormWithTokenAndParseResponse?

seratch commented 7 months ago

Thanks for the comment. I thought so too when I checked this issue, but actually it is not necessary. I found that the only changes needed for fixing this issue is the correction in AsyncMethodsRateLimiter. In rate-limited error scenario, setRateLimitedMethodRetryEpochMillis method call is done on the underlying MethodsClient side, so all the things async side should do is just to check the existence of the rate limited state of the API method.

gunrein commented 7 months ago

Nice. Thanks for confirming and thanks again for the fix!