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

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

Merged
merged 2 commits into from
Jan 24, 2024

Conversation

seratch
Copy link
Contributor

@seratch seratch commented Jan 24, 2024

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 [ ])

  • bolt (Bolt for Java)
  • bolt-{sub modules} (Bolt for Java - optional modules)
  • slack-api-client (Slack API Clients)
  • slack-api-model (Slack API Data Models)
  • slack-api-*-kotlin-extension (Kotlin Extensions for Slack API Clients)
  • slack-app-backend (The primitive layer of Bolt for Java)

Requirements

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

@seratch seratch added bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented project:slack-api-client project:slack-api-client labels Jan 24, 2024
@seratch seratch added this to the 1.38.0 milestone Jan 24, 2024
@seratch seratch self-assigned this Jan 24, 2024
Copy link

codecov bot commented Jan 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@             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              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@seratch seratch merged commit 36e41c9 into slackapi:main Jan 24, 2024
4 checks passed
@seratch seratch deleted the issue-1273 branch January 24, 2024 03:52
@gunrein
Copy link
Contributor

gunrein commented Jan 24, 2024

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
Copy link
Contributor Author

seratch commented Jan 24, 2024

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
Copy link
Contributor

gunrein commented Jan 25, 2024

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

seratch added a commit to seratch/java-slack-sdk that referenced this pull request Jan 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented project:slack-api-client project:slack-api-client
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AsyncMethodsRateLimiter does not handle ratelimitted errors properly
2 participants