slackapi / java-slack-sdk

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

Rate limited team reported differs from request team #1216

Closed igorrpessoa closed 8 months ago

igorrpessoa commented 11 months ago

Hey, for some reason we have some users being rate limited for different teams that they belong to. For example, we have the team_id T012ABCDE0A and a user_id U012ABCDE01 being rate limited even though he should not because of the number of requests done at the last minute, and we can see this in the logs from logger com.slack.api.methods.impl.AsyncRateLimitExecutor

Got a rate-limited response from users.profile.set API (team: T987PQRS0X0, error: status: 429, error: ratelimited, needed: null, provided: null, warning: null, retry-after: 6)

As you can see, the team reported as being rate limited differs from the one that made the request. This is happening for the users.profile.set method.

One of our attempts to solve this was to implicitly add the user token to the request to make sure we were using the correct team_id later on. We also created a Slack bug report and it seems like in this case, the user being rate limited was not actually being rate limited at that time.

Reproducible:

@Factory
public class SlackClientFactory {
  @Singleton
  Slack make(@Named(ExecutorServiceFactory.SLACK_CLIENT_EXECUTOR_NAME) ScheduledExecutorService executorService) {
    final SlackConfig config = new SlackConfig();

    final ExecutorServiceProvider executorServiceProvider = new ExecutorServiceProvider() {
      @Override
      public ExecutorService createThreadPoolExecutor(String threadGroupName, int poolSize) {
        return executorService;
      }

      @Override
      public ScheduledExecutorService createThreadScheduledExecutor(String threadGroupName) {
        return executorService;
      }
    };
    config.setExecutorServiceProvider(executorServiceProvider);

    return Slack.getInstance(config);
  }
}

public class SlackUserClient {
  private final AsyncMethodsClient methods;

  public SlackUserClient(SlackUser slackUser, Slack slack) {
    methods = slack.methodsAsync(slackUser.getAccessToken(), slackUser.getTeamId());
  }

  public CompletableFuture<UsersProfileSetResponse> setUserProfile() {
    final UsersProfileSetRequest.UsersProfileSetRequestBuilder usersProfileSetRequestBuilder = 
    UsersProfileSetRequest.builder()
                    .token(slackUser.getAccessToken())
                    .profile(profile)
                    .user(slackUser.getUserId());
    final UsersProfileSetRequest usersProfileSetRequest = usersProfileSetRequestBuilder.build();
    return methods.usersProfileSet(usersProfileSetRequest);
  }
}

The Slack SDK version

implementation 'com.slack.api:slack-api-client:1.30.0'

Java Runtime version

Java 17.0.6_10

OS info

Linux/X86_64

WilliamBergamin commented 11 months ago

Hello @igorrpessoa thanks for sharing this 💯

Are the workspaces you are testing this on Enterprise Grid or Orgs? it may be possible that the team_id returned is the org level one instead of the individual workspace

This seems to be a server-side issue, did the slack bug report indicate that this was an issue specific to the java-slack-sdk?

igorrpessoa commented 11 months ago

Hi, thanks for the response! I think we have it in orgs because we do not support Enterprise Grid. This is happening to a couple of different users that should not have anything in common. Two different companies, workspaces, and users that do not share the same workspaces. Also looked at the tokens to ensure there weren't any duplicates. I reached them and handled the team_id and user_id that were being rate limited, and this was their response:

Interestingly, I can't see your app getting ratelimited at that time. That combined with the wrong team_id being returned from the SDK, I wonder is there a potential issue with the SDK itself.

Can you create a new issue directly on the SDK below:

https://github.com/slackapi/java-slack-sdk/issues
Creating an issue there will put you in contact with the SDK developers directly, so hopefully they should be able to point you in the right direction.
WilliamBergamin commented 11 months ago

Thanks for sharing this information @igorrpessoa 👍

I took a closer look into the code you shared above, if you could share more it would be greatly appreciated 💯

From what I can tell it seems like you may be creating a singleton instance of the Slack object and and updating it asynchronously, this may be causing some concurrency issues within the singleton since there may be multiple processes updating its values while others are using it to send requests.

I would recommend creating a Slack client instance for each user token being used, this should avoid any situation where multiple users try to use the Slack client at the same time with different user_tokens. In the Bolt frameworks we tend to implement a similar logic to avoid this race condition.

igorrpessoa commented 11 months ago

Thanks for the response! We are actually doing this. The only singleton instance that we are using is the Slack object as it's recommended. Here is the part that we create SlackClient object, usually called after searching SlackUser and SlackBot data saved in our DB.

    protected SlackClient client(@Nullable User user, SlackUser slackUser, SlackBot bot) {
        final Logger log = LOG.with(user, slackUser, bot);

        return new SlackClient(user, slackUser, bot, new SlackBotClient(bot, slack), new SlackUserClient(slackUser, slack), this);
    }

As the earlier code shared, SlackBotClient and SlackUserClient are the classes that intantiates the AsyncMethodsClient for Bot and User requests.

public class SlackBotClient {

 private final AsyncMethodsClient methods;

    public SlackBotClient(SlackBot slackBot, Slack slack) {
        this.slackBot = slackBot;
        methods = slack.methodsAsync(slackBot.getAccessToken(), slackBot.getTeamId());

    }
...
}

public class SlackUserClient {
    private final AsyncMethodsClient methods;

    public SlackUserClient(SlackUser slackUser, Slack slack) {
        methods = slack.methodsAsync(slackUser.getAccessToken(), slackUser.getTeamId());
    }

    public CompletableFuture<UsersProfileSetResponse> setUserProfile(UsersProfileSetRequest usersProfileSetRequest) {
        return methods.usersProfileSet(usersProfileSetRequest);
    }
...
}

And here is where we call and log the information

          final UsersProfileSetRequest params = UsersProfileSetRequest.builder()
                  .user(slackUser.getUserId())
                  .token(slackUser.getAccessToken())
                  .profile(profile)
                  .build();
 client.setUserProfile(params).thenAccept(result -> {
                if (result.isOk()) {
                    userlog.info("Slack status cleared");
                } else {
                    userlog.add("error", result.getError()).error("Failed to clear Slack status");
                }
            });
github-actions[bot] commented 9 months ago

👋 It looks like this issue has been open for 30 days with no activity. We'll mark this as stale for now, and wait 10 days for an update or for further comment before closing this issue out. If you think this issue needs to be prioritized, please comment to get the thread going again! Maintainers also review issues marked as stale on a regular basis and comment or adjust status if the issue needs to be reprioritized.

github-actions[bot] commented 8 months ago

As this issue has been inactive for more than one month, we will be closing it. Thank you to all the participants! If you would like to raise a related issue, please create a new issue which includes your specific details and references this issue number.