Skip to content

native_activity: Only wait for state to update while main thread is running #187

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

MarijnS95
Copy link
Member

@MarijnS95 MarijnS95 commented Mar 12, 2025

We see that some Android callbacks like onStart() deadlock, specifically when returning out of the main thread before running any event loop (but likely also whenever terminating the event loop), because they don't check if the thread is still even running and are otherwise guaranteed receive an activity_state update or other state change to unblock themselves.

This is a followup to #94 which only concerned itself with a deadlock caused by a destructor not running because that very object was kept alive to poll on the destroyed field that destructor was supposed to set, but its new thread_state can be reused to disable these condvar waits when the "sending" thread has disappeared.

Separately, that PR mentions Activity recreates because of configuration changes which isn't supported anyway because Activity is still wrongly assumed to be a global singleton (I am still working on fixing all this 🤞!).


Note that I'm not at all going to claim this to be really fixing anything, it's more of a bandaid on top of another bandaid with doubts if it'll stick.

#94 seems to have only considered thread state, which makes sense given that finish() is also unconditionally called when the "main" thread created by android-activity returns, but the mentioned missing drop() handling of WaitableNativeActivityState also seems to very specifically exist because AndroidApp is explicitly and intentionally cheaply Cloneable and Send/Sync. If anything the user could move things to a different thread to handle their looping behaviour there. Liveness of these objects is an indicator that the user may/will still poll elsewhere, or at least has the ability to. That should likely have been the predicate to exit out of various while loops rather than the thread state, but here we are.

Add to that the request to not start a thread for the user in the first place (requires a redesign to signify that users must timely return from their not-really-main()-anymore), so that they can load things from JNIEnv which uses prior Java stackframes to resolve classes that cannot otherwise be found without poking at Context.getClassLoader()...

…unning

We see that some Android callbacks like `onStart()` deadlock,
specifically when returning out of the main thread before running
any event loop (but likely also whenever terminating the event loop),
because they don't check if the thread is still even running and are
otherwise guaranteed receive an `activity_state` update or other state
change to unblock themselves.

This is a followup to [#94] which only concerned itself with a deadlock
caused by a destructor not running because that very object was kept
alive to poll on the `destroyed` field that destructor was supposed to
set, but its new `thread_state` can be reused to disable these condvar
waits when the "sending" thread has disappeared.

Separately, that PR mentions `Activity` recreates because of
configuration changes which isn't supported anyway because `Activity` is
still wrongly assumed to be a global singleton.

[#94]: https://togithub.com/rust-mobile/android-activity/pull/94
@MarijnS95 MarijnS95 force-pushed the fix-deadlock-on-terminate branch from c841f66 to 5a82603 Compare March 12, 2025 14:12
@MarijnS95 MarijnS95 requested a review from rib March 13, 2025 13:18
@@ -468,7 +470,9 @@ impl WaitableNativeActivityState {
if guard.pending_window.is_some() {
guard.write_cmd(AppCmd::InitWindow);
}
while guard.window != guard.pending_window {
while guard.thread_state == NativeThreadState::Running
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess all of these (maybe with an exception for input events) should probably be checking for != NativeThreadState::Stopped so that callbacks effectively buffer during initialization and allow the app to start handling events or else exit and have this transition to Stopped.

The protocol for notifying the app about a window is supposed to be synchronous and it would be a significant change to have these start being async until the thread is initialized and 'running'.

@rib
Copy link
Member

rib commented Mar 20, 2025

If anything the user could move things to a different thread to handle their looping behaviour there. Liveness of these objects is an indicator that the user may/will still poll elsewhere, or at least has the ability to.

Btw, it's explicitly documented that polling is only allowed on the android_main thread. I had been meaning to add explicit assertions for that, but I guess maybe didn't do that.

Ref:

//! # Send and Sync [AndroidApp]
//!
//! Although an [AndroidApp] implements Send and Sync you do need to take
//! into consideration that some APIs, such as [AndroidApp::poll_events()] are
//! explicitly documented to only be usable from your android_main() thread.

@rib
Copy link
Member

rib commented Mar 20, 2025

It looks like I put a warning that poll_events can panic if used from a non-main thread but it's a hollow threat currently :)

/// # Panics
///
/// This must only be called from your android_main() thread and it may panic if called
/// from another thread.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants