PacktPublishing / Asynchronous-Programming-in-Rust

Asynchronous Programming in Rust, published by Packt
MIT License
172 stars 55 forks source link

Potential race condition of event registration and adding waker? #23

Open npuichigo opened 2 months ago

npuichigo commented 2 months ago

@cfsamson Would it be possible that after the interest is registered, the reactor quickly get the event and tries to call wake, but waker has not been added since lock here. Now that epoll is edge-level here, the event will never be received again so never wake the executor. I'm not sure if I'm right.

https://github.com/PacktPublishing/Asynchronous-Programming-in-Rust/blob/04d893c7bc8f83e64fbf5e3fa11677a8b1244830/ch10/a-rust-futures-bonus/src/http.rs#L69-L70

cfsamson commented 2 months ago

Hi!

Good catch! As far as I can see you're right in theory. Even though it's unlikely in this example since a network call is so slow (even locally) that it's highly unlikely that you could get in trouble here. However, if you the response could return much quicker, you could have the situation that the reactor loop recieves an event, takes the lock on waker first and finds no wakers for that event and just goes back to sleep. In that case, we could risk never getting a notification again just as you reasoned.

As I see it, there are two options here:

  1. Implement something like register_with_waker that takes the lock on wakers before registering interest and releases it when the waker is registered so that if the reactor recieves an event, it will block on the mutex until the waker is registered and it can take the lock.
  2. Move the registration of interest and waker to before we make the call to write_request(), which is less robust and more of a hack.
        if self.stream.is_none() {
            println!("FIRST POLL - START OPERATION");
            // CHANGED
            let stream = (&mut self).stream.as_mut().unwrap();
            runtime::reactor().register(stream, Interest::READABLE, id);
            runtime::reactor().set_waker(waker, self.id);
            // ============
            self.write_request();
        }

The first is the best since it makes it less likely to make the same error again.

I'll note this down as an improvement for a second revision if it comes to that, for now, let's just leave this issue open so it's possible to find it.

Thanks again for reporting!