From e4a96394fd158c541d1fe54debb7d4ed90c6e3e2 Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Mon, 11 Nov 2024 13:41:17 +0100 Subject: [PATCH] ndk/looper: Deprecate `poll_all*()` and document implied wake results [Upstream clarified] that due to limitations in `ALooper`, the `poll*()` functions may not be able to distinguish an explicit `wake()` call from any other wakeup source, including callbacks, timeouts, fd events or even errors. Any return value from these functions should be treated as if a `Poll::Wake` event is also returned. While this is not only problematic to tell a wake apart from other events, it makes it impossible for the `poll_all*()` variants to return correctly as they will keep looping internally on callbacks (which are subsuming a wake event). According to [upstream `ALooper_pollAll()` documentation] this is considered "not safe" and the function is no longer allowed to be called. [Upstream clarified]: https://github.com/android/ndk/discussions/2020 [upstream `ALooper_pollAll()` documentation]: https://developer.android.com/ndk/reference/group/looper#alooper_pollall --- ndk/src/looper.rs | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/ndk/src/looper.rs b/ndk/src/looper.rs index d8eb644b..9c040b76 100644 --- a/ndk/src/looper.rs +++ b/ndk/src/looper.rs @@ -73,13 +73,17 @@ bitflags::bitflags! { /// The poll result from a [`ThreadLooper`]. #[derive(Debug)] pub enum Poll<'fd> { - /// This looper was woken using [`ForeignLooper::wake()`] + /// This looper was woken using [`ForeignLooper::wake()`]. + /// + /// # Note + /// All possible return values from any of the `poll*()` functions on [`ThreadLooper`], + /// *including [`Err`]*, may imply that the looper was also woken up. Wake, /// For [`ThreadLooper::poll_once*()`][ThreadLooper::poll_once()], an event was received and processed using a callback. Callback, /// For [`ThreadLooper::poll_*_timeout()`][ThreadLooper::poll_once_timeout()], the requested timeout was reached before any events. Timeout, - /// An event was received + /// An event was received. Event { ident: i32, /// # Safety @@ -118,6 +122,9 @@ impl ThreadLooper { /// Polls the looper, blocking on processing an event, but with a timeout in milliseconds. /// Give a timeout of `0` to make this non-blocking. + /// + /// # Note + /// Any return value, *including [`Err`]*, should be treated as if [`Poll::Wake`] was returned as well. fn poll_once_ms(&self, ms: i32) -> Result, LooperError> { let mut fd = -1; let mut events = -1; @@ -141,6 +148,9 @@ impl ThreadLooper { } /// Polls the looper, blocking on processing an event. + /// + /// # Note + /// Any return value, *including [`Err`]*, should be treated as if [`Poll::Wake`] was returned as well. #[inline] pub fn poll_once(&self) -> Result, LooperError> { self.poll_once_ms(-1) @@ -151,6 +161,9 @@ impl ThreadLooper { /// /// It panics if the timeout is larger than expressible as an [`i32`] of milliseconds (roughly 25 /// days). + /// + /// # Note + /// Any return value, *including [`Err`]*, should be treated as if [`Poll::Wake`] was returned as well. #[inline] pub fn poll_once_timeout(&self, timeout: Duration) -> Result, LooperError> { self.poll_once_ms( @@ -165,6 +178,7 @@ impl ThreadLooper { /// milliseconds. Give a timeout of `0` to make this non-blocking. /// /// This function will never return [`Poll::Callback`]. + #[deprecated = "This function may not return even if [`Self::wake()`] is called. Call one of the `poll_once*()` methods instead."] fn poll_all_ms(&self, ms: i32) -> Result, LooperError> { let mut fd = -1; let mut events = -1; @@ -189,8 +203,10 @@ impl ThreadLooper { /// Repeatedly polls the looper, blocking on processing an event. /// /// This function will never return [`Poll::Callback`]. + #[deprecated = "This function may not return even if [`Self::wake()`] is called. Call one of the `poll_once*()` methods instead."] #[inline] pub fn poll_all(&self) -> Result, LooperError> { + #[allow(deprecated)] self.poll_all_ms(-1) } @@ -201,8 +217,10 @@ impl ThreadLooper { /// /// It panics if the timeout is larger than expressible as an [`i32`] of milliseconds (roughly 25 /// days). + #[deprecated = "This function may not return even if [`Self::wake()`] is called. Call one of the `poll_once*()` methods instead."] #[inline] pub fn poll_all_timeout(&self, timeout: Duration) -> Result, LooperError> { + #[allow(deprecated)] self.poll_all_ms( timeout .as_millis() @@ -309,6 +327,11 @@ impl ForeignLooper { } /// Wakes the looper. An event of [`Poll::Wake`] will be sent. + /// + /// # Note + /// In case there are other events to handle, `poll*()` functions may not return [`Poll::Wake`] + /// at all. Any [`Poll`] event and [`Err`] returned by `poll*()` could imply that the looper + /// has also been woken. pub fn wake(&self) { unsafe { ffi::ALooper_wake(self.ptr.as_ptr()) } }