Skip to content

Commit

Permalink
Generalize log-forwarding setup and stop thread on IO errors
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
MarijnS95 committed Nov 20, 2023
1 parent 98aef99 commit a9985cf
Show file tree
Hide file tree
Showing 3 changed files with 54 additions and 72 deletions.
44 changes: 7 additions & 37 deletions android-activity/src/game_activity/mod.rs
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -15,7 +11,7 @@ use std::time::Duration;
use std::{ptr, thread};

Check failure on line 11 in android-activity/src/game_activity/mod.rs

View workflow job for this annotation

GitHub Actions / build (1.68.0)

unused import: `thread`

Check failure on line 11 in android-activity/src/game_activity/mod.rs

View workflow job for this annotation

GitHub Actions / build (stable)

unused import: `thread`

use libc::c_void;
use log::{error, trace, Level};
use log::{error, trace};

use jni_sys::*;

Expand All @@ -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;
Expand Down Expand Up @@ -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)
}
}
}
Expand Down Expand Up @@ -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;
Expand Down
35 changes: 2 additions & 33 deletions android-activity/src/native_activity/glue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};

Expand Down Expand Up @@ -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 = {}",
Expand Down
47 changes: 45 additions & 2 deletions android-activity/src/util.rs
Original file line number Diff line number Diff line change
@@ -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> {
Expand Down Expand Up @@ -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!("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<dyn std::any::Any + Send>) {
let rust_panic = unsafe { CStr::from_bytes_with_nul_unchecked(b"RustPanic\0") };

Expand Down

0 comments on commit a9985cf

Please sign in to comment.