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.

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

External printer API rework #737

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

IanManske
Copy link
Member

@IanManske IanManske commented Jan 31, 2024

This PR brushes up the external printer feature and is motivated by this nushell PR. Notable changes include:

  • Uses std::sync::mpsc instead of crossbeam-channel, since crossbeam-channel was merged into std::sync::mpsc in Rust 1.67.
  • Users can send/print raw bytes instead of only Strings.
  • Reduced intermediate allocations using Receiver::try_iter.

@IanManske
Copy link
Member Author

IanManske commented Jan 31, 2024

On error, is returning the raw String or Vec<u8> value ok, or should these be wrapped in an PrintError<T>(pub T) type?

There are still some issues with printing. For example, the screen is cleared if the prompt is not at the bottom of the screen and more than one line is printed through the external printer. I think there was a previous, similar issue to this that was fixed, but I guess the fix was not carried over to the external printer?

@stormasm
Copy link
Contributor

stormasm commented Feb 1, 2024

@IanManske would it be possible or do you think it makes sense to have the ability to send the data from the channel to a file or the terminal --- and or have some type of configuration option that says send data to a file or to the terminal...

The reason I am asking this is because I have implemented via a logger the ability to send data to a file using the EnvLogger crate...

main...stormasm:reedline:simplelogcomplete02

This is the general API call for the logger -> https://docs.rs/simplelog/0.12.1/simplelog/struct.WriteLogger.html

If we had the ability to send data to a file via the ExternalPrinter then I would not need to use this logging facility...

And or maybe we should have both types of functionality in Reedline...

Both a logging mechanism and the ExternalPrinter ---- but that seems probably redundant ?

One of the reasons I like sending data to a file instead of the terminal is because it allows me to review what is going on later on as well as have a permanent state of what happened.

I also find sending data to the terminal as "messy" and confusing with so much stuff flying across the screen it gets kind of hard to understand what is going on.

Thanks for thinking about the file option as a potential solution in addition to sending data to the terminal 😄

@IanManske
Copy link
Member Author

@storm, I think it's fine to have both a logging mechanism and the external printer, since the two are meant for two different purposes. The current job of the external printer is to synchronize multiple writers to the terminal and to manipulate newlines to account for the terminal being in raw mode. So, if we are just logging to a file, then I don't think there's much overlap with the external printer. I.e., the external printer with its mpsc channel is probably overkill/not applicable, since logs can just be directly written to a file without needing to go through a channel.

For more context, in nushell's case, I intend that redirecting background job output to a file will be handled through normal redirection (o>, e>, etc.). So, reedline won't be involved at all in this case, since there's no output bound for the terminal.

Anyways, thanks for bringing this up!

One of the reasons I like sending data to a file instead of the terminal is because it allows me to review what is going on later on as well as have a permanent state of what happened.
I also find sending data to the terminal as "messy" and confusing with so much stuff flying across the screen it gets kind of hard to understand what is going on.

Yep, makes sense 😄

@stormasm
Copy link
Contributor

stormasm commented Feb 1, 2024

@IanManske cool ! thanks for the thorough write up... I learned a lot yesterday in our meeting discussing the external printer and again with this conversation.... I will probably have more questions --- this is important work and will certainly add more depth to Reedline and Nushell in general so its great you are taking on this project and diving into the details of how everything works ---- and we are the beneficiaries of the education on the subject 👍

In Rust 1.67, `crossbeam-channel` was merged into `std::sync::mpsc`.
Copy link
Contributor

@stormasm stormasm left a comment

Choose a reason for hiding this comment

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

