Retry after delay

[JoschuaL]

So, as far as I know there is not a build-in way in the library to handle 429 responses (too many requests) coming from a graph call when throttling kicks in.

I also feel like the current Library Api makes it really hard to handle retries in a generic way.

I can't create a generic function that takes a IntoResponse<T, Clinet> as it does not implement clone (to send, parse the error, and then send again after a delay)

I can't pass the GraphResult, as it does not feature any way to recreate the request.

GraphResponse contains all the info needed in theory, but since a 429 will lead to a GraphFailure, I dont get one.

And finally, GraphFailure does also not contain the needed information like url to retry the request.

So, does anyone have hints how to handle retries in a somewhat generic way where I dont have to copy paste a bunch of code everywhere i make a request?

[sreeise]

Yeah so there are a couple of reasons why the functionality with IntoResponse works the way it does.

The first is that it has to handle both blocking and asynchronous request types and it does this by using a generic value: Client and IntoResponse also requires that Client be an implementation of the RequestClient trait which just requires that specific methods be implemented for a Client. There are several reasons for this is but the main one is that its easier to scale. If blocking/async had to be done separately it would mean writing every api macro call twice and every response type twice to some degree.

https://github.com/sreeise/graph-rs/blob/4f4b1a718f7ec26049486538838c74c9a91cd8f1/graph-http/src/intoresponse.rs#L19-L29

The second reason gets to the point you bring up which is that the GraphResult does not provide relevant information to the request that was made. When there is an error that happens earlier in the process of the request being built, not as a result of the request such as is with a timeout, the error is actually stored and deferred, and then returned as the result of that eventual Send call. This makes it so that there are no unwanted unwrap calls internally and also so that the caller doesnt have to unwrap several times or handle errors in several different places. But it also means we don't or may not have information about the request, not that we couldn't, only that its design currently does not allow for it.

I think what your getting at though is important. We don't necessarily want to force having to rewrite a request every time they need to perform the same request. A simple solution from the callers perspective is just to write functions that call the same request. But that would just add more work on the caller.

Changes to what is returned in the GraphResult object wouldnt be too difficult and I think that is probably a smart thing to do for information purposes. I am interested in how your making calls that would require a URL? The client builds out the URL so you typically don't need that unless your running the request without the client. Which you can totally do but it would just defeat the purpose of using an api client.

To be honest I think in some ways a retry-able request object is more reliable because it contains the exact information you need without having to recreate the same request. And I have wanted something like this for a while as well.

[sreeise]

Also possibly something like having a type that can be passed like a request might be a good idea as well.

client.send<CustomResponseType>(value)

Possibly a trait that could be used. I havnt thought about it too much but I could see this having some usefulness.

[JoschuaL]

The reason i would want the url was because i noticed this when doing batch requests, which afaik can currently only be done by specifying the url manually. Since a batch request can have up to 20 requests but the limit for requests per second is 30 one often runs into the station where half of the requests succeed and half error. So in this situation recovering the url would be almost sufficient for retrying. but in the general case i suppose not.

[sreeise]

Yeah I get what your saying. I forgot about the batch requests. That does remind me that the crate needs to add a type that can make it easier to build batch requests instead of the caller having to write everything explicitly. With any api request if there is a body its been generally left up to the caller to implement it in whatever form they choose as long as it can be transformed using serde_json. But in this case I think it makes sense to provide another way to write the body because its also a list of request objects which would normally be handled by the client.

It does make sense to provide as much information as possible about each request that took place. Especially when its something like a batch request where your may not be exactly sure which of the requests failed or just need more information to do a retry.

[sreeise]

Filed ticket: #363
I summarized your main points in the ticket to make sure those get addressed as well.

[sreeise]

I mentioned this in another discussion as well but wanted to just add here that I gladly welcome contributions to this or any other ticket as well if anyone would like too. Not a requirement at all but if you do want to contribute I can also help answer any questions.

[sreeise]

Filed ticket. : #363

Thanks for the feedback!

[JoschuaL]

So one thing i can be up with that might be a reasonable way to handle this would be a send_with_retry function implemented on IntoResponse (analogue to the send function) that could be configured how and when to retry a request. The configuration could be done through a builder pattern struct that would contain the number of retires, a list of status codes to retry on, and potentially a Fn(GraphResult) -> bool (or maybe GraphResponse or even T) with a default implementation that just retries 1 or 2 times on any GraphError. I'll see if I can code a pull request together, though I'm not promising anything.

[JoschuaL]

ups, i guess this could go into the issue discussion directly

[sreeise]

ups, i guess this could go into the issue discussion directly

Either way works. 👍

[DevLazio]

Following the release of the 1.0 version I would like to add my 2 cents.

I have been using the tokio-retry crate to implement retries, either from 429 or incorrect responses from Azure (yes, it happens sometimes).
Example :

match Retry::spawn((retry_strategy.clone()), || async {
        client
            .reports()
            .get_office_365_services_user_counts_by_period(period)
            .send().await.map_err(|e| SimpleError::from(e))?
            .text()
            .await.map_err(|e| SimpleError::from(e))
    }).await {
        Ok(report) => {
           // cut
        }
        Err(e) => { error!( "Error while processing {} : {:?}" , (format!("office365_services_user_counts_{}", period).as_str()) , e ) }
    }

It's a bit cumbersome, because of the (new) need to have a mutable ref to client if you need to switch from v1 and beta often like I do (not possible in an async block) and the need to encapsulate error now that there is multiple function that can fail on the same chain.

I can take a look to implement an internal retry if I can find time for that.
Building upon the new GraphClientConfiguration (nice addition btw) seems obvious but do you have your mind set for the internal details @sreeise ?

[sreeise]

Yeah feel free to take a look at it and reopen the related ticket on this discussion or open a new one. I'm all for it. I am not gonna have as much time starting on April 10th because I start a new job so I won't necessarily be able to put a ton of time into it but im down to help.

For sure there was no good way to do it without having a mutable ref. Before there wasn't a need for a mutable ref because the underlying code was using a RefCell for blocking and an Arc for async which was not exactly the best performance wise since reqwest was already using Arc underneath and it was kinda like extra for no reason.

With that said, one possible short fix you could do for that is add a method onto the client to start out with that:

    pub fn new_beta(access_token: &str) -> Graph {
        Graph {
            client: Client::new(access_token),
            endpoint: PARSED_GRAPH_URL_BETA.clone(),
        }
    }

Or another thing you could do is add a derive for Clone to the client so you can clone it easily and have one use v1 and one use beta. Ive already added a ticket for adding derive traits to the Graph client: #410

If you get a chance. see if you can show a full example maybe so I can kinda see what issues your running into and what your solving with using the tokio retry. Just so I can be of more help to you in adding something useful to the crate.

I don't have anything specific in mind. Also, adding the endpoint to the GraphClientConfiguration could be done as well.