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

Skip critical on NO TTY case #11

Closed
wants to merge 1 commit into from
Closed

Conversation

rushprint
Copy link

@rushprint rushprint commented Sep 29, 2020

Hello Nelo.

I compared a few tools, but gow is the best.
However, when I am using it in Docker container I have a problem.
When it run and try to make terminal in raw mode it fails and finish because of critical(err).

state, err := makeTerminalRaw(syscall.Stdin)
critical(err)
                ===========> at this point gow quit with  "[gow] inappropriate ioctl for device" message.
termios = &state

The root cause is that docker containers may run without terminal.
I think we have a few solutions,

  • just ignore errors on ENOTTY error case.
  • a command-line flag to avoid terminal setting operation.
  • catch every signals and forward them to sub-process.

I thought the first one is most easy and made a PR.
Please accept or deny it If you have different idea.

Best regards
Sungkyu Kim

@mitranim
Copy link
Owner

Thanks for reporting and fixing the issue. Glad to hear you like the tool! 🙂

I'm thinking of a slightly more general solution. Instead of ignoring termios errors, it would be better to have a mode where the terminal state is not altered at all. I haven't checked, but in addition to Docker, this probably also breaks on Windows, and possibly in some other places. Issue #6 about Windows support has been pending for a while.

Adding such a mode might require more than a boolean flag wrapping "alter terminal" and "restore terminal" calls, because of signal handling. It might even turn out that we don't need the raw state, and could just register signal handlers instead. It's been a while since I checked. Been meaning to get around to this, but not sure when...

@mitranim
Copy link
Owner

This is probably fine to merge, but it needs some fixes:

if err != nil {
	if errors.Is(err, syscall.ENOTTY) {
		log.Println("failed to set raw terminal mode: no terminal exists")
	} else {
		critical(err)
	}
} else {
	termios = &state
}
  • Use errors.Is.
  • Use log.Println and lowercase for consistency with other logging.
  • Set termios to non-nil only if the terminal state was successfully altered. If no terminal exists, leave it nil so that cleanup doesn't try to alter the state again.

mitranim added a commit that referenced this pull request Oct 27, 2020
When failing to switch the terminal into raw mode because no TTY exists,
continue running. This might be useful when running gow inside Docker
containers. Haven't personally tested, there might be gotchas.

Addresses GH issue #11.
@mitranim
Copy link
Owner

Applied the patch locally and tested for a while. Doesn't seem to cause problems in normal operation. Pushed the changes, please see if this works for you in Docker. 🙂 Closing the pull for now, feel free to reopen if this doesn't work.

@mitranim mitranim closed this Oct 27, 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

Successfully merging this pull request may close these issues.

2 participants