Question: Can force notifying the eventfd leave should_notify in a bad state?

[tontinton]

Notifying a thread to wake up function in glommio:

    pub(crate) fn notify(&self, force: bool) {
        if self
            .should_notify
            .compare_exchange(true, false, Ordering::Relaxed, Ordering::Relaxed)
            .is_ok()
            || force
        {
            write_eventfd(self.eventfd_fd());
        }
    }

From what I understand when force is true, compare_exchange might fail, but we still wake up that thread, meaning a thread can be woken up, while its should_notify remains true.

The only thing I can think of that is that the next time someone calls notify on that thread, we will write to the eventfd, even though that thread is already running.

Really minor, just thought to share after noticing it to understand whether this was thought of and just not interesting enough to actually set should_notify when force is true, something like this:

    pub(crate) fn notify(&self, force: bool) {
        if force {
            self.should_notify.store(false, Ordering::Relaxed);
            write_eventfd(self.eventfd_fd());
        } else if self
            .should_notify
            .compare_exchange(true, false, Ordering::Relaxed, Ordering::Relaxed)
            .is_ok() {
            write_eventfd(self.eventfd_fd());
        }
    }

[vlovich]

I don't see any functional difference in what you've suggested and what's written in the codebase today. Today, regardless of the value of force, should_notify is reset to false. In your version, that's also true. Today, we write to eventfd iff we observed should_notify as true OR force. With your suggestion, that's also true.

Are you suggesting the latter version is easier to read?

[tontinton]

compare_exchange(true, false)

Returns ok only when the previous state was true and changed to false, meaning that it could fail setting to false, when the previous value was not true.

Basically, the atomic operation fails if the comparison fails.

But now I understand that the only reason the atomic operation fails is if should_notify was already false, making my version of setting it to false is redundant.

Closing :)