Merge pull request #133 from rust-mobile/marijn/bail-log-thread-on-re…

…ad_line-error Stop log-forwarding thread on IO errors
rust-mobile · Nov 20, 2023 · bfd8bfd · bfd8bfd
2 parents 98aef99 + af34189
commit bfd8bfd
Show file tree

Hide file tree

Showing 3 changed files with 55 additions and 73 deletions.
diff --git a/android-activity/src/game_activity/mod.rs b/android-activity/src/game_activity/mod.rs
@@ -1,21 +1,17 @@
 #![cfg(feature = "game-activity")]
 
 use std::collections::HashMap;
-use std::ffi::{CStr, CString};
-use std::fs::File;
-use std::io::{BufRead, BufReader};
 use std::marker::PhantomData;
 use std::ops::Deref;
-use std::os::unix::prelude::*;
 use std::panic::catch_unwind;
+use std::ptr;
 use std::ptr::NonNull;
 use std::sync::Weak;
 use std::sync::{Arc, Mutex, RwLock};
 use std::time::Duration;
-use std::{ptr, thread};
 
 use libc::c_void;
-use log::{error, trace, Level};
+use log::{error, trace};
 
 use jni_sys::*;
 
@@ -29,9 +25,9 @@ use ndk::native_window::NativeWindow;
 use crate::error::InternalResult;
 use crate::input::{Axis, KeyCharacterMap, KeyCharacterMapBinding};
 use crate::jni_utils::{self, CloneJavaVM};
-use crate::util::{abort_on_panic, android_log, log_panic};
+use crate::util::{abort_on_panic, forward_stdio_to_logcat, log_panic, try_get_path_from_ptr};
 use crate::{
-    util, AndroidApp, ConfigurationRef, InputStatus, MainEvent, PollEvent, Rect, WindowManagerFlags,
+    AndroidApp, ConfigurationRef, InputStatus, MainEvent, PollEvent, Rect, WindowManagerFlags,
 };
 
 mod ffi;
@@ -617,21 +613,21 @@ impl AndroidAppInner {
     pub fn internal_data_path(&self) -> Option<std::path::PathBuf> {
         unsafe {
             let app_ptr = self.native_app.as_ptr();
-            util::try_get_path_from_ptr((*(*app_ptr).activity).internalDataPath)
+            try_get_path_from_ptr((*(*app_ptr).activity).internalDataPath)
         }
     }
 
     pub fn external_data_path(&self) -> Option<std::path::PathBuf> {
         unsafe {
             let app_ptr = self.native_app.as_ptr();
-            util::try_get_path_from_ptr((*(*app_ptr).activity).externalDataPath)
+            try_get_path_from_ptr((*(*app_ptr).activity).externalDataPath)
         }
     }
 
     pub fn obb_path(&self) -> Option<std::path::PathBuf> {
         unsafe {
             let app_ptr = self.native_app.as_ptr();
-            util::try_get_path_from_ptr((*(*app_ptr).activity).obbPath)
+            try_get_path_from_ptr((*(*app_ptr).activity).obbPath)
         }
     }
 }
