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

Prompt is printed before command output #24

Closed
TatriX opened this issue Aug 13, 2020 · 6 comments
Closed

Prompt is printed before command output #24

TatriX opened this issue Aug 13, 2020 · 6 comments

Comments

@TatriX
Copy link
Contributor

TatriX commented Aug 13, 2020

Welcome to Crush
Type "help" for... help.
crush# ls
crush# file
.git .gitignore Cargo.lock Cargo.toml LICENSE README.md build.rs docs example_data ideas ordered_map signature src target tests todo

As you can see prompt after running ls is printed before command output.
After reading the code I found that this happens because command output is send to a printer thread, i.e. readline loop doesn't wait for command to finish.

@liljencrantz
Copy link
Owner

Hi. So this is a problem that I haven't quite fond the right solution to. Commands that send streams of data exit as soon as they have sent all their output, even if the next pipeline step (or the pretty printer) hasn't processed them. In general, I think this is the way it should work.

As you point out, printing output in Crush is done by only one thread to make sure that output is still nicely formatted even if multiple background jobs are printing data to the screen at the same time. I still believe this is a good design, but it does lead to the problem you describe.

I guess the Printer struct would need a method that would wait until there is no output pending to be printed. The underlying channel::Sender has a is_empty method, but no wait_until_empty.

@TatriX
Copy link
Contributor Author

TatriX commented Aug 14, 2020

still nicely formatted even if multiple background jobs are printing data to the screen at the same time

Note that println! uses stdout which is protected by a mutex. So it's safe to print from multiple threads. And you may want to lock it in the printer thread and use that handle for performance reasons.

@liljencrantz
Copy link
Owner

Didn't know that, thanks for the tip!

Down the line, I'd like to make multiplexing two streams fully readable, sort of like if you do tail -f *.log, at which point it's not enough to lock by row, you have to take the lock for the duration of printing a "segment" of a stream. I'd say that the whole pretty-printer is rather naive right now.

To be honest, making an amazing pretty-printer doesn't sound like the hardest part of writing a shell with the largest number of pitfalls, so I'm sort of punting that until later and trying to see if I can flesh out the trickiest bits of the design.

@TatriX
Copy link
Contributor Author

TatriX commented Aug 18, 2020

Right, any fix would do for now.

@liljencrantz
Copy link
Owner

I believe 0d4ac9e should fix the issue. At least, I can no longer reproduce the problem.

Let me know if you're still seeing the problem.

@TatriX
Copy link
Contributor Author

TatriX commented Sep 21, 2020

Works like a charm, thanks!

@TatriX TatriX closed this as completed Sep 21, 2020
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

No branches or pull requests

2 participants