withoutboats / notty

A new kind of terminal
GNU Affero General Public License v3.0
2.3k stars 41 forks source link

#30 Fix Ctrl-D termination behavior. #35

Closed waynr closed 8 years ago

waynr commented 8 years ago

Specifically set up a GTK main timeout function to periodically check an Arc value that indicates whether a read on the pty in the listening thread fails--this failure is taken to mean that the process on the other side of the pseudoterminal has exited.

Also improve error handling in CommandApplicator. Also decrease timeout for CommandApplicatior::apply() calls to improve responsiveness to keystrokes (notice difference between 125 and 25 milliseconds when holding down a character in scaffolding)

withoutboats commented 8 years ago

I think scaffolding must be behaving very differently on our systems; for me, this change is a regression. Can you provide a detailed description of the behavior of scaffolding on your system, both on master and on this branch?

On my system (Mac OS X Yosemite), on the master branch, when I enter ctrl+D, the terminal does not immediately exit. When I then enter another character, the terminal exits with a positive error code. On this branch, when I enter ctrl+D, the terminal does not immediately exist, and when I enter another character, I hit the panic that you reintroduced at scaffolding/src/commands.rs:34. When I revert this change to how failure in commands is handled, I get the same behavior I had before.

withoutboats commented 8 years ago

I don't understand in the abstract how this change could improve behavior. Before this commit, when an error is reached, the process exits. After this commit, when an error is reached, a boolean is set. Another thread is periodically polling that boolean and when it is set, the process exits. As far as I can tell this just introduces some indirection and the overhead of another thread polling a boolean, how is this better than exiting immediately when the error is reached?

The issue, on my system at least, is that output blocks when the tty is closed, instead of returning an EOF error. I'm not sure whether this is caused by the behavior of the tty subsystem or the implementation of the std::io::Chars iterator, but it is a problem.

waynr commented 8 years ago

Before this commit, when an error is reached, the process exits.

Not true.

waynr commented 8 years ago

The issue, on my system at least, is that output blocks when the tty is closed,

It is not clear what you mean by "output blocks when the tty is closed"

instead of returning an EOF error. I'm not sure whether this is caused by the behavior of the tty subsystem or the implementation of the std::io::Chars iterator, but it is a problem.

What is an "EOF error"?

waynr commented 8 years ago

On my system (Mac OS X Yosemite), on the master branch, when I enter ctrl+D, the terminal does not immediately exit. When I then enter another character, the terminal exits with a positive error code.

It sounds like after sending the EOF character you don't hit an IO error until scaffolding attempts to write to the pty file descriptor whereas I reach it as soon as scaffolding attempts to read from the pty file descriptor.

As far as I can tell this just introduces some indirection and the overhead of another thread polling a boolean, how is this better than exiting immediately when the error is reached?

There are no new threads in this branch. gtk::glib::timeout_add() actually registers a function that gets called on timed intervals within the GTK main thread. I am open to any other ideas anyone has to properly call gtk::main_quit() within the GTK main thread once there is some indication that the pseudoterminal is closed, this is the only way I could come up with.

withoutboats commented 8 years ago

Thanks for clarifying the situation with gtk::main_quit(). I understand the issue this PR is trying to solve now.

I've also now been able to replicate it. On Mac OS X (and probably the BSDs), when the shell exits, reading from the tty does not raise EIO, but instead returns a result which causes the Output iterator to return None, exiting the for loop and ending the thread (and never calling exit_on_io_error from that thread). The solution to this is to set the atomic boolean both when an error is reached and when the output parsing loop ends.

However, the issue remains that the program should exit successfully when a command application fails. Commands can only fail if the tty has been closed, which is not a bug but a normal part of program execution. Its possible for the program to panic if a user presses a key directly after exiting the shell, and before the exiting callback has fired.

So there are two more changes that would cause everything to behave correctly:

waynr commented 8 years ago

The boolean setting code needs to be called both when output returns an error and when the output for loop exits.

Ah, that makes sense, I'll make this change.

The exiting function needs to be called both from the boolean checking callback, and from the command applicator.

This is what I plan to work on today--as suggested in one of my comments late last night, I'd like change the return type of the CommandApplicator::apply() method to return a Result that can be dealt "closer" to the UI element that registers it as a callback. This is in anticipation of needing to deal with multiple terminals--we won't be able to call gtk::main_quit() when there might still be open psuedoterminals.

waynr commented 8 years ago

@withoutboats yo, let me know if this latest update works for you.

withoutboats commented 8 years ago

This works on my system and has been merged.