Generalize log-forwarding setup and stop thread on IO errors

MarijnS95 · MarijnS95 · commit af341897a2d2 · 2023-11-20T14:15:54.000+01:00
When `read_line()` starts returning `Err` the current `if let Ok`
condition ignores those, likely causing the `loop` to spin indefinitely
while this function keeps returning errors.

Note that we don't currently store the join handle for this thread
anywhere, so won't see the error surface either (just like how the join
handle for the main thread is never checked).  Perhaps we should call
`log::error!()` to make the user aware that their IO logging has
mysteriously terminated.
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") };
 
