Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cli: drop built-in pager_handle when appropriate #4187

Closed
wants to merge 1 commit into from
Closed

cli: drop built-in pager_handle when appropriate #4187

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 24 additions & 5 deletions cli/src/ui.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
use std::io::{IsTerminal as _, Stderr, StderrLock, Stdout, StdoutLock, Write};
use std::process::{Child, ChildStdin, Stdio};
use std::str::FromStr;
use std::sync::{Arc, RwLock};
use std::thread::JoinHandle;
use std::{env, fmt, io, mem};

Expand Down Expand Up @@ -45,7 +46,7 @@ enum UiOutput {

/// A builtin pager
pub struct BuiltinPager {
pager: MinusPager,
pager: Arc<RwLock<Option<MinusPager>>>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd add a comment:

Option<MinusPager> is set to None after the pager quits (e.g. the user quits the pager early).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed! This is one of the things I was hoping to do before taking this out of draft. 🙂 Will attend to this soon.

dynamic_pager_thread: JoinHandle<()>,
}

Expand All @@ -56,8 +57,14 @@ impl Write for &BuiltinPager {
}

fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
let pager = self
.pager
.read()
.unwrap()
.clone()
.ok_or_else(|| io::Error::from(io::ErrorKind::BrokenPipe))?;
let string = std::str::from_utf8(buf).map_err(io::Error::other)?;
self.pager.push_str(string).map_err(io::Error::other)?;
pager.push_str(string).map_err(io::Error::other)?;
Ok(buf.len())
}
}
Expand All @@ -70,6 +77,7 @@ impl Default for BuiltinPager {

impl BuiltinPager {
pub fn finalize(self) {
self.pager.write().unwrap().take();
let dynamic_pager_thread = self.dynamic_pager_thread;
dynamic_pager_thread.join().unwrap();
}
Expand All @@ -81,13 +89,24 @@ impl BuiltinPager {
pager
.set_exit_strategy(minus::ExitStrategy::PagerQuit)
.expect("Able to set the exit strategy");
let pager_handle = pager.clone();
let pager_handle = Arc::new(RwLock::new(Some(pager.clone())));

pager
.add_exit_callback(Box::new({
let pager_handle = pager_handle.clone();
move || {
pager_handle.write().unwrap().take();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to disable late write()s, we don't have to drop pager object. Instead, maybe we can use AtomicBool to be set on pager quit.

BTW, this PR revealed that the current builtin pager is fundamentally broken. Because the backing channel is unbounded, .write() will never stuck and burn CPU time.

Copy link
Contributor

@ilyagr ilyagr Aug 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the backing channel is unbounded, .write() will never stuck and burn CPU time.

(Just curious) What do you mean? Are you talking about the problem this PR solves or about a separate problem? Are you talking about something we do or minus's design?

Copy link
Contributor

@ilyagr ilyagr Aug 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, AtomicBool sounds fine to me as far as this PR goes, but I'll mention that I was considering replacing the Option here with a Result later for #4182 (comment). I am not 100% sure that's the best solution to that problem, but if we went with that approach, it would need preserving more information than just one AtomicBool.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's kinda the same problem under the hood, but user might not notice. Basically, if the pager is minus, jj log -r.. -p will scan all ancestors, materialize diffs, and the whole output will be buffered in memory.

Our expectation is that .write() into the pager will stuck if the pipe is full, but minus doesn't because the underlying channel is unbounded.

Copy link
Contributor

@ilyagr ilyagr Aug 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be symptoms of this? Running out of memory? Something more serious?

Would anything short of redesigning minus help? Though, they might accept a PR to block on writes at some limit and return BrokenPipe after quitting.

I think the pager Sapling used, streampager, was more carefully designed for huge inputs, but it seemed abandoned last I checked.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yuja: It's true that we could use AtomicBool for signaling here, but I see no advantage to doing so, and a few advantages in keeping the current Option container:

  1. If we don't intend to use the pager anymore, this makes it inaccessible by design. It matches our intent; I think it will be far less surprising for those unfamiliar with this code to see this handle being discarded than an AtomicBool as a peer.
  2. Because minus' pager handle is implemented as a sender and receiver channel pair for duplix comms. with the paging thread, dropping pager_handle's contents should free the CLI thread's Receiver's resource (which is very relevant to the above discussion about resource usage with lines that haven't been displayed yet). It's unclear to me what comms. might be queued up in return from the paging thread, but this sidesteps any question of whether they're significant.

Copy link
Contributor

@yuja yuja Aug 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I would say AtomicBool is simpler and that's the advantage.

As far as I understand the implementation, minus::dynamic_paging() doesn't drop their tx end, so dropping our pager instance wouldn't signal the termination. Ideally, all rx ends should be dropped on pager quit, so any further pager.push_str() automatically fails. However, minus API appears not designed with such use case in mind.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the point that AtomicBool avoids the possibility of deadlocks? While it's more primitive, that makes its docs more complicated (https://doc.rust-lang.org/std/sync/atomic/enum.Ordering.html, for example), so I couldn't immediately figure that out.

Like Erich, I liked the idea of making invalid states inexpressible, but I'm not sure about the downsides of RwLock and whether they are worth it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the point that AtomicBool avoids the possibility of deadlocks?

No? I just mean boolean flag is easier to follow than heavy lifting on interior mutability.

BTW, judging from the discussion on Discord, it might be better to just switch to streampager.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If anyone is reading this, here is what seems like a nice book that discusses Atomic types:

https://marabos.nl/atomics/atomics.html#atomic-load-and-store-operations

}
}))
.unwrap();

BuiltinPager {
pager,
pager: pager_handle.clone(),
dynamic_pager_thread: std::thread::spawn(move || {
// This thread handles the actual paging.
minus::dynamic_paging(pager_handle).unwrap();
let res = minus::dynamic_paging(pager);
pager_handle.write().unwrap().take();
res.unwrap();
}),
}
}
Expand Down