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

Add retry handler for 500s to WebClient by default #1364

Closed
1 of 9 tasks
digitalnomd opened this issue May 4, 2023 · 3 comments · Fixed by #1367
Closed
1 of 9 tasks

Add retry handler for 500s to WebClient by default #1364

digitalnomd opened this issue May 4, 2023 · 3 comments · Fixed by #1367
Labels
enhancement M-T: A feature request for new functionality good first issue Version: 3x web-client
Milestone

Comments

@digitalnomd
Copy link
Contributor

digitalnomd commented May 4, 2023

Include custom retry handler for Slack 500s by default similar to how ConnectionErrorRetryHandler is included by default in the WebClient. I think this would be a good addition because as a developer using the slack sdk I'd expect this to be the default behavior to prevent transient Slack server errors from raising a SlackApiError 😄.

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

  • slack_sdk.web.WebClient (sync/async) (Web API client)
  • slack_sdk.webhook.WebhookClient (sync/async) (Incoming Webhook, response_url sender)
  • slack_sdk.models (UI component builders)
  • slack_sdk.oauth (OAuth Flow Utilities)
  • slack_sdk.socket_mode (Socket Mode client)
  • slack_sdk.audit_logs (Audit Logs API client)
  • slack_sdk.scim (SCIM API client)
  • slack_sdk.rtm (RTM client)
  • slack_sdk.signature (Request Signature Verifier)

Requirements

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

@mwbrooks mwbrooks added needs info An issue that is claimed to be a bug and hasn't been reproduced, or otherwise needs more info web-client and removed untriaged labels May 6, 2023
@mwbrooks
Copy link
Member

mwbrooks commented May 6, 2023

Hey @digitalnomd 👋🏻

Thanks a bunch for the feature request 🙇🏻 Always appreciate hearing real-world usage feature requests! I certainly agree that having the framework provide a retry handler would be an improvement.

Out of curiosity, when are you receiving 500 responses from the Slack API? Is it on specific API methods?

@digitalnomd
Copy link
Contributor Author

digitalnomd commented May 6, 2023

Hey @mwbrooks

I'm getting these 500s when hitting the usergroups.users.update endpoint.

I certainly agree that having the framework provide a retry handler would be an improvement.

That's great 🙌! Are you accepting PRs for this change if I were to put one up?

@seratch
Copy link
Member

seratch commented May 8, 2023

@digitalnomd Thanks for taking the time to share the idea! Adding a new built-in handler sounds fine for me too.

Are you accepting PRs for this change if I were to put one up?

We appreciate your contribution 🙌 One thing I wanted to ask is that we don't want to enable the retry handler by default. The 500 errors you occasionally encournter may be resolved when immediately retrying the same but in many situations, the internal server errors may continue for a while especially during Slack's outage.

So, in summary,

  • We can add it to the bulit-in ones (sync and async) but we should not add it to default_retry_handlers() and async_default_retry_handlers()
  • It must use the backoff interval calculator to avoid making lots of repeated requests to the servers

Thanks again for the idea here!

@seratch seratch added enhancement M-T: A feature request for new functionality Version: 3x good first issue and removed needs info An issue that is claimed to be a bug and hasn't been reproduced, or otherwise needs more info labels May 8, 2023
@seratch seratch added this to the 3.22.0 milestone May 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement M-T: A feature request for new functionality good first issue Version: 3x web-client
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants