stoiveyp / Slack.NetStandard

.NET Core package that helps with Slack interactions
MIT License
41 stars 16 forks source link

Can the "Retry-After" header be accessed through methods like"Conversation.List"? #44

Closed ProductiveRage closed 3 years ago

ProductiveRage commented 3 years ago

The docs about rate limiting say that there will be a header named "retry-after" that indicates how long the client should pause before retrying. Is that exposed anywhere in the library?

I initially hoped that it would somehow magically appear in the "OtherFields" dictionary but I've had a cursory glance through the source code and couldn't see any headers access and the tests about the Web calls seem to have sample responses that are the response body only and do not include any headers.. so I'm guessing that it wouldn't currently appear in "OtherFields"?

(Note the official docs show it as "Retry-After", with capital letters, while this Medium post indicates that it is "retry-after", all lower case - I'm not sure if one is correct and the other not!)

stoiveyp commented 3 years ago

Hi @ProductiveRage - the retry-after header is part of a 429 status code in the response (there's an example in the Medium post). As this is a non-successful status code the HttpClient will throw this as an exception and won't ever be returned as part of a successful response.

As such this would have to be part of your client code to pick up in exception handling.

ProductiveRage commented 3 years ago

But when I've seen it happen (same with authentication problems), I get a response object from your library but with OK false and the failure code on the Error property. No 429 appears to be thrown from the call that it is make to your code - no exception at all, in fact.

ProductiveRage commented 3 years ago

Reproduce code (created partially to reassure myself that what I was saying was correct and that there isn't an exception that is thrown when I call the library!) -

var client = new SlackWebApiClient("ACCESS TOKEN GOES HERE");
var numberOfCallsQueued = 0;
var numberOfCallsCompleted = 0;
while (true)
{
    ThreadPool.QueueUserWorkItem(async _ =>
    {
        var response = await client.Users.List(limit: 100);
        Console.WriteLine($"Completed {Interlocked.Increment(ref numberOfCallsCompleted)} call(s)");
        if (!response.OK)
        {

        }
    });
    numberOfCallsQueued++;
    if (numberOfCallsQueued % 100 == 0)
        await Task.Yield();
}

The response that I get back when the !response.OK condition is met looks like this:

image

stoiveyp commented 3 years ago

hmm that's odd - if that's coming from a 429 it shouldn't be parsed by my app....I'll take a look

ProductiveRage commented 3 years ago

I think that it might be because in SlackWebApiClient.MakeUrlEncodedCall you don't check the response code on the message from Client.PostAsync and that HttpClient doesn't throw exceptions for non-200 responses, which is why it has an IsSuccessStatusCode property.

Even in error responses, it seems like the Slack API will return data (with ok: false, error: "ratelimited"), as you can see in this SO question where they forgot to include an access token:

stoiveyp commented 3 years ago

Nope - it was because I forgot that I was handling WebExceptions in JSON calls

Just release v2.12.0 and v2.13.0-beta1, which have a RetryAfter TimeSpan property in all web api responses, and I've just used your code to try it and it works.

ProductiveRage commented 3 years ago

Oh, nice one - I'll give that a go! (And it means that I don't have to make many changes to my other code relating to catching other failure cases, which is much appreciated!)

ProductiveRage commented 3 years ago

Ran the repro code again and have got a "RetryAfter" value of 3s - fantastic! Thanks again for the rapid response!