DataDog / glommio

Glommio is a thread-per-core crate that makes writing highly parallel asynchronous applications in a thread-per-core architecture easier for rustaceans.
Other
3.12k stars 164 forks source link

Timer documentation says confusing things about the task queue #654

Closed vlovich closed 6 months ago

vlovich commented 7 months ago

There's this scary sounding warning around Timer:

Note that because of that, Timers always block the current task queue in which they currently execute.

This warning is not repeated for sleep which uses Timer internally. I think all it's saying is that it'll delay the current async task but the wording seems to be fairly precise. I'm unclear what it's trying to say because I've not been able to reproduce an example justifying this warning.

This code:

    #[test]
    fn does_timer_block_task_queue() {
        test_executor!(async move {
            let task_queue = crate::executor().current_task_queue();
            let t1 = crate::executor().spawn_local_into(async move {
                for i in 0..10 {
                    eprintln!("t1 sleep # {i} in tq");
                    Timer::new(Duration::from_millis(100)).await;
                    eprintln!("t1 finished sleep # {i}");
                }
            }, task_queue).unwrap();
            let t2 = crate::executor().spawn_local_into(async move {
                eprintln!("Running t2 in tq");
                Timer::new(Duration::from_secs(1)).await;
                eprintln!("Finished running t2");
            }, task_queue).unwrap();

            futures::join!(t1, t2);

        });
    }

generates this output:

t1 sleep # 0 in tq
Running t2 in tq
t1 finished sleep # 0
t1 sleep # 1 in tq
t1 finished sleep # 1
t1 sleep # 2 in tq
t1 finished sleep # 2
t1 sleep # 3 in tq
t1 finished sleep # 3
t1 sleep # 4 in tq
t1 finished sleep # 4
t1 sleep # 5 in tq
t1 finished sleep # 5
t1 sleep # 6 in tq
t1 finished sleep # 6
t1 sleep # 7 in tq
t1 finished sleep # 7
t1 sleep # 8 in tq
t1 finished sleep # 8
t1 sleep # 9 in tq
Finished running t2
t1 finished sleep # 9

We can see that t2 starts running while there's a Timer.await in the same task queue. Similarly t1 keeps running while t2 is sleeping in the same task queue. Don't see how the task queue is blocked.

Or "blocked" means something more specific and could use some elaboration.

vlovich commented 7 months ago

UPDATE: Never mind. The reason I was seeing blocking is because I had a background task spinning on a future that always returned an error so it never yielded. So not relevant to the above & still confused by the note.

Ok, I am seeing scenarios where the Timer never returns if I await a tokio oneshot receiver and attach an .or with a timer to that future:

   let (_tx, rx) = tokio::oneshot::channel();
   rx.or(async move {
      glommio::sleep(Duration::from_millis(30));
   }).await // Deadlock!!!

The timeout code does work if I use glommio::timeout so I suspect this deadlock is what the Timer docs refer to. What's confusing is that on the front page of the docs the example has:

    let timeout = async {
        Timer::new(Duration::from_secs(10)).await;
        Err(io::Error::new(io::ErrorKind::TimedOut, "").into())
    };
    let stream = TcpStream::connect("::80").or(timeout).await?;

which would then seem like an erroneous example to have without the warning / an explanation of why this example might work but others following the same pattern won't in other cases (or maybe the example itself doesn't work which would be even worse).

vlovich commented 7 months ago

@glommer if you get a chance I'd love to understand what the warning means if you remember.

glommer commented 7 months ago

The wording there is not the best, since it doesn't really block it. What it means is that given Rust's concurrency model, calling .await will - as you put it - delay the current task.

I think I still had the seastar model in my mind when I wrote that code, in which a future starts executing immediately when you call it, and in my mind this behavior was "blocking"-like (but hey, it's been a while)

vlovich commented 7 months ago

Ok. I'll put up a PR to clear up the wording.