raw_sync-rs icon indicating copy to clipboard operation
raw_sync-rs copied to clipboard

Allow spurious wake up for handling cancellation

Open xixixao opened this issue 4 years ago • 3 comments

Context: I am using this library (and shared-memory) to build a simpler cross-process synchronization/communication protocol on top of it (I'm using it for hot reloading but it could be used for anything).

Here's the problem: On macOS, two threads cannot wait on the same cond variable. If this happens, the wait errors out with EINVAL (see https://wiki.sei.cmu.edu/confluence/display/c/POS53-C.+Do+not+use+more+than+one+mutex+for+concurrent+waiting+operations+on+a+condition+variable).

Now this is a problem for me because I need to be able to kill a first thread while it's waiting for an Event and spawn a new one. This is not some crazy kill, it's a standard SIGTERM. If I just let the OS do it, the mutex is reaquired and I get the error (I think https://linux.die.net/man/3/pthread_cond_wait has the relevant info on what happens on cancellations). This is a sucky behavior but I'm sure Cupertino experts had a good reason for it.

So to handle this, I need to use a custom signal hook and let the thread wake up. The problem is, this library is already handling the spurious wake ups, and ignores them. I need a way to hook into them.

I'm happy to rework this in any way if you can think of a better API.

The "client" of this library then does something like this:

    #[cfg(target_family = "unix")]
    {
      let is_being_killed = Arc::new(AtomicBool::new(false));
      let is_being_killed_for_handler = Arc::clone(&is_being_killed);
      let hook_id = unsafe {
        signal_hook::low_level::register(signal_hook::consts::signal::SIGTERM, move || {
          is_being_killed_for_handler.store(true, Ordering::Relaxed);
        })
        .unwrap()
      };
      while matches!(
        event.wait_allow_spurious_wake_up(Timeout::Infinite).unwrap(),
        EventState::Clear
      ) {
        if is_being_killed.load(Ordering::Relaxed) {
          signal_hook::low_level::unregister(hook_id);
          // This should probably be `unsafe { llibc::kill(std::process::id() as i32, libc::SIGTERM); }` 
          // but it doesn't suspend the thread for me 🤷.
          std::process::abort();
        }
      }
    }
    #[cfg(not(target_family = "unix"))]
    {
      self.memory.event.wait(Timeout::Infinite).unwrap();
    }

xixixao avatar Jan 10 '21 06:01 xixixao

This addresses the originally reported issue in #5 (if you press ctrl+c) - funny I searched the issues only after figuring all this out.

xixixao avatar Jan 10 '21 06:01 xixixao

Hi xixixao,

I finally had some spare time to look into this and think i understand what issue #5 was all about haha.

I am currently cleaning up some of this code in the release_0.2.0 branch.

I will add a trait EventImplExt (to try to follow Rust's std style) for unix where you will be able to call a function like the one in your PR.

I will let you know here when I am done the changes if you wish to test them to make sure you are getting the wanted behavior.

elast0ny avatar Jan 19 '21 01:01 elast0ny

Hi, i have implemented the allow_spurious_wake feature through the EventImplExt::inner_wait function.

Feel free to test the implementation from the release_0.2.0 branch

The default behavior of EventImpl::wait remained unchanged (ignores spurious wake) to stay consistent across platforms

elast0ny avatar Jan 19 '21 02:01 elast0ny