serenity-rs / serenity

A Rust library for the Discord API.
https://discord.gg/serenity-rs
ISC License
4.73k stars 579 forks source link

Using channels in EventHandler trait #1383

Closed Selyatin closed 3 years ago

Selyatin commented 3 years ago

Hey, I'm trying to use channels inside the EventHandler trait and I was wondering if there's a way to use channels without wrapping it around a Mutex

Right now my code looks like this:

pub struct Handler {
    db: Arc<DB>,
    watch_sender: watch::Sender<Action>,
    watch_receiver: watch::Receiver<Action>,
    info_sender: Arc<Mutex<mpsc::Sender<BotInfo>>>,
    info_receiver: Arc<Mutex<mpsc::Receiver<bool>>>,
    leveling_sender: Arc<Mutex<mpsc::Sender<(UserId, GuildId)>>>,
    bot_options: Arc<BotOptions>,
    afk_users: Mutex<FxHashSet<UserId>>
}

With for example message handler looking like this:

async fn message(&self, context: Context, msg: Message) {
         if let Some(guild_id) = &msg.guild_id {
            let mut leveling_sender = self.leveling_sender.lock().await;

            if let Err(err) = leveling_sender.send((msg.author.id, *guild_id)).await{
                println!("Error while trying to send to leveling task: {}", err);
            }
        }
}

How can this be done better? What would be the standard of implementing something like this?

If I can't use channels without a Mutex, wouldn't just wrapping the actual data around a Mutex and executing the task in the same context be better?

adumbidiot commented 3 years ago

What sender are you using? Most of the ones I know of take a &self to send, so you wouldn't need a mutex.

I'm also not really sure about what you are trying to accomplish with using channels. Are you trying to implement the actor pattern? I usually perform whatever operations are needed in the same task, since serenity spawns a new task for each event that occurs.

Selyatin commented 3 years ago

I'm using tokio's channels mainly the broadcast and mpsc channel to manage multiple bots.

Selyatin commented 3 years ago

And it's not just for channels to be honest, I would love to modify my hashmaps without using a Mutex too.

adumbidiot commented 3 years ago

You can send items through tokio's channels without mutexes since the send function on the tokio channel takes a &self, a read-only reference. You can get a read-only reference pretty easily since you already borrow your handler through a read-only reference. I believe the following will work:

pub struct Handler {
    db: Arc<DB>,
    watch_sender: watch::Sender<Action>,
    watch_receiver: watch::Receiver<Action>,
    info_sender: Arc<Mutex<mpsc::Sender<BotInfo>>>,
    info_receiver: Arc<Mutex<mpsc::Receiver<bool>>>,
    leveling_sender: Arc<mpsc::Sender<(UserId, GuildId)>>,
    bot_options: Arc<BotOptions>,
    afk_users: Mutex<FxHashSet<UserId>>
}
async fn message(&self, context: Context, msg: Message) {
    if let Some(guild_id) = &msg.guild_id {
        if let Err(err) = self.leveling_sender.send((msg.author.id, *guild_id)).await{
            println!("Error while trying to send to leveling task: {}", err);
        }
    }
}

HashMaps will need a mutex since their get_mut function takes a &mut self, assuming you want to mutate the items inside your hashmap and not just read them, since the get function takes a &self.

If you really want to avoid the mutex, I would suggest dashmap or some other concurrent hash map. In a lot of cases though a Mutex<HashMap> will do just fine so think about whether you really need to pull in an extra dependency with potential footguns. dashmap for example will deadlock if you use it incorrectly.

Also, you should avoid using tokio's mutexes unless absolutely necessary. Read this for more info. Basically, a tokio mutex is more expensive than a std::sync::Mutex or parking_lot::Mutex and you really don't want to hold locks across await points unless you are absolutely sure that you have/want to.

By "manage multiple bots" do you mean that you are running multiple bots in a single program? So like each bot will increase the "level" of the user in a shared hashmap store? I think most people would make a shared database server or something and split each bot into a separate program at that point, but channels are a good of a way as any to synchronize shared data in-program. Personally, I think that you might as well just share the backing hashmap/database though since I don't see any benefits to using channels?

Selyatin commented 3 years ago

Channels are used to retrieve information from each bot and also to inform tasks to end themselves when a bot is closed. The reason why I used tokio's Mutex is so that when there are lots of messages incoming they won't block one another but maybe that's not needed since there's some delay between the messages. I've also fixed the senders, they're not using mutexes anymore and since serenity spawns a new task to deal with each message anyways there's no need for using channels. I was thinking that I could keep processing messages and just send the leveling up stuff to another task that'll constantly loop and talk with the database to persist the data etc, it made sense before you told me that serenity already spawns a new task for each message, so that architecture probably won't gain me anything.

Selyatin commented 3 years ago

By managing I don't just mean running a bot, I mean configuring, running, starting/closing bots via a rest api, if it were implemented as each bot to be running as a separate process then each bot would need some sort of way of communicating with the main process, which is just a pain in the ass to handle and at that point there would be no point of using rust anyways, since you can do that with node.js or python in a much easier way, but when you're managing all bots in a single process using channels to communicate with them it becomes much easier to manage.