Skip to content

Commit

Permalink
fix: use bounded queue between pty and screen
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
kxt committed May 27, 2021
1 parent 5c1c1a1 commit 63c295d
Show file tree
Hide file tree
Showing 7 changed files with 43 additions and 14 deletions.
6 changes: 5 additions & 1 deletion src/tests/integration/close_pane.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
5 changes: 5 additions & 0 deletions src/tests/integration/resize_down.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
3 changes: 3 additions & 0 deletions src/tests/integration/resize_left.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
3 changes: 3 additions & 0 deletions src/tests/integration/resize_right.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
5 changes: 5 additions & 0 deletions src/tests/integration/resize_up.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
9 changes: 7 additions & 2 deletions zellij-server/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,11 @@ fn init_session(
) -> SessionMetaData {
let (to_screen, screen_receiver): ChannelWithContext<ScreenInstruction> = channels::unbounded();
let to_screen = SenderWithContext::new(to_screen);

let (to_screen_bounded, bounded_screen_receiver): ChannelWithContext<ScreenInstruction> =
channels::bounded(50);
let to_screen_bounded = SenderWithContext::new(to_screen_bounded);

let (to_plugin, plugin_receiver): ChannelWithContext<PluginInstruction> = channels::unbounded();
let to_plugin = SenderWithContext::new(to_plugin);
let (to_pty, pty_receiver): ChannelWithContext<PtyInstruction> = channels::unbounded();
Expand Down Expand Up @@ -337,7 +342,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),
Expand All @@ -354,7 +359,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),
Expand Down
26 changes: 15 additions & 11 deletions zellij-server/src/pty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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;
}
})
}
Expand Down

0 comments on commit 63c295d

Please sign in to comment.