twilight-rs / twilight

Powerful, flexible, and scalable ecosystem of Rust libraries for the Discord API.
https://discord.gg/twilight-rs
ISC License
673 stars 132 forks source link

Cancelling requests in the ratelimit queue delays further requests by 10 seconds #1309

Closed jogramming closed 2 years ago

jogramming commented 2 years ago

(to the same bucket that is)

The following code takes 60 seconds to complete even though only 1 request is fired off and 6 are cancelled:

use std::sync::Arc;

use tracing::info;
use twilight_http::Client;
use twilight_model::id::ChannelId;

#[tokio::main]
async fn main() {
    tracing_subscriber::fmt::init();
    let token = std::env::var("DISCORD_TOKEN").expect("set DISCORD_TOKEN");
    let client = Arc::new(Client::new(token));

    create_cancel_req(client.clone());
    create_cancel_req(client.clone());
    create_cancel_req(client.clone());
    create_cancel_req(client.clone());
    create_cancel_req(client.clone());
    create_cancel_req(client.clone());
    info!("cancelled a bunch of requests");

    // this request wont be fired off until 1 minute has passed even though all the previous requests were cancelled
    client
        .create_message(ChannelId::new(913455776704638996).unwrap())
        .content("test")
        .unwrap()
        .exec()
        .await
        .unwrap();
    info!("finally fired off real request");
}

fn create_cancel_req(client: Arc<Client>) {
    let _fut = client
        .create_message(ChannelId::new(913455776704638996).unwrap())
        .content("test")
        .unwrap()
        .exec();

    // fut is dropped here, cancelling the request
}

This is because of the following: https://github.com/twilight-rs/twilight/blob/2dd9687bda7e4fb98e3348d2de68ad5f72efecc2/http/src/ratelimiting/bucket.rs#L185-L200

In the event of queue_tx.send failing, then _sent will hold Err(tx) stopping it from being dropped and the receive below will timeout after 10 second instead of immediately returning Ok(None)

7596ff commented 2 years ago

This seems to be fixed on next, in the forthcoming http-ratelimiting crate (per #1191):

     Running `target/debug/testing`
2021-11-30T14:35:42.037204Z  INFO testing: cancelled a bunch of requests
2021-11-30T14:35:42.363801Z  INFO testing: finally fired off real request

If this is satisfying, you can go ahead and close the issue. I am hoping to release this at some point soon.