Return an error when the terminal reports zero columns and is refreshed #87
Conversation
|
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 |
|
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? |
|
See also #75 |
|
I think |
When used within a call to `docker exec -it` where the original container does not have a TTY allocated to it (such as in moby/moby#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.
|
@peterh is there anything else I need to do to make this PR mergeable? |
|
Oh, my apologies. I completely forgot that this was pending. What I was trying to say was "How about 22e0727 ?" 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? |
|
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. |
|
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." |
|
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. |
|
Oh, I see. We bake the terminal mode before entering Prompt. Give me a minute to modify my commit. |
|
Could you please give 5095b62 a try and let me know what happens? |
|
That appears to work! I get a semi-functional terminal too and Ctrl+D correctly signals an EOF. |
|
Pushed. Thanks for all the testing. |
|
run "stty cols 80" after docker exec -ti |
When used within a call to
docker exec -itwhere the originalcontainer does not have a TTY allocated to it (such as in
moby/moby#8755), the number of columns read from the
ioctl()callwill be zero, but it will not return an error. If you call
Prompt()orPasswordPrompt()after this, the prompt will panic when it tries todivide a number by the number of columns (which is zero).
This change detects when the number of columns returned is zero and
returns
ErrPromptUnsupportedfrom thePromptXXXmethods to avoid apanic and so the application is able to deal with the problem more
easily.