pogmommy/zellij
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

refactor(IO): random fixes (#521)

Browse source 
* refactor(os_input_output): use Pid for child process

* fix(debug): change debug_to_file to write &[u8]

This patch changes debug_to_file to write entire slices in one call, to
avoid the overhead of opening and closing files for each byte written.

* refactor(ServerOsApi): remove unnecessary muts from methods

Co-authored-by: KOVACS Tamas <ktamas@fastmail.fm>
This commit is contained in:
kxt 2021-05-18 17:39:36 +02:00 • committed by GitHub
parent 12de9b1073
commit 68445af63f
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
6 changed files with 48 additions and 49 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

27
src/tests/fakes.rs
View file

@ -8,7 +8,7 @@ use std::path::PathBuf;
use std::sync::{mpsc, Arc, Condvar, Mutex}; use std::sync::{mpsc, Arc, Condvar, Mutex};
use std::time::{Duration, Instant}; use std::time::{Duration, Instant};
use zellij_client::os_input_output::ClientOsApi; use zellij_client::os_input_output::ClientOsApi;
use zellij_server::os_input_output::ServerOsApi; use zellij_server::os_input_output::{Pid, ServerOsApi};
use zellij_tile::data::Palette; use zellij_tile::data::Palette;
use zellij_utils::{ use zellij_utils::{
channels::{ChannelWithContext, SenderType, SenderWithContext}, channels::{ChannelWithContext, SenderType, SenderWithContext},
@ -22,7 +22,7 @@ const MIN_TIME_BETWEEN_SNAPSHOTS: Duration = Duration::from_millis(150);
#[derive(Clone)] #[derive(Clone)]
pub enum IoEvent { pub enum IoEvent {
Kill(RawFd), Kill(Pid),
SetTerminalSizeUsingFd(RawFd, u16, u16), SetTerminalSizeUsingFd(RawFd, u16, u16),
IntoRawMode(RawFd), IntoRawMode(RawFd),
UnsetRawMode(RawFd), UnsetRawMode(RawFd),
@ -127,7 +127,7 @@ impl FakeInputOutput {
} }
self.stdin_commands = Arc::new(Mutex::new(stdin_commands)); self.stdin_commands = Arc::new(Mutex::new(stdin_commands));
} }
pub fn add_terminal(&mut self, fd: RawFd) { pub fn add_terminal(&self, fd: RawFd) {
self.stdin_writes.lock().unwrap().insert(fd, vec![]); self.stdin_writes.lock().unwrap().insert(fd, vec![]);
} }
pub fn add_sigwinch_event(&mut self, new_position_and_size: PositionAndSize) { pub fn add_sigwinch_event(&mut self, new_position_and_size: PositionAndSize) {
@ -225,7 +225,7 @@ impl ClientOsApi for FakeInputOutput {
} }
impl ServerOsApi for FakeInputOutput { impl ServerOsApi for FakeInputOutput {
fn set_terminal_size_using_fd(&mut self, pid: RawFd, cols: u16, rows: u16) { fn set_terminal_size_using_fd(&self, pid: RawFd, cols: u16, rows: u16) {
let terminal_input = self let terminal_input = self
.possible_tty_inputs .possible_tty_inputs
.get(&cols) .get(&cols)
@ -239,12 +239,15 @@ impl ServerOsApi for FakeInputOutput {
.unwrap() .unwrap()
.push(IoEvent::SetTerminalSizeUsingFd(pid, cols, rows)); .push(IoEvent::SetTerminalSizeUsingFd(pid, cols, rows));
} }
fn spawn_terminal(&mut self, _file_to_open: Option<PathBuf>) -> (RawFd, RawFd) { fn spawn_terminal(&self, _file_to_open: Option<PathBuf>) -> (RawFd, Pid) {
let next_terminal_id = self.stdin_writes.lock().unwrap().keys().len() as RawFd + 1; let next_terminal_id = self.stdin_writes.lock().unwrap().keys().len() as RawFd + 1;
self.add_terminal(next_terminal_id); self.add_terminal(next_terminal_id);
(next_terminal_id as i32, next_terminal_id + 1000) // secondary number is arbitrary here (
next_terminal_id as i32,
Pid::from_raw(next_terminal_id + 1000),
) // secondary number is arbitrary here
} }
fn write_to_tty_stdin(&mut self, pid: RawFd, buf: &mut [u8]) -> Result<usize, nix::Error> { fn write_to_tty_stdin(&self, pid: RawFd, buf: &[u8]) -> Result<usize, nix::Error> {
let mut stdin_writes = self.stdin_writes.lock().unwrap(); let mut stdin_writes = self.stdin_writes.lock().unwrap();
let write_buffer = stdin_writes.get_mut(&pid).unwrap(); let write_buffer = stdin_writes.get_mut(&pid).unwrap();
let mut bytes_written = 0; let mut bytes_written = 0;
@ -254,7 +257,7 @@ impl ServerOsApi for FakeInputOutput {
} }
Ok(bytes_written) Ok(bytes_written)
} }
fn read_from_tty_stdout(&mut self, pid: RawFd, buf: &mut [u8]) -> Result<usize, nix::Error> { fn read_from_tty_stdout(&self, pid: RawFd, buf: &mut [u8]) -> Result<usize, nix::Error> {
let mut read_buffers = self.read_buffers.lock().unwrap(); let mut read_buffers = self.read_buffers.lock().unwrap();
let mut bytes_read = 0; let mut bytes_read = 0;
match read_buffers.get_mut(&pid) { match read_buffers.get_mut(&pid) {
@ -271,15 +274,15 @@ impl ServerOsApi for FakeInputOutput {
None => Err(nix::Error::Sys(nix::errno::Errno::EAGAIN)), None => Err(nix::Error::Sys(nix::errno::Errno::EAGAIN)),
} }
} }
fn tcdrain(&mut self, pid: RawFd) -> Result<(), nix::Error> { fn tcdrain(&self, pid: RawFd) -> Result<(), nix::Error> {
self.io_events.lock().unwrap().push(IoEvent::TcDrain(pid)); self.io_events.lock().unwrap().push(IoEvent::TcDrain(pid));
Ok(()) Ok(())
} }
fn box_clone(&self) -> Box<dyn ServerOsApi> { fn box_clone(&self) -> Box<dyn ServerOsApi> {
Box::new((*self).clone()) Box::new((*self).clone())
} }
fn kill(&mut self, fd: RawFd) -> Result<(), nix::Error> { fn kill(&self, pid: Pid) -> Result<(), nix::Error> {
self.io_events.lock().unwrap().push(IoEvent::Kill(fd)); self.io_events.lock().unwrap().push(IoEvent::Kill(pid));
Ok(()) Ok(())
} }
fn recv_from_client(&self) -> (ClientToServerMsg, ErrorContext) { fn recv_from_client(&self) -> (ClientToServerMsg, ErrorContext) {
@ -292,7 +295,7 @@ impl ServerOsApi for FakeInputOutput {
fn send_to_client(&self, msg: ServerToClientMsg) { fn send_to_client(&self, msg: ServerToClientMsg) {
self.send_instructions_to_client.send(msg).unwrap(); self.send_instructions_to_client.send(msg).unwrap();
} }
fn add_client_sender(&mut self) {} fn add_client_sender(&self) {}
fn update_receiver(&mut self, _stream: LocalSocketStream) {} fn update_receiver(&mut self, _stream: LocalSocketStream) {}
fn load_palette(&self) -> Palette { fn load_palette(&self) -> Palette {
default_palette() default_palette()

44
zellij-server/src/os_input_output.rs
View file

@ -4,7 +4,7 @@ use nix::pty::{forkpty, Winsize};
use nix::sys::signal::{kill, Signal}; use nix::sys::signal::{kill, Signal};
use nix::sys::termios; use nix::sys::termios;
use nix::sys::wait::waitpid; use nix::sys::wait::waitpid;
use nix::unistd::{self, ForkResult, Pid}; use nix::unistd::{self, ForkResult};
use signal_hook::consts::*; use signal_hook::consts::*;
use std::env; use std::env;
use std::os::unix::io::RawFd; use std::os::unix::io::RawFd;
@ -18,6 +18,7 @@ use zellij_utils::ipc::{
}; };
use zellij_utils::shared::default_palette; use zellij_utils::shared::default_palette;
pub use nix::unistd::Pid;
pub(crate) fn set_terminal_size_using_fd(fd: RawFd, columns: u16, rows: u16) { pub(crate) fn set_terminal_size_using_fd(fd: RawFd, columns: u16, rows: u16) {
// TODO: do this with the nix ioctl // TODO: do this with the nix ioctl
use libc::ioctl; use libc::ioctl;
@ -76,8 +77,8 @@ fn handle_command_exit(mut child: Child) {
/// set. /// set.
// FIXME this should probably be split into different functions, or at least have less levels // FIXME this should probably be split into different functions, or at least have less levels
// of indentation in some way // of indentation in some way
fn spawn_terminal(file_to_open: Option<PathBuf>, orig_termios: termios::Termios) -> (RawFd, RawFd) { fn spawn_terminal(file_to_open: Option<PathBuf>, orig_termios: termios::Termios) -> (RawFd, Pid) {
let (pid_primary, pid_secondary): (RawFd, RawFd) = { let (pid_primary, pid_secondary): (RawFd, Pid) = {
match forkpty(None, Some(&orig_termios)) { match forkpty(None, Some(&orig_termios)) {
Ok(fork_pty_res) => { Ok(fork_pty_res) => {
let pid_primary = fork_pty_res.master; let pid_primary = fork_pty_res.master;
@ -112,7 +113,7 @@ fn spawn_terminal(file_to_open: Option<PathBuf>, orig_termios: termios::Termios)
} }
}, },
}; };
(pid_primary, pid_secondary.as_raw()) (pid_primary, pid_secondary)
} }
Err(e) => { Err(e) => {
panic!("failed to fork {:?}", e); panic!("failed to fork {:?}", e);
@ -133,20 +134,17 @@ pub struct ServerOsInputOutput {
/// Zellij server requires. /// Zellij server requires.
pub trait ServerOsApi: Send + Sync { pub trait ServerOsApi: Send + Sync {
/// Sets the size of the terminal associated to file descriptor `fd`. /// Sets the size of the terminal associated to file descriptor `fd`.
fn set_terminal_size_using_fd(&mut self, fd: RawFd, cols: u16, rows: u16); fn set_terminal_size_using_fd(&self, fd: RawFd, cols: u16, rows: u16);
/// Spawn a new terminal, with an optional file to open in a terminal program. /// Spawn a new terminal, with an optional file to open in a terminal program.
fn spawn_terminal(&mut self, file_to_open: Option<PathBuf>) -> (RawFd, RawFd); fn spawn_terminal(&self, file_to_open: Option<PathBuf>) -> (RawFd, Pid);
/// Read bytes from the standard output of the virtual terminal referred to by `fd`. /// Read bytes from the standard output of the virtual terminal referred to by `fd`.
fn read_from_tty_stdout(&mut self, fd: RawFd, buf: &mut [u8]) -> Result<usize, nix::Error>; fn read_from_tty_stdout(&self, fd: RawFd, buf: &mut [u8]) -> Result<usize, nix::Error>;
/// Write bytes to the standard input of the virtual terminal referred to by `fd`. /// Write bytes to the standard input of the virtual terminal referred to by `fd`.
fn write_to_tty_stdin(&mut self, fd: RawFd, buf: &mut [u8]) -> Result<usize, nix::Error>; fn write_to_tty_stdin(&self, fd: RawFd, buf: &[u8]) -> Result<usize, nix::Error>;
/// Wait until all output written to the object referred to by `fd` has been transmitted. /// Wait until all output written to the object referred to by `fd` has been transmitted.
fn tcdrain(&mut self, fd: RawFd) -> Result<(), nix::Error>; fn tcdrain(&self, fd: RawFd) -> Result<(), nix::Error>;
/// Terminate the process with process ID `pid`. /// Terminate the process with process ID `pid`.
// FIXME `RawFd` is semantically the wrong type here. It should either be a raw libc::pid_t, fn kill(&self, pid: Pid) -> Result<(), nix::Error>;
// or a nix::unistd::Pid. See `man kill.3`, nix::sys::signal::kill (both take an argument
// called `pid` and of type `pid_t`, and not `fd`)
fn kill(&mut self, pid: RawFd) -> Result<(), nix::Error>;
/// Returns a [`Box`] pointer to this [`ServerOsApi`] struct. /// Returns a [`Box`] pointer to this [`ServerOsApi`] struct.
fn box_clone(&self) -> Box<dyn ServerOsApi>; fn box_clone(&self) -> Box<dyn ServerOsApi>;
/// Receives a message on server-side IPC channel /// Receives a message on server-side IPC channel
@ -154,41 +152,41 @@ pub trait ServerOsApi: Send + Sync {
/// Sends a message to client /// Sends a message to client
fn send_to_client(&self, msg: ServerToClientMsg); fn send_to_client(&self, msg: ServerToClientMsg);
/// Adds a sender to client /// Adds a sender to client
fn add_client_sender(&mut self); fn add_client_sender(&self);
/// Update the receiver socket for the client /// Update the receiver socket for the client
fn update_receiver(&mut self, stream: LocalSocketStream); fn update_receiver(&mut self, stream: LocalSocketStream);
fn load_palette(&self) -> Palette; fn load_palette(&self) -> Palette;
} }
impl ServerOsApi for ServerOsInputOutput { impl ServerOsApi for ServerOsInputOutput {
fn set_terminal_size_using_fd(&mut self, fd: RawFd, cols: u16, rows: u16) { fn set_terminal_size_using_fd(&self, fd: RawFd, cols: u16, rows: u16) {
set_terminal_size_using_fd(fd, cols, rows); set_terminal_size_using_fd(fd, cols, rows);
} }
fn spawn_terminal(&mut self, file_to_open: Option<PathBuf>) -> (RawFd, RawFd) { fn spawn_terminal(&self, file_to_open: Option<PathBuf>) -> (RawFd, Pid) {
let orig_termios = self.orig_termios.lock().unwrap(); let orig_termios = self.orig_termios.lock().unwrap();
spawn_terminal(file_to_open, orig_termios.clone()) spawn_terminal(file_to_open, orig_termios.clone())
} }
fn read_from_tty_stdout(&mut self, fd: RawFd, buf: &mut [u8]) -> Result<usize, nix::Error> { fn read_from_tty_stdout(&self, fd: RawFd, buf: &mut [u8]) -> Result<usize, nix::Error> {
unistd::read(fd, buf) unistd::read(fd, buf)
} }
fn write_to_tty_stdin(&mut self, fd: RawFd, buf: &mut [u8]) -> Result<usize, nix::Error> { fn write_to_tty_stdin(&self, fd: RawFd, buf: &[u8]) -> Result<usize, nix::Error> {
unistd::write(fd, buf) unistd::write(fd, buf)
} }
fn tcdrain(&mut self, fd: RawFd) -> Result<(), nix::Error> { fn tcdrain(&self, fd: RawFd) -> Result<(), nix::Error> {
termios::tcdrain(fd) termios::tcdrain(fd)
} }
fn box_clone(&self) -> Box<dyn ServerOsApi> { fn box_clone(&self) -> Box<dyn ServerOsApi> {
Box::new((*self).clone()) Box::new((*self).clone())
} }
fn kill(&mut self, pid: RawFd) -> Result<(), nix::Error> { fn kill(&self, pid: Pid) -> Result<(), nix::Error> {
// TODO: // TODO:
// Ideally, we should be using SIGINT rather than SIGKILL here, but there are cases in which // Ideally, we should be using SIGINT rather than SIGKILL here, but there are cases in which
// the terminal we're trying to kill hangs on SIGINT and so all the app gets stuck // the terminal we're trying to kill hangs on SIGINT and so all the app gets stuck
// that's why we're sending SIGKILL here // that's why we're sending SIGKILL here
// A better solution would be to send SIGINT here and not wait for it, and then have // A better solution would be to send SIGINT here and not wait for it, and then have
// a background thread do the waitpid stuff and send SIGKILL if the process is stuck // a background thread do the waitpid stuff and send SIGKILL if the process is stuck
kill(Pid::from_raw(pid), Some(Signal::SIGKILL)).unwrap(); kill(pid, Some(Signal::SIGKILL)).unwrap();
waitpid(Pid::from_raw(pid), None).unwrap(); waitpid(pid, None).unwrap();
Ok(()) Ok(())
} }
fn recv_from_client(&self) -> (ClientToServerMsg, ErrorContext) { fn recv_from_client(&self) -> (ClientToServerMsg, ErrorContext) {
@ -207,7 +205,7 @@ impl ServerOsApi for ServerOsInputOutput {
.unwrap() .unwrap()
.send(msg); .send(msg);
} }
fn add_client_sender(&mut self) { fn add_client_sender(&self) {
assert!(self.send_instructions_to_client.lock().unwrap().is_none()); assert!(self.send_instructions_to_client.lock().unwrap().is_none());
let sender = self let sender = self
.receive_instructions_from_client .receive_instructions_from_client

14
zellij-server/src/pty.rs
View file

@ -8,7 +8,7 @@ use std::pin::*;
use std::time::{Duration, Instant}; use std::time::{Duration, Instant};
use crate::{ use crate::{
os_input_output::ServerOsApi, os_input_output::{Pid, ServerOsApi},
panes::PaneId, panes::PaneId,
screen::ScreenInstruction, screen::ScreenInstruction,
thread_bus::{Bus, ThreadSenders}, thread_bus::{Bus, ThreadSenders},
@ -37,7 +37,7 @@ impl ReadFromPid {
impl Stream for ReadFromPid { impl Stream for ReadFromPid {
type Item = Vec<u8>; type Item = Vec<u8>;
fn poll_next(mut self: Pin<&mut Self>, _cx: &mut Context<'_>) -> Poll<Option<Self::Item>> { fn poll_next(self: Pin<&mut Self>, _cx: &mut Context<'_>) -> Poll<Option<Self::Item>> {
let mut read_buffer = [0; 65535]; let mut read_buffer = [0; 65535];
let pid = self.pid; let pid = self.pid;
let read_result = &self.os_input.read_from_tty_stdout(pid, &mut read_buffer); let read_result = &self.os_input.read_from_tty_stdout(pid, &mut read_buffer);
@ -97,7 +97,7 @@ impl From<&PtyInstruction> for PtyContext {
pub(crate) struct Pty { pub(crate) struct Pty {
pub bus: Bus<PtyInstruction>, pub bus: Bus<PtyInstruction>,
pub id_to_child_pid: HashMap<RawFd, RawFd>, pub id_to_child_pid: HashMap<RawFd, Pid>,
debug_to_file: bool, debug_to_file: bool,
task_handles: HashMap<RawFd, JoinHandle<()>>, task_handles: HashMap<RawFd, JoinHandle<()>>,
} }
@ -177,9 +177,7 @@ fn stream_terminal_bytes(
while let Some(bytes) = terminal_bytes.next().await { while let Some(bytes) = terminal_bytes.next().await {
let bytes_is_empty = bytes.is_empty(); let bytes_is_empty = bytes.is_empty();
if debug { if debug {
for byte in bytes.iter() { debug_to_file(&bytes, pid).unwrap();
debug_to_file(*byte, pid).unwrap();
}
} }
if !bytes_is_empty { if !bytes_is_empty {
let _ = senders.send_to_screen(ScreenInstruction::PtyBytes(pid, bytes)); let _ = senders.send_to_screen(ScreenInstruction::PtyBytes(pid, bytes));
@ -235,7 +233,7 @@ impl Pty {
} }
} }
pub fn spawn_terminal(&mut self, file_to_open: Option<PathBuf>) -> RawFd { pub fn spawn_terminal(&mut self, file_to_open: Option<PathBuf>) -> RawFd {
let (pid_primary, pid_secondary): (RawFd, RawFd) = self let (pid_primary, pid_secondary): (RawFd, Pid) = self
.bus .bus
.os_input .os_input
.as_mut() .as_mut()
@ -255,7 +253,7 @@ impl Pty {
let total_panes = layout.total_terminal_panes(); let total_panes = layout.total_terminal_panes();
let mut new_pane_pids = vec![]; let mut new_pane_pids = vec![];
for _ in 0..total_panes { for _ in 0..total_panes {
let (pid_primary, pid_secondary): (RawFd, RawFd) = let (pid_primary, pid_secondary): (RawFd, Pid) =
self.bus.os_input.as_mut().unwrap().spawn_terminal(None); self.bus.os_input.as_mut().unwrap().spawn_terminal(None);
self.id_to_child_pid.insert(pid_primary, pid_secondary); self.id_to_child_pid.insert(pid_primary, pid_secondary);
new_pane_pids.push(pid_primary); new_pane_pids.push(pid_primary);

2
zellij-server/src/route.rs
View file

@ -189,7 +189,7 @@ fn route_action(action: Action, session: &SessionMetaData, os_input: &dyn Server
pub(crate) fn route_thread_main( pub(crate) fn route_thread_main(
sessions: Arc<RwLock<Option<SessionMetaData>>>, sessions: Arc<RwLock<Option<SessionMetaData>>>,
mut os_input: Box<dyn ServerOsApi>, os_input: Box<dyn ServerOsApi>,
to_server: SenderWithContext<ServerInstruction>, to_server: SenderWithContext<ServerInstruction>,
) { ) {
loop { loop {

6
zellij-server/src/tab.rs
View file

@ -232,7 +232,7 @@ impl Tab {
position: usize, position: usize,
name: String, name: String,
full_screen_ws: &PositionAndSize, full_screen_ws: &PositionAndSize,
mut os_api: Box<dyn ServerOsApi>, os_api: Box<dyn ServerOsApi>,
senders: ThreadSenders, senders: ThreadSenders,
max_panes: Option<usize>, max_panes: Option<usize>,
pane_id: Option<PaneId>, pane_id: Option<PaneId>,
@ -624,9 +624,9 @@ impl Tab {
match pane_id { match pane_id {
PaneId::Terminal(active_terminal_id) => { PaneId::Terminal(active_terminal_id) => {
let active_terminal = self.panes.get(&pane_id).unwrap(); let active_terminal = self.panes.get(&pane_id).unwrap();
let mut adjusted_input = active_terminal.adjust_input_to_terminal(input_bytes); let adjusted_input = active_terminal.adjust_input_to_terminal(input_bytes);
self.os_api self.os_api
.write_to_tty_stdin(active_terminal_id, &mut adjusted_input) .write_to_tty_stdin(active_terminal_id, &adjusted_input)
.expect("failed to write to terminal"); .expect("failed to write to terminal");
self.os_api self.os_api
.tcdrain(active_terminal_id) .tcdrain(active_terminal_id)

4
zellij-utils/src/logging.rs
View file

@ -71,7 +71,7 @@ pub fn _delete_log_dir() -> io::Result<()> {
} }
} }
pub fn debug_to_file(message: u8, pid: RawFd) -> io::Result<()> { pub fn debug_to_file(message: &[u8], pid: RawFd) -> io::Result<()> {
let mut path = PathBuf::new(); let mut path = PathBuf::new();
path.push(&*ZELLIJ_TMP_LOG_DIR); path.push(&*ZELLIJ_TMP_LOG_DIR);
path.push(format!("zellij-{}.log", pid.to_string())); path.push(format!("zellij-{}.log", pid.to_string()));
@ -81,5 +81,5 @@ pub fn debug_to_file(message: u8, pid: RawFd) -> io::Result<()> {
.create(true) .create(true)
.open(&path)?; .open(&path)?;
set_permissions(&path)?; set_permissions(&path)?;
file.write_all(&[message]) file.write_all(message)
} }