Node-slack-sdk: users.profile.set can throw ratelimited errors with rejectRateLimitedCalls=false

[jayjanssen]

Description

This client claims to handle rate limiting automatically such that promises returned from API calls will block until rate limiting is bypassed with the default settings.

However, I can cause API error exceptions for 'ratelimited' by calling users.profile.set. I suspect this might be something where the API returns an (undocumented?) error that isn't coming back as a 429. Code to reproduce the problem below.

What type of issue is this? (place an x in one of the [ ])

• [X] bug
• [ ] enhancement (feature request)
• [ ] question
• [ ] documentation related
• [ ] testing related
• [ ] discussion

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

• [X] I've read and understood the Contributing guidelines and have done my best effort to follow them.
• [X] I've read and agree to the Code of Conduct.
• [X] I've searched for any related issues and avoided creating a duplicate issue.

---

Bug Report

Filling out the following details about bugs will help us solve your issue sooner.

Reproducible in:

@slack/client version: 4.8.0

node version: 11.3.0, 10.13.0, etc.

OS version(s): any -- tested MacOS - latest and in docker: Linux f883fafeedcf 4.9.125-linuxkit #1 SMP Fri Sep 7 08:20:28 UTC 2018 x86_64 Linux

Steps to reproduce:

1. Call users.profile.set repeatedly and check for any errors thrown.
2. I start getting the error after roughly 10 calls in rapid succession.

Expected result:

I expect the calls to get slower as rate limiting throttles me assuming I'm using rejectRateLimitedCalls = false (which is the default). This behavior would be fine with rejectRateLimitedCalls = true.

Actual result:

I get exceptions: Error: An API error occurred: ratelimited

Attachments:

#!/usr/bin/env coffee

slack_client = require '@slack/client'

token = 'SECRET'


do () ->
  web = new slack_client.WebClient token

  i = 100

  while i > 0
    try
      await web.users.profile.set {
        user: 'VALID_USER_ID'
        profile: {
          status_text: ''
          status_emoji: ''
          status_expiration: 0
        }
      }
      console.log "got profile: #{i}"
    catch err
      console.error "got err: #{i}: #{err}"

    i -= 1

Sample output:

$ ./rate_limit.coffee
got profile: 100
got profile: 99
got profile: 98
got profile: 97
got profile: 96
got profile: 95
got profile: 94
got profile: 93
got profile: 92
got profile: 91
got profile: 90
got err: 89: Error: An API error occurred: ratelimited
got err: 88: Error: An API error occurred: ratelimited
got err: 87: Error: An API error occurred: ratelimited
got err: 86: Error: An API error occurred: ratelimited
got err: 85: Error: An API error occurred: ratelimited
got err: 84: Error: An API error occurred: ratelimited
got err: 83: Error: An API error occurred: ratelimited

[aoberoi]

@jayjanssen thanks for reporting the issue and having thorough reproduction steps. i haven't gotten a chance to reproduce it on my own just yet, but i wanted you to know that we would look into this soon.

[aoberoi]

@jayjanssen i was able to reproduce this issue locally. i did it with a very similar program written in javascript rather than coffeescript:

const { WebClient } = require('@slack/client');

const token = 'my-token';

(async () => {
  const slack = new WebClient(token);

  for (let _ of Array(100)) {
    try {
      const result = await slack.users.profile.set({
        user: 'my user id',
        profile: {
          status_text: 'node sdk test',
          status_emoji: ':robot_face:',
          status_expiration: 0,
        },
      });
      console.log(`got profile`);
      console.log(result);
    } catch (error) {
      console.error(`got error: ${error.message}`);
    }
  }
})();

[clavin]

The output suggests that the SDK is receiving a response from the API that looks a little like:

HTTP/1.1 200 OK
{
    "ok": false,
    "error": "ratelimited"
}

Whereas the Web API client is expecting that rate limited responses look more like what the docs describe, e.g.:

HTTP/1.1 429 Too Many Requests
Retry-After: 30

I went and actually tested this, and I did notice that the ok: false payload was being delivered with a 200 OK response. Further, this 200 OK response didn't have a Retry-After header.

---

I'm not really sure if this SDK should be supporting this behavior because it's completely undocumented (as far as I could research), making it feel more like a bug with the platform API.

[jayjanssen]

Sorry for the coffeescript, just what I’m most comfortable with.

I kind of suspected it was just a non-standard implementation of rate limiting. @aoberoi — I’m presuming you work for Slack? Is this just something that can be filed internally with your platform api team?

[aoberoi]

Yup, I work for Slack and we’ve logged this one as a bug on the platform level. I’m leaving this issue as a bug to more or less track the progress on that internal issue.

Just some info for those curious, this is an internal rate limit mechanism meant for a different purpose that is “leaking” through the public API because it happens to be more aggressive of a limit than the public one. We have discussed our options and selected one. It’s just a matter of implementing it, testing it, and releasing it. I suspect that will all happen early next week.

Until then, rate limiting for this one method is broken, and the work around would be to manually retry failures that result in a error.data.error === ‘ratelimited’ condition being true.

Also, I don’t mind the coffeescript! I’m just glad you took the time out to report your findings.

[jayjanssen]

@aoberoi absolutely no complaints about having a projected fix so quickly! I like the auto-rate limiting handling feature, I have to write it into my library wrappers for most of my other services.

[nylen]

It is now "next week", for some definition of "next" 😉

@aoberoi Any update on this issue?

[aoberoi]

@nylen the issue hasn't been fixed yet. its a medium priority right now, which means it may be several weeks before its fixed. i'll update here when i hear more.

for now, i think the workaround i mentioned above should be your strategy to deal with this error. are you coming across any difficulties with this? if you're hitting the rate limit very often, we should probably discuss how you can reduce the need for your app to call that method so many times.

[nylen]

the work around would be to manually retry failures that result in a error.data.error === 'ratelimited' condition being true.

This is fine for now, just unexpected and less than ideal. Thanks for the update!

[aoberoi]

Good news friends(@nylen and others interested)! We shipped a change on Friday that fixed this issue. :tada:

I’ll close the issue now, but it would be helpful for someone who experienced it to confirm that it’s back to working as expected.

[amyers]

FWIW, if you do too many users.profiles.get() calls with include_labels: true, you'll get the same http200/ok="false"/error="ratelimited" response as above.

[mdouglass]

Followup since I just hit this -- users.profile.get() with include_labels: false also triggers it.

[lucasconstantino]

So far it seems we don't have an answer for the users.profile.get() issue, do we?