From a9985cf3dcf693e7f22de7b2651b47146d4cf4b4 Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Mon, 16 Oct 2023 18:38:02 +0200 Subject: [PATCH] Generalize log-forwarding setup and stop thread on IO errors 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. --- android-activity/src/game_activity/mod.rs | 44 +++--------------- android-activity/src/native_activity/glue.rs | 35 +-------------- android-activity/src/util.rs | 47 +++++++++++++++++++- 3 files changed, 54 insertions(+), 72 deletions(-) diff --git a/android-activity/src/game_activity/mod.rs b/android-activity/src/game_activity/mod.rs index f9904d1..b23112d 100644 --- a/android-activity/src/game_activity/mod.rs +++ b/android-activity/src/game_activity/mod.rs @@ -1,12 +1,8 @@ #![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::NonNull; use std::sync::Weak; @@ -15,7 +11,7 @@ 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 { 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 { 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 { 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 index 003eaa5..30f4843 100644 --- 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 index 6e04758..8310aca 100644 --- 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 { @@ -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> { + // 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!("Log 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) { let rust_panic = unsafe { CStr::from_bytes_with_nul_unchecked(b"RustPanic\0") };