mvniekerk / tokio-cron-scheduler

Schedule tasks on Tokio using cron-like annotation
Apache License 2.0
529 stars 59 forks source link

Improve code quality #65

Closed JakkuSakura closed 3 months ago

JakkuSakura commented 6 months ago

Hi, thanks for the effort being done.

Recently, I hit an issue that if scheduler.spawn() is called in the code, without actual tasks, my server blocks.

I started reviewing code in this crate. I noticed the following points to improve

  1. use of Arc<Mutex<bool>> can be written as Arc<AtomicBool>
  2. use of
    tokio::spawn(async move {
    if let Err(e) = tx.send(uuid) {
        error!("Error sending deletion {:?}", e);
    }
    });

    can be replaced with just tx.send()

  3. use of
    let mut start_rx: Option<Receiver<bool>> = None;
    std::mem::swap(&mut start_rx, &mut *w);
    start_rx

    can be replaced as w.take()

haydenflinner commented 6 months ago

Interestingly, the ::init function is not mentioned in the example of how to use the scheduler, it gets called implicitly when adding a job. But we don't use atomic swap for writing to inited, just acquire a lock and then write true, so it seems that if someone had a few clones of the JobScheduler (uninitted) and called add on them at the same time, the actors may get started twice. Seems to be likely harmless, though.

mvniekerk commented 3 months ago

Hi @JakkuSakura, thank you for the report! Thank you @haydenflinner for also commenting.

I'll have a look in these issues mentioned

mvniekerk commented 3 months ago

@JakkuSakura for 0.10.3, I've added the suggestions as per 1 and 2. For 3, my intention is to let whatever delete handlers to be notified on deletion. In that, if something goes wrong with one of them, then the issue is isolated per that spawn of deletion.

Please re-open this issue if you've got more, I appreciate it.

JakkuSakura commented 2 months ago

for 3, I don't comment on delete behavior I mean

let mut start_rx: Option<Receiver<bool>> = None;
std::mem::swap(&mut start_rx, &mut *w);
start_rx

is effectively

let start_rx = w.take();
start_rx

they do exactly the same thing, but the latter is clearer and shorter