@@ -913,33 +909,7 @@ extern "Rust" {
 #[no_mangle]
 pub unsafe extern "C" fn _rust_glue_entry(native_app: *mut ffi::android_app) {
     abort_on_panic(|| {
-        // Maybe make this stdout/stderr redirection an optional / opt-in feature?...
-
-        let file = {
-            let mut logpipe: [RawFd; 2] = Default::default();
-            libc::pipe2(logpipe.as_mut_ptr(), libc::O_CLOEXEC);
-            libc::dup2(logpipe[1], libc::STDOUT_FILENO);
-            libc::dup2(logpipe[1], libc::STDERR_FILENO);
-            libc::close(logpipe[1]);
-
-            File::from_raw_fd(logpipe[0])
-        };
-
-        thread::spawn(move || {
-            let tag = CStr::from_bytes_with_nul(b"RustStdoutStderr\0").unwrap();
-            let mut reader = BufReader::new(file);
-            let mut buffer = String::new();
-            loop {
-                buffer.clear();
-                if let Ok(len) = reader.read_line(&mut buffer) {
-                    if len == 0 {
-                        break;
-                    } else if let Ok(msg) = CString::new(buffer.clone()) {
-                        android_log(Level::Info, tag, &msg);
-                    }
-                }
-            }
-        });
+        let _join_log_forwarder = forward_stdio_to_logcat();
 
         let jvm = unsafe {
             let jvm = (*(*native_app).activity).vm;

diff --git a/android-activity/src/native_activity/glue.rs b/android-activity/src/native_activity/glue.rs
@@ -3,23 +3,17 @@
 //! synchronization between the two threads.
 
 use std::{
-    ffi::{CStr, CString},
-    fs::File,
-    io::{BufRead, BufReader},
     ops::Deref,
-    os::unix::prelude::{FromRawFd, RawFd},
     panic::catch_unwind,
     ptr::{self, NonNull},
     sync::{Arc, Condvar, Mutex, Weak},
 };
 
-use log::Level;
 use ndk::{configuration::Configuration, input_queue::InputQueue, native_window::NativeWindow};
 
 use crate::{
     jni_utils::CloneJavaVM,
-    util::android_log,
-    util::{abort_on_panic, log_panic},
+    util::{abort_on_panic, forward_stdio_to_logcat, log_panic},
     ConfigurationRef,
 };
 
@@ -834,32 +828,7 @@ extern "C" fn ANativeActivity_onCreate(
     saved_state_size: libc::size_t,
 ) {
     abort_on_panic(|| {
-        // Maybe make this stdout/stderr redirection an optional / opt-in feature?...
-        let file = unsafe {
-            let mut logpipe: [RawFd; 2] = Default::default();
-            libc::pipe2(logpipe.as_mut_ptr(), libc::O_CLOEXEC);
-            libc::dup2(logpipe[1], libc::STDOUT_FILENO);
-            libc::dup2(logpipe[1], libc::STDERR_FILENO);
-            libc::close(logpipe[1]);
-
-            File::from_raw_fd(logpipe[0])
-        };
-
-        std::thread::spawn(move || {
-            let tag = CStr::from_bytes_with_nul(b"RustStdoutStderr\0").unwrap();
-            let mut reader = BufReader::new(file);
-            let mut buffer = String::new();
-            loop {
-                buffer.clear();
-                if let Ok(len) = reader.read_line(&mut buffer) {
-                    if len == 0 {
-                        break;
-                    } else if let Ok(msg) = CString::new(buffer.clone()) {
-                        android_log(Level::Info, tag, &msg);
-                    }
-                }
-            }
-        });
+        let _join_log_forwarder = forward_stdio_to_logcat();
 
         log::trace!(
             "Creating: {:p}, saved_state = {:p}, save_state_size = {}",

diff --git a/android-activity/src/util.rs b/android-activity/src/util.rs
@@ -1,7 +1,12 @@
-use log::Level;
+use log::{error, Level};
 use std::{
     ffi::{CStr, CString},
-    os::raw::c_char,
+    fs::File,
+    io::{BufRead as _, BufReader, Result},
+    os::{
+        fd::{FromRawFd as _, RawFd},
+        raw::c_char,
+    },
 };
 
 pub fn try_get_path_from_ptr(path: *const c_char) -> Option<std::path::PathBuf> {
@@ -31,6 +36,44 @@ pub(crate) fn android_log(level: Level, tag: &CStr, msg: &CStr) {
     }
 }
 
+pub(crate) fn forward_stdio_to_logcat() -> std::thread::JoinHandle<Result<()>> {
+    // XXX: make this stdout/stderr redirection an optional / opt-in feature?...
+
+    let file = unsafe {
+        let mut logpipe: [RawFd; 2] = Default::default();
+        libc::pipe2(logpipe.as_mut_ptr(), libc::O_CLOEXEC);
+        libc::dup2(logpipe[1], libc::STDOUT_FILENO);
+        libc::dup2(logpipe[1], libc::STDERR_FILENO);
+        libc::close(logpipe[1]);
+
+        File::from_raw_fd(logpipe[0])
+    };
+
+    std::thread::Builder::new()
+        .name("stdio-to-logcat".to_string())
+        .spawn(move || -> Result<()> {
+            let tag = CStr::from_bytes_with_nul(b"RustStdoutStderr\0").unwrap();
+            let mut reader = BufReader::new(file);
+            let mut buffer = String::new();
+            loop {
+                buffer.clear();
+                let len = match reader.read_line(&mut buffer) {
+                    Ok(len) => len,
+                    Err(e) => {
+                        error!("Logcat forwarder failed to read stdin/stderr: {e:?}");
+                        break Err(e);
+                    }
+                };
+                if len == 0 {
+                    break Ok(());
+                } else if let Ok(msg) = CString::new(buffer.clone()) {
+                    android_log(Level::Info, tag, &msg);
+                }
+            }
+        })
+        .expect("Failed to start stdout/stderr to logcat forwarder thread")
+}
+
 pub(crate) fn log_panic(panic: Box<dyn std::any::Any + Send>) {
     let rust_panic = unsafe { CStr::from_bytes_with_nul_unchecked(b"RustPanic\0") };