peterh / liner

Pure Go line editor with history, inspired by linenoise
MIT License
1.04k stars 132 forks source link

Return an error when the terminal reports zero columns and is refreshed #87

Closed jsternberg closed 7 years ago

jsternberg commented 7 years ago

When used within a call to docker exec -it where the original container does not have a TTY allocated to it (such as in docker/docker#8755), the number of columns read from the ioctl() call will be zero, but it will not return an error. If you call Prompt() or PasswordPrompt() after this, the prompt will panic when it tries to divide a number by the number of columns (which is zero).

This change detects when the number of columns returned is zero and returns ErrPromptUnsupported from the PromptXXX methods to avoid a panic and so the application is able to deal with the problem more easily.

jsternberg commented 7 years ago

If there's a better error message for this, please tell me. I wasn't sure what to name the error or how to document it. I also wasn't sure how exactly to test it because of the weird way that it happens.

We run into this a decent amount from people trying to use the influx CLI from within Docker like in influxdata/influxdb#7995.

peterh commented 7 years ago

In general, I prefer to fall back to PromptUnsupported when there is nonsense (such as a terminal zero columns wide). Would that work for your use case, or is there really no user interaction possible when Docker is in that state?

peterh commented 7 years ago

See also #75

jsternberg commented 7 years ago

I think ErrPromptUnsupported works. I don't know if there is any other interaction that is possible, but I'm not sure what you mean by that.

jsternberg commented 7 years ago

@peterh is there anything else I need to do to make this PR mergeable?

peterh commented 7 years ago

Oh, my apologies. I completely forgot that this was pending.

What I was trying to say was "How about 22e0727f76c777a1c7e91819a8532fcb59bca92d ?" I'd rather avoid returning an extra error if possible.

It looks like we might need your patch too, for safety, but most use cases shouldn't need to handle the extra error (unless the terminal width is likely to go back to zero later?)

Comments?

jsternberg commented 7 years ago

I'm going to try it out in a docker container under the scenario described above and I'll get back to you. I'm a bit skeptical since it still seems to try reading the prompt and if it's going to fail, I'd rather it fail fast and consistently, but let me see how it functions.

The important part to me is that we'll be able to inspect the error and write out a helpful error message.

jsternberg commented 7 years ago

I tried it out and I don't think the behavior is desirable. It successfully prints the prompt, but then it just freezes until you push Ctrl+C and you are unable to type anything into the prompt. I think this will be more confusing than just giving up and stating, "This situation is impossible to deal with."

peterh commented 7 years ago

It just freezes? Ick. So how do you launch the application in the first place? Can you even use bash? Can you report this as a bug to the Docker people?

Without this, I'm not sure what to do about #88 , which I presume is not using Docker, but has the same "terminal exists and has zero columns" bug.

Still looking for ideas.

peterh commented 7 years ago

Oh, I see. We bake the terminal mode before entering Prompt. Give me a minute to modify my commit.

peterh commented 7 years ago

Could you please give 5095b625d32db5ffe3e1ef5fe272b2546ba11df8 a try and let me know what happens?

jsternberg commented 7 years ago

That appears to work! I get a semi-functional terminal too and Ctrl+D correctly signals an EOF.

peterh commented 7 years ago

Pushed. Thanks for all the testing.

xlight commented 7 years ago

run "stty cols 80" after docker exec -ti