Skip to content

Commit

Permalink
errors: Remove log::error in server (zellij-org#1881)
Browse files Browse the repository at this point in the history 
* server/wasm_vm: Replace `log::error!`

with better error logging by means of `non_fatal`. This preserves the
original error and allows adding context information on top. Also makes
error formatting more uniform across the application.

* server/tab: Replace `log::error!`

with better error logging by means of `non_fatal`. This preserves the
original error and allows adding context information on top. Also makes
error formatting more uniform across the application.

* server/route: Replace `log::error!`

and propagate the error to the caller instead.

* server/pty: Replace `log::error!`

with better error logging by means of `non_fatal`. This preserves the
original error and allows adding context information on top. Also makes
error formatting more uniform across the application.

Also add per-instruction error context to make it clear what we tried to
accomplish when an error occured.

* server/panes/tiled_panes: Merge dependencies

and sort them into a better order.

* server/panes/tiled_panes: Replace `log::error!`

with better error logging by means of `non_fatal`. This preserves the
original error and allows adding context information on top. Also makes
error formatting more uniform across the application.

* server/os_input_output: Merge depndencies

and sort them into a better order.

* server/logging_pipe: Replace `log::error!`

with better error logging by means of `non_fatal`. This preserves the
original error and allows adding context information on top. Also makes
error formatting more uniform across the application.

* server/os_io: Remove uses of `log::error`

* changelog: Add PR zellij-org#1881

* server/os_io: Gracefully handle failing resize

for terminals IDs that don't exist, instead of propagating the error to
the user.

* server/lib: Remove leftover log message

* server/pty: Log error cause

rather than providing a hard-coded error reason which is plain wrong in
this context.

* server/screen: Remove calls to `log::error!`

and change `get_active_tab(_mut)?` to return a `Result` instead of an
`Option`. This already makes many places in the code obsolete where
previously "failed to get active tab..." was logged manually.

Rather than logging, use the `anyhow::Error`s we have, along with all
their context information, and log these instead.
  • Loading branch information
@har7an
har7an authored Nov 8, 2022
1 parent 39c8d97 commit 4531427
Show file tree
Hide file tree
Showing 10 changed files with 343 additions and 253 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
* fix: allow cli actions to be run outside of a tty environment (https://github.com/zellij-org/zellij/pull/1905)
* Terminal compatibility: send focus in/out events to terminal panes (https://github.com/zellij-org/zellij/pull/1908)
* fix: various bugs with no-frames and floating panes (https://github.com/zellij-org/zellij/pull/1909)
* debugging: Improve error logging in server (https://github.com/zellij-org/zellij/pull/1881)

## [0.32.0] - 2022-10-25

Expand Down
5 changes: 0 additions & 5 deletions zellij-server/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -573,11 +573,6 @@ pub fn start_server(mut os_input: Box<dyn ServerOsApi>, socket_path: PathBuf) {
},
ServerInstruction::ActiveClients(client_id) => {
let client_ids = session_state.read().unwrap().client_ids();
log::error!(
"Sending client_ids {:?} to client {}",
client_ids,
client_id
);
send_to_client!(
client_id,
os_input,
Expand Down
37 changes: 20 additions & 17 deletions zellij-server/src/logging_pipe.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use std::{

use log::{debug, error};
use wasmer_wasi::{WasiFile, WasiFsError};
use zellij_utils::serde;
use zellij_utils::{errors::prelude::*, serde};

use chrono::prelude::*;
use serde::{Deserialize, Serialize};
Expand Down Expand Up @@ -75,27 +75,30 @@ impl Write for LoggingPipe {
fn flush(&mut self) -> std::io::Result<()> {
self.buffer.make_contiguous();

if let Ok(converted_buffer) = std::str::from_utf8(self.buffer.as_slices().0) {
if converted_buffer.contains('\n') {
let mut consumed_bytes = 0;
let mut split_converted_buffer = converted_buffer.split('\n').peekable();

while let Some(msg) = split_converted_buffer.next() {
if split_converted_buffer.peek().is_none() {
// Log last chunk iff the last char is endline. Otherwise do not do it.
if converted_buffer.ends_with('\n') && !msg.is_empty() {
match std::str::from_utf8(self.buffer.as_slices().0) {
Ok(converted_buffer) => {
if converted_buffer.contains('\n') {
let mut consumed_bytes = 0;
let mut split_converted_buffer = converted_buffer.split('\n').peekable();

while let Some(msg) = split_converted_buffer.next() {
if split_converted_buffer.peek().is_none() {
// Log last chunk iff the last char is endline. Otherwise do not do it.
if converted_buffer.ends_with('\n') && !msg.is_empty() {
self.log_message(msg);
consumed_bytes += msg.len() + 1;
}
} else {
self.log_message(msg);
consumed_bytes += msg.len() + 1;
}
} else {
self.log_message(msg);
consumed_bytes += msg.len() + 1;
}
drop(self.buffer.drain(..consumed_bytes));
}
drop(self.buffer.drain(..consumed_bytes));
}
} else {
error!("Buffer conversion didn't work. This is unexpected");
},
Err(e) => Err::<(), _>(e)
.context("failed to flush logging pipe buffer")
.non_fatal(),
}

Ok(())
Expand Down
82 changes: 43 additions & 39 deletions zellij-server/src/os_input_output.rs
View file
Original file line number Diff line number Diff line change
@@ -1,45 +1,44 @@
use std::collections::{BTreeMap, HashMap, HashSet};
use std::{fs::File, io::Write};
use crate::{panes::PaneId, ClientId};

use crate::panes::PaneId;
use zellij_utils::tempfile::tempfile;

use std::env;
use std::os::unix::io::RawFd;
use std::os::unix::process::CommandExt;
use std::path::PathBuf;
use std::process::{Child, Command};
use std::sync::{Arc, Mutex};

use zellij_utils::errors::prelude::*;
use zellij_utils::{async_std, interprocess, libc, nix, signal_hook};

use async_std::fs::File as AsyncFile;
use async_std::os::unix::io::FromRawFd;
use async_std::{fs::File as AsyncFile, io::ReadExt, os::unix::io::FromRawFd};
use interprocess::local_socket::LocalSocketStream;

use sysinfo::{ProcessExt, ProcessRefreshKind, System, SystemExt};

use nix::pty::{openpty, OpenptyResult, Winsize};
use nix::sys::signal::{kill, Signal};
use nix::sys::termios;

use nix::unistd;
use nix::{
pty::{openpty, OpenptyResult, Winsize},
sys::{
signal::{kill, Signal},
termios,
},
unistd,
};
use signal_hook::consts::*;
use sysinfo::{ProcessExt, ProcessRefreshKind, System, SystemExt};
use zellij_utils::{
async_std,
data::Palette,
errors::prelude::*,
input::command::{RunCommand, TerminalAction},
interprocess,
ipc::{ClientToServerMsg, IpcReceiverWithContext, IpcSenderWithContext, ServerToClientMsg},
libc, nix,
shared::default_palette,
signal_hook,
tempfile::tempfile,
};

use async_std::io::ReadExt;
pub use async_trait::async_trait;
use std::{
collections::{BTreeMap, HashMap, HashSet},
env,
fs::File,
io::Write,
os::unix::{io::RawFd, process::CommandExt},
path::PathBuf,
process::{Child, Command},
sync::{Arc, Mutex},
};

pub use async_trait::async_trait;
pub use nix::unistd::Pid;

use crate::ClientId;

fn set_terminal_size_using_fd(fd: RawFd, columns: u16, rows: u16) {
// TODO: do this with the nix ioctl
use libc::ioctl;
Expand Down Expand Up @@ -164,10 +163,11 @@ fn handle_openpty(
command.current_dir(current_dir);
} else {
// TODO: propagate this to the user
log::error!(
"Failed to set CWD for new pane. {} does not exist or is not a folder",
return Err(anyhow!(
"Failed to set CWD for new pane. '{}' does not exist or is not a folder",
current_dir.display()
);
))
.context("failed to open PTY");
}
}
command
Expand Down Expand Up @@ -417,16 +417,18 @@ pub trait ServerOsApi: Send + Sync {

impl ServerOsApi for ServerOsInputOutput {
fn set_terminal_size_using_terminal_id(&self, id: u32, cols: u16, rows: u16) -> Result<()> {
let err_context = || {
format!(
"failed to set terminal id {} to size ({}, {})",
id, rows, cols
)
};

match self
.terminal_id_to_raw_fd
.lock()
.to_anyhow()
.with_context(|| {
format!(
"failed to set terminal id {} to size ({}, {})",
id, rows, cols
)
})?
.with_context(err_context)?
.get(&id)
{
Some(Some(fd)) => {
Expand All @@ -435,7 +437,9 @@ impl ServerOsApi for ServerOsInputOutput {
}
},
_ => {
log::error!("Failed to find terminal fd for id: {id}, so cannot resize terminal");
Err::<(), _>(anyhow!("failed to find terminal fd for id {id}"))
.with_context(err_context)
.non_fatal();
},
}
Ok(())
Expand Down
56 changes: 32 additions & 24 deletions zellij-server/src/panes/tiled_panes/mod.rs
View file
Original file line number Diff line number Diff line change
@@ -1,29 +1,29 @@
mod pane_resizer;
mod tiled_pane_grid;

use crate::tab::{Pane, MIN_TERMINAL_HEIGHT, MIN_TERMINAL_WIDTH};
use tiled_pane_grid::{split, TiledPaneGrid};

use crate::{
os_input_output::ServerOsApi,
output::Output,
panes::{ActivePanes, PaneId},
ui::boundaries::Boundaries,
ui::pane_contents_and_ui::PaneContentsAndUi,
tab::{Pane, MIN_TERMINAL_HEIGHT, MIN_TERMINAL_WIDTH},
ui::{boundaries::Boundaries, pane_contents_and_ui::PaneContentsAndUi},
ClientId,
};
use std::cell::RefCell;
use std::collections::{BTreeMap, HashMap, HashSet};
use std::rc::Rc;
use std::time::Instant;
use zellij_utils::errors::prelude::*;
use tiled_pane_grid::{split, TiledPaneGrid};
use zellij_utils::{
data::{ModeInfo, Style},
input::command::RunCommand,
input::layout::SplitDirection,
errors::prelude::*,
input::{command::RunCommand, layout::SplitDirection},
pane_size::{Offset, PaneGeom, Size, SizeInPixels, Viewport},
};

use std::{
cell::RefCell,
collections::{BTreeMap, HashMap, HashSet},
rc::Rc,
time::Instant,
};

macro_rules! resize_pty {
($pane:expr, $os_input:expr) => {
if let PaneId::Terminal(ref pid) = $pane.pid() {
Expand Down Expand Up @@ -226,17 +226,18 @@ impl TiledPanes {
*self.display_area.borrow(),
*self.viewport.borrow(),
);
let result = match direction {
match direction {
SplitDirection::Horizontal => {
pane_grid.layout(direction, (*self.display_area.borrow()).cols)
},
SplitDirection::Vertical => {
pane_grid.layout(direction, (*self.display_area.borrow()).rows)
},
};
if let Err(e) = &result {
log::error!("{:?} relayout of the tab failed: {}", direction, e);
}
.or_else(|e| Err(anyError::msg(e)))
.with_context(|| format!("{:?} relayout of tab failed", direction))
.non_fatal();

self.set_pane_frames(self.draw_pane_frames);
}
pub fn set_pane_frames(&mut self, draw_pane_frames: bool) {
Expand Down Expand Up @@ -492,16 +493,23 @@ impl TiledPanes {
display_area.cols = cols;
},
Err(e) => {
log::error!("Failed to horizontally resize the tab: {:?}", e);
Err::<(), _>(anyError::msg(e))
.context("failed to resize tab horizontally")
.non_fatal();
},
};
match pane_grid.layout(SplitDirection::Vertical, rows) {
Ok(_) => {
let row_difference = rows as isize - display_area.rows as isize;
viewport.rows = (viewport.rows as isize + row_difference) as usize;
display_area.rows = rows;
},
Err(e) => {
Err::<(), _>(anyError::msg(e))
.context("failed to resize tab vertically")
.non_fatal();
},
};
if pane_grid.layout(SplitDirection::Vertical, rows).is_ok() {
let row_difference = rows as isize - display_area.rows as isize;
viewport.rows = (viewport.rows as isize + row_difference) as usize;
display_area.rows = rows;
} else {
log::error!("Failed to vertically resize the tab!!!");
}
}
self.set_pane_frames(self.draw_pane_frames);
}
Expand Down
Loading

0 comments on commit 4531427

Please sign in to comment.