Thats cool that
crossbeam-channelwas merged intostd::sync::mpsc`
and that you made the update...

Copy link
Member

@sholderbach sholderbach left a comment

Choose a reason for hiding this comment

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

Haven't done adverserial manual testing around the drawing but I like the direction this is going.

Comment on lines +691 to +695

// There could be multiple events queued up!
// pasting text, resizes, blocking this thread (e.g. during debugging)
// We should be able to handle all of them as quickly as possible without causing unnecessary output steps.
while event::poll(Duration::from_millis(POLL_WAIT))? {
Copy link
Member

Choose a reason for hiding this comment

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

There was some subtlety around when we crossterm::event::poll and crossterm::event::read to avoid idling in a hotter than necessary loop #651

So I am not sure if this do ... while to while transformation is correct.

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately, the current loop structure will block on the first read and later external messages will not get printed until the user types which is confusing. I wonder if there's a better solution though...

Copy link
Member

Choose a reason for hiding this comment

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

Damn you asynchronous concurrency....

Not blocking by polling certainly makes reading that channel possible.
Could we shove both kinds of events into one channel and block on that? (that would be the no async just threads everywhere path)

With how we dealt with crossterm and stdout so far I am certainly not screaming: cargo add tokio

Copy link
Member Author

@IanManske IanManske Feb 17, 2024

Choose a reason for hiding this comment

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

Using the channel + threads should work, but I think that means there will have to be 3 threads in total:

  • One to forward messages from event::read
  • One to forward messages from the external printer
  • The main thread, to pull messages off the channel

It is possible to remove the external printer thread if we wrap the external printer sender in a new type. This way, we can allow the external thread to send messages directly to the merged channel (Reciever<ReedlineMessage>), instead of having to move data from the external channel Reciever<Vec<u8>> to the merged channel. E.g.,:

enum ReedlineMessage {
    Event(crossterm::Event),
    External(Vec<u8>),
}

struct ExternalPrinterSender(SyncSender<ReedlineMessage>);

impl ExternalPrinterSender {
    pub fn send(&self, data: Vec<u8>) -> SendError<Vec<u8> {
        self.0.send(ReedlineMessage::External(data)).map_err(...)
    }
}

The potential problem with this is that nu-system, or wherever job controls ends up, would need to take reedline as a dependency.

Copy link
Member

Choose a reason for hiding this comment

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

Crossterm mentions a restriction on the threads their blocking functions can be run from and that it is incompatible with their async oriented EventStream

https://docs.rs/crossterm/latest/crossterm/event/index.html

Maybe we need to bite the Future bullet at some point? (providing an executor would suck for library users, but we may want to unlock things like async prompt or completion updates.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Crossterm mentions a restriction on the threads their blocking functions can be run from

This should be fine, one thread can both poll and read.

Maybe we need to bite the Future bullet at some point?

In the future (ha), it might be necessary. So far, it looks like we can probably work around it?

Copy link
Member Author

@IanManske IanManske Feb 24, 2024

Choose a reason for hiding this comment

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

Two other possible solutions:

  1. Use a higher poll wait time like it was suggested in external_printer does not print until I press some key #757. We could use the higher wait time for the first poll only and then switch back to the current wait time after the first read.

  2. Have only two threads:

    1. the main thread: has the old poll + read loop and prints the prompt, etc.
    2. the external printer thread: takes messages off the channel receiver and prints these messages.

    Then, we synchronize the two threads using a Mutex or something similar to ensure that only one of them prints to the terminal at a time. Compared to the three thread approach before, this has the benefit that it should be easier to isolate external_printer handling, so users that do not use the external_printer feature should not have to pay any extra performance cost.

Copy link
Member

Choose a reason for hiding this comment

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

Not sure how 1) would play out both in terms of external latency and syscall churn. (would need to see it)

  1. sounds more involved. Question would be if the crossterm cursor or terminal size polling operations may collide with streaming output from another thread. But yeah with the appropriate locking this may be workable.

src/external_printer.rs Show resolved Hide resolved
src/external_printer.rs Show resolved Hide resolved
@stormasm
Copy link
Contributor

stormasm commented Feb 18, 2024

The potential problem with this is that nu-system, or wherever job controls ends up, would need to take reedline as a dependency.

That would not work well --- ideally we don't want to have any reedline dependencies in nushell except for where they are currently located which is in nu-cli which is the only place we want it...

and nu-lsp...
And having it in nu-lsp is for the

use reedline::Completer

Reason being we want to be able to allow our development community to be able to swap out nu-cli and replace it with their own cli / line editor...

Or run nushell inside a nana type application where no cli or line editor is needed...

@IanManske
Copy link
Member Author

IanManske commented Feb 27, 2024

Ok, the polling problem is a little more involved than I thought. I'm not sure if either of the thread solutions will work well or at all:

  • For the three thread solution: this will interleave crossterm events and external messages into one channel. The potential problem with this is that an external message source can send a bunch of messages and swamp the channel (e.g., background job suddenly outputs a lot of text). This could prevent/delay us from processing user input which is not the best, since the user input could contain ctrlc's or shell commands to kill (background) jobs. I.e., the user input needs to have priority over external messages.
    (Although, in practice this might not be a problem? If the merged channel has a bounded capacity, it will eventually fill up, and the user input thread should be able to fight for the next send with equal odds.)
  • For the two thread solution: the external printer thread would need to reprint the prompt, since the main thread could be blocked on event::read. But this would require the external printer thread to have access to the &dyn Prompt and &mut self Reedline. Not sure if there's a way to do this well (if at all).

So, this leaves a few options:

  1. We use a continuous poll, and users with the external-printer feature enabled would see some decrease in battery life.
  2. We bite the async bullet.
  3. Maybe there's another possible solution (e.g., we somehow make one of the thread solutions workable).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants