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

fix race condition in terminal init #457

Merged
merged 1 commit into from Mar 22, 2016

Conversation

Projects
None yet
3 participants
@ailin-nemui
Contributor

ailin-nemui commented Mar 22, 2016

remove the tcgetattr call to a single time on irssi load instead of
querying it each time. Fixes #450

fix race condition in terminal init
remove the tcgetattr call to a single time on irssi load instead of
querying it each time. Fixes #450
@ailin-nemui

This comment has been minimized.

Contributor

ailin-nemui commented Mar 22, 2016

(I hope)

@LemonBoy

This comment has been minimized.

Member

LemonBoy commented Mar 22, 2016

I think there's no need to call terminfo_term_init again after the kill SIGSTP returns, a strace shows that the terminal settings are applied twice because of that

@ailin-nemui

This comment has been minimized.

Contributor

ailin-nemui commented Mar 22, 2016

they may be applied twice because of term_init being called twice, once from the do_redraw sigcont handler https://github.com/irssi/irssi/blob/master/src/fe-text/term-terminfo.c#L91 and once from the kill-stop function https://github.com/irssi/irssi/blob/master/src/fe-text/term-terminfo.c#L610

@LemonBoy

This comment has been minimized.

Member

LemonBoy commented Mar 22, 2016

Exactly, and #452 fixes the problem by sending the smcup only (if available) when we resume from the TSTP signal, we save the current state and then the old settings are re-applied in the SIGCNT handler

@ailin-nemui

This comment has been minimized.

Contributor

ailin-nemui commented Mar 22, 2016

in that case I wonder if even the smcup of the code in your suggestion from #452 is necessary? if so, why?

@LemonBoy

This comment has been minimized.

Member

LemonBoy commented Mar 22, 2016

The smcup saves the terminal window to be re-applied with a rmcup on the next resume/exit

@ailin-nemui

This comment has been minimized.

Contributor

ailin-nemui commented Mar 22, 2016

but terminfo_cont (which is run by the sigcont hander) also runs smcup so why do we need to send that twice?

@LemonBoy

This comment has been minimized.

Member

LemonBoy commented Mar 22, 2016

You're right, upon further inspection I've updated the PR

@dequis dequis removed the 0.8.18 label Mar 22, 2016

@ailin-nemui

This comment has been minimized.

Contributor

ailin-nemui commented Mar 22, 2016

after discussion on IRC I conclude that we want to have both this and the #452 eventually

ailin-nemui added a commit that referenced this pull request Mar 22, 2016

Merge pull request #457 from ailin-nemui/fix_450
fix race condition in terminal init

@ailin-nemui ailin-nemui merged commit 29e160f into irssi:master Mar 22, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

ailin-nemui added a commit to ailin-nemui/irssi that referenced this pull request Mar 22, 2016

Merge pull request irssi#457 from ailin-nemui/fix_450
fix race condition in terminal init

ailin-nemui added a commit to ailin-nemui/irssi that referenced this pull request Mar 22, 2016

Merge pull request irssi#457 from ailin-nemui/fix_450
fix race condition in terminal init

@ailin-nemui ailin-nemui deleted the ailin-nemui:fix_450 branch Mar 24, 2016

ailin-nemui added a commit to ailin-nemui/irssi that referenced this pull request Aug 7, 2017

Revert "Merge pull request irssi#452 from LemonBoy/terminfo-cup"
Fixes irssi#733. The fix outlined in irssi#452 had adverse effects for the
following reason. The code removed the restoration path that would go on
the code path from kill SIGTSTP. The problem is this: When Irssi is not
running in a controlling parent (like a shell), the TSTP will in fact be
ignored. In that case, there is no process sending a CONT either and
thus the screen state never gets restored. Luckily, the patch in irssi#457 is
sufficient to prevent the problem in irssi#450 (which lead to the development
of irssi#452). To that end, we do end up with potentially calling
terminfo_cont twice but that is better than not calling it at all.

This reverts commit b1ffd5f, reversing
changes made to 9cb0419.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment