From 8ccf3d61a02e8df8c45e70b230d4cf3266195f0b Mon Sep 17 00:00:00 2001 From: KOVACS Tamas Date: Wed, 26 May 2021 00:56:27 +0200 Subject: [PATCH 1/2] fix: use bounded queue between pty and screen Pty reads a command's output and feeds it to Screen using an unbounded queue. However, if the command produces output faster than what `Screen` can render, `Pty` still pushes it on the queue, causing it to grow indefinitely, resulting in high memory usage and latency. This patch fixes this by using a bounded queue between Pty and Screen, so if Screen can't keep up with Pty, the queue will fill up, exerting back pressure on Pty, making it read the command's output only as fast as Screen renders it. The unbounded queue is kept between Screen and producers other than Pty to avoid a deadlock such as this scenario: * pty thread filling up screen queue as soon as screen thread pops something from it * wasm thread is processing a Render instruction, blocking on the screen queue * screen thread is trying to render a plugin pane. It attempts to send a Render insturction to the blocked wasm thread running the plugin. This patch also adds a generous amount of sleeps to the integration tests as having backpressure changes the timing of how instructions are processed. Fixes #525. --- src/tests/integration/close_pane.rs | 6 +++++- src/tests/integration/resize_down.rs | 5 +++++ src/tests/integration/resize_left.rs | 3 +++ src/tests/integration/resize_right.rs | 3 +++ src/tests/integration/resize_up.rs | 5 +++++ zellij-server/src/lib.rs | 9 +++++++-- zellij-server/src/pty.rs | 26 +++++++++++++++----------- 7 files changed, 43 insertions(+), 14 deletions(-) diff --git a/src/tests/integration/close_pane.rs b/src/tests/integration/close_pane.rs index b5ba7f1985..5988151450 100644 --- a/src/tests/integration/close_pane.rs +++ b/src/tests/integration/close_pane.rs @@ -9,7 +9,7 @@ use crate::CliArgs; use crate::tests::utils::commands::{ CLOSE_PANE_IN_PANE_MODE, ESC, MOVE_FOCUS_IN_PANE_MODE, PANE_MODE, QUIT, RESIZE_DOWN_IN_RESIZE_MODE, RESIZE_LEFT_IN_RESIZE_MODE, RESIZE_MODE, RESIZE_UP_IN_RESIZE_MODE, - SPLIT_DOWN_IN_PANE_MODE, SPLIT_RIGHT_IN_PANE_MODE, + SLEEP, SPLIT_DOWN_IN_PANE_MODE, SPLIT_RIGHT_IN_PANE_MODE, }; use zellij_utils::input::config::Config; @@ -404,6 +404,7 @@ pub fn close_pane_with_multiple_panes_above_it_away_from_screen_edges() { &MOVE_FOCUS_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, + &SLEEP, &SPLIT_RIGHT_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, @@ -464,6 +465,7 @@ pub fn close_pane_with_multiple_panes_below_it_away_from_screen_edges() { &PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, + &SLEEP, &SPLIT_RIGHT_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, @@ -530,6 +532,7 @@ pub fn close_pane_with_multiple_panes_to_the_left_away_from_screen_edges() { &MOVE_FOCUS_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, + &SLEEP, &SPLIT_DOWN_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, @@ -592,6 +595,7 @@ pub fn close_pane_with_multiple_panes_to_the_right_away_from_screen_edges() { &PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, + &SLEEP, &SPLIT_DOWN_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, diff --git a/src/tests/integration/resize_down.rs b/src/tests/integration/resize_down.rs index 4ef45a0692..e5d35c2662 100644 --- a/src/tests/integration/resize_down.rs +++ b/src/tests/integration/resize_down.rs @@ -438,6 +438,7 @@ pub fn resize_down_with_panes_above_aligned_left_and_right_with_current_pane() { &SPLIT_DOWN_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, + &SLEEP, &SPLIT_DOWN_IN_PANE_MODE, &RESIZE_MODE, &RESIZE_DOWN_IN_RESIZE_MODE, @@ -491,6 +492,7 @@ pub fn resize_down_with_panes_below_aligned_left_and_right_with_current_pane() { &SPLIT_DOWN_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, + &SLEEP, &SPLIT_DOWN_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, @@ -547,10 +549,12 @@ pub fn resize_down_with_panes_above_aligned_left_and_right_with_panes_to_the_lef &RESIZE_LEFT_IN_RESIZE_MODE, &RESIZE_LEFT_IN_RESIZE_MODE, &PANE_MODE, + &SLEEP, &SPLIT_DOWN_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, + &SLEEP, &SPLIT_DOWN_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, @@ -619,6 +623,7 @@ pub fn resize_down_with_panes_below_aligned_left_and_right_with_to_the_left_and_ &RESIZE_LEFT_IN_RESIZE_MODE, &RESIZE_LEFT_IN_RESIZE_MODE, &PANE_MODE, + &SLEEP, &SPLIT_DOWN_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, diff --git a/src/tests/integration/resize_left.rs b/src/tests/integration/resize_left.rs index 1b82050191..564d5c605d 100644 --- a/src/tests/integration/resize_left.rs +++ b/src/tests/integration/resize_left.rs @@ -416,6 +416,7 @@ pub fn resize_left_with_panes_to_the_left_aligned_top_and_bottom_with_current_pa &SPLIT_RIGHT_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, + &SLEEP, &SPLIT_RIGHT_IN_PANE_MODE, &RESIZE_MODE, &RESIZE_LEFT_IN_RESIZE_MODE, @@ -469,6 +470,7 @@ pub fn resize_left_with_panes_to_the_right_aligned_top_and_bottom_with_current_p &SPLIT_RIGHT_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, + &SLEEP, &SPLIT_RIGHT_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, @@ -529,6 +531,7 @@ pub fn resize_left_with_panes_to_the_left_aligned_top_and_bottom_with_panes_abov &MOVE_FOCUS_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, + &SLEEP, &SPLIT_RIGHT_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, diff --git a/src/tests/integration/resize_right.rs b/src/tests/integration/resize_right.rs index 44601f2cd6..716ed128ca 100644 --- a/src/tests/integration/resize_right.rs +++ b/src/tests/integration/resize_right.rs @@ -416,6 +416,7 @@ pub fn resize_right_with_panes_to_the_left_aligned_top_and_bottom_with_current_p &SPLIT_RIGHT_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, + &SLEEP, &SPLIT_RIGHT_IN_PANE_MODE, &RESIZE_MODE, &RESIZE_RIGHT_IN_RESIZE_MODE, @@ -469,6 +470,7 @@ pub fn resize_right_with_panes_to_the_right_aligned_top_and_bottom_with_current_ &SPLIT_RIGHT_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, + &SLEEP, &SPLIT_RIGHT_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, @@ -529,6 +531,7 @@ pub fn resize_right_with_panes_to_the_left_aligned_top_and_bottom_with_panes_abo &MOVE_FOCUS_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, + &SLEEP, &SPLIT_RIGHT_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, diff --git a/src/tests/integration/resize_up.rs b/src/tests/integration/resize_up.rs index 9aca8d5a26..0097756a8e 100644 --- a/src/tests/integration/resize_up.rs +++ b/src/tests/integration/resize_up.rs @@ -432,6 +432,7 @@ pub fn resize_up_with_panes_above_aligned_left_and_right_with_current_pane() { &SPLIT_DOWN_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, + &SLEEP, &SPLIT_DOWN_IN_PANE_MODE, &RESIZE_MODE, &RESIZE_UP_IN_RESIZE_MODE, @@ -485,6 +486,7 @@ pub fn resize_up_with_panes_below_aligned_left_and_right_with_current_pane() { &SPLIT_DOWN_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, + &SLEEP, &SPLIT_DOWN_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, @@ -541,10 +543,12 @@ pub fn resize_up_with_panes_above_aligned_left_and_right_with_panes_to_the_left_ &RESIZE_LEFT_IN_RESIZE_MODE, &RESIZE_LEFT_IN_RESIZE_MODE, &PANE_MODE, + &SLEEP, &SPLIT_DOWN_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, + &SLEEP, &SPLIT_DOWN_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, @@ -613,6 +617,7 @@ pub fn resize_up_with_panes_below_aligned_left_and_right_with_to_the_left_and_ri &RESIZE_LEFT_IN_RESIZE_MODE, &RESIZE_LEFT_IN_RESIZE_MODE, &PANE_MODE, + &SLEEP, &SPLIT_DOWN_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, &MOVE_FOCUS_IN_PANE_MODE, diff --git a/zellij-server/src/lib.rs b/zellij-server/src/lib.rs index 6d33eddf1b..2f0389ef7a 100644 --- a/zellij-server/src/lib.rs +++ b/zellij-server/src/lib.rs @@ -303,6 +303,11 @@ fn init_session( ) -> SessionMetaData { let (to_screen, screen_receiver): ChannelWithContext = channels::unbounded(); let to_screen = SenderWithContext::new(to_screen); + + let (to_screen_bounded, bounded_screen_receiver): ChannelWithContext = + channels::bounded(50); + let to_screen_bounded = SenderWithContext::new(to_screen_bounded); + let (to_plugin, plugin_receiver): ChannelWithContext = channels::unbounded(); let to_plugin = SenderWithContext::new(to_plugin); let (to_pty, pty_receiver): ChannelWithContext = channels::unbounded(); @@ -334,7 +339,7 @@ fn init_session( let pty = Pty::new( Bus::new( vec![pty_receiver], - Some(&to_screen), + Some(&to_screen_bounded), None, Some(&to_plugin), Some(&to_server), @@ -351,7 +356,7 @@ fn init_session( .name("screen".to_string()) .spawn({ let screen_bus = Bus::new( - vec![screen_receiver], + vec![screen_receiver, bounded_screen_receiver], None, Some(&to_pty), Some(&to_plugin), diff --git a/zellij-server/src/pty.rs b/zellij-server/src/pty.rs index 803c042486..41f9d1a9b7 100644 --- a/zellij-server/src/pty.rs +++ b/zellij-server/src/pty.rs @@ -148,6 +148,12 @@ async fn deadline_read( } } +async fn async_send_to_screen(senders: ThreadSenders, screen_instruction: ScreenInstruction) { + task::spawn_blocking(move || senders.send_to_screen(screen_instruction)) + .await + .unwrap() +} + fn stream_terminal_bytes( pid: RawFd, senders: ThreadSenders, @@ -171,36 +177,34 @@ fn stream_terminal_bytes( match deadline_read(async_reader.as_mut(), render_deadline, &mut buf).await { ReadResult::Ok(0) | ReadResult::Err(_) => break, // EOF or error ReadResult::Timeout => { - let _ = senders.send_to_screen(ScreenInstruction::Render); + async_send_to_screen(senders.clone(), ScreenInstruction::Render).await; // next read does not need a deadline as we just rendered everything render_deadline = None; - - // yield so Screen thread has some time to render before send additional - // PtyBytes. - task::sleep(Duration::from_millis(10)).await; } ReadResult::Ok(n_bytes) => { let bytes = &buf[..n_bytes]; if debug { let _ = debug_to_file(bytes, pid); } - let _ = senders - .send_to_screen(ScreenInstruction::PtyBytes(pid, bytes.to_vec())); + async_send_to_screen( + senders.clone(), + ScreenInstruction::PtyBytes(pid, bytes.to_vec()), + ) + .await; // if we already have a render_deadline we keep it, otherwise we set it // to the duration of `render_pause`. render_deadline.get_or_insert(Instant::now() + render_pause); } } } - let _ = senders.send_to_screen(ScreenInstruction::Render); + async_send_to_screen(senders.clone(), ScreenInstruction::Render).await; #[cfg(not(any(feature = "test", test)))] // this is a little hacky, and is because the tests end the file as soon as // we read everything, rather than hanging until there is new data // a better solution would be to fix the test fakes, but this will do for now - senders - .send_to_screen(ScreenInstruction::ClosePane(PaneId::Terminal(pid))) - .unwrap(); + async_send_to_screen(senders, ScreenInstruction::ClosePane(PaneId::Terminal(pid))) + .await; } }) } From ee4e619b8001c8fe6b188633058c6de199d14081 Mon Sep 17 00:00:00 2001 From: Kunal Mohan Date: Fri, 28 May 2021 20:37:33 +0530 Subject: [PATCH 2/2] fix(tests): Add SLEEP to top_and_quit test --- src/tests/integration/compatibility.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/tests/integration/compatibility.rs b/src/tests/integration/compatibility.rs index 3b28a97032..ec983b0957 100644 --- a/src/tests/integration/compatibility.rs +++ b/src/tests/integration/compatibility.rs @@ -8,7 +8,7 @@ use crate::tests::utils::{get_next_to_last_snapshot, get_output_frame_snapshots} use crate::CliArgs; use zellij_utils::pane_size::PositionAndSize; -use crate::tests::utils::commands::QUIT; +use crate::tests::utils::commands::{QUIT, SLEEP}; use zellij_utils::input::config::Config; /* @@ -607,7 +607,7 @@ pub fn top_and_quit() { }; let fixture_name = "top_and_quit"; let mut fake_input_output = get_fake_os_input(&fake_win_size, fixture_name); - fake_input_output.add_terminal_input(&[&QUIT]); + fake_input_output.add_terminal_input(&[&SLEEP, &QUIT]); start( Box::new(fake_input_output.clone()), CliArgs::default(),