External printer API rework

[IanManske]

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]

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]

@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]

@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]

@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 👍

[stormasm]

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

[sholderbach]

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

[sholderbach]

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.

[IanManske]

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...

[sholderbach]

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

[IanManske]

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.

[sholderbach]

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.)

[IanManske]

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?

[IanManske]

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.

[sholderbach]

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

2. 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.

[stormasm]

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]

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).