Readline not adjusting width after resize with 6.3

[CarlosPita]

BPO	23735
Nosy	@bitdancer, @takluyver, @vadmium, @serhiy-storchaka, @MojoVampire
Files	rl_sigwinch_update.patch rl_sigwinch_calling_old_handler.patch: Updated patch to call old handler rl_sigwinch_version_3.patch: Includes a config flag and reinstalls itself rl_sigwinch_version_4.patch: Reinstall itself after calling python's signal handler

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2016-04-03.10:04:33.702>
created_at = <Date 2015-03-22.00:42:54.297>
labels = ['extension-modules', 'type-bug']
title = 'Readline not adjusting width after resize with 6.3'
updated_at = <Date 2016-04-03.10:04:33.701>
user = 'https://bugs.python.org/CarlosPita'

bugs.python.org fields:

activity = <Date 2016-04-03.10:04:33.701>
actor = 'martin.panter'
assignee = 'none'
closed = True
closed_date = <Date 2016-04-03.10:04:33.702>
closer = 'martin.panter'
components = ['Extension Modules']
creation = <Date 2015-03-22.00:42:54.297>
creator = 'Carlos Pita'
dependencies = []
files = ['40870', '42246', '42302', '42307']
hgrepos = []
issue_num = 23735
keywords = ['patch']
message_count = 29.0
messages = ['238857', '238860', '238868', '238870', '253105', '253336', '253359', '253361', '253364', '253365', '253535', '253559', '253560', '261848', '262166', '262170', '262226', '262230', '262233', '262235', '262482', '262508', '262518', '262522', '262535', '262813', '262815', '262819', '262820']
nosy_count = 12.0
nosy_names = ['r.david.murray', 'neologix', 'python-dev', 'takluyver', 'martin.panter', 'serhiy.storchaka', 'josh.r', 'Carlos Pita', 'count0', 'johnmorr', 'Eric Price', 'Christian Zommerfelds']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue23735'
versions = ['Python 2.7', 'Python 3.5', 'Python 3.6']

[CarlosPita]

See here:

ipython/ipython#6974

Chet Ramey confirmed the bug is downstream:

As I recall from looking briefly at the ipython/python source code, it has
to do with python not handling the SIGWINCH and expecting readline to do
it even when readline is not active. In readline-6.3, readline only
installs its signal handlers when rl_callback_read_char is called, which
python does only when it receives a character and its select(2) call
returns. The way readline-6.2 and earlier did things, it could `steal'
signals from an application without being active. I think python doesn't
handle SIGWINCH at all, which means that it expects readline's signal
handler to be active all the time, instead of just when python calls back
into readline to have it read a character.

A possible fix would be to have python's readline module install a signal
handler for SIGWINCH and have it set readline's idea of the screen size.

[bitdancer]

Why would this not constitute a bug in readline? Readline isn't going to be active all of the time in most applications, so why shouldn't it be readline's responsibility to install the signal handler at initialization and handle it? If the application wants control of that signal, it can then install its own handler after readline initialization completes. Clearly it used to work that way. Is there some documentation from readline you can point us to that explains the reasoning behind this change in behavior?

[CarlosPita]

Chet again:

Here's the story from the top.

Prior to readline-6.3, readline could `steal' signals from the calling
application in the sense that readline's signal handler got a crack at
all signals in which readline was interested before the application did.
Now, that was usually ok, since readline handled signals immediately
upon receipt and resent the signal to the calling application. It did
all this in a signal handler context.

It's dangerous to execute `unsafe' library functions and system calls from
a signal handler. The Posix standard has a (short) list of signal-safe
functions. Before bash-4.3/readline-6.3, readline and bash did far too
much work in signal handlers. The most you are supposed to do in a signal
handler is set variables, preferably of type sig_atomic_t, and nothing
else. The biggest offenders are malloc and free, for two reasons:
applications often want to provide their own memory allocation atop
malloc and free, and using them from signal handlers can interfere; and
the glibc versions use internal locks extensively, and calling, say, free
from a signal handler can end up in a deadlock.

I made some progress up to bash-4.2/readline-6.2 in deferring `real' work
until readline wasn't in a signal handling context using the
RL_CHECK_SIGNALS() macro, but there were still a few places left that
handled the signal immediately. One of those places was the callback
handling code; another was the SIGWINCH handling.

The SIGWINCH code signal handling functions eventually generated the same
sort of bug reports as other signals. One representative report is

http://lists.gnu.org/archive/html/bug-bash/2011-02/msg00291.html

The fix was to make SIGWINCH handling the same as other signals: set a
flag and defer handling until no longer in a signal handling context.
This was necessary in both `direct' and callback modes.

The gdb folks reported most of the problems with the callback code handling
signals immediately instead of deferring handling until out of a signal
handler context; one such report is

http://lists.gnu.org/archive/html/bug-readline/2011-07/msg00010.html

So now the SIGWINCH and the callback code had to be changed to avoid
unsafe function calls from within a signal handler.

That very quickly uncovered a problem: the issue of readline stealing
the application's signals became much worse.

http://lists.gnu.org/archive/html/bug-readline/2011-07/msg00012.html

is the first explanation of that phenomenon. If readline's signal
handlers are called when the application is active (e.g., between the
calls to rl_callback_handler_install and rl_callback_read_char), and all
they do is set a private flag the application doesn't know about, the
signals will effectively be ignored until the next time the application
calls into the readline callback interface and readline can check signals.
This is not acceptable in most contexts, including for SIGWINCH.

The fix for that is to make readline's signal handlers be active only
when readline is active and can check the flag the handlers set.

And so we reach where we are. If a SIGWINCH arrives while readline is
not active, and the application using callback mode does not catch it and
tell readline about the updated screen dimensions (rl_set_screen_size()
exists for this purpose), readline will not know about the new size.

It seems like the issue is that people assume that the pre-readline-6.3
behavior was acceptable because they never encountered a problem, and that
any change must therefore be a bug.

Please feel free to add this message to the python issue tracker.

[bitdancer]

If it used to handle the signal and then re-raise it, why doesn't it now set its flag and then re-raise the signal? (I also don't understand why the API isn't that the application takes back the signal handler if it needs it if it is using readline, but I'm not familiar enough with signal handling in applications to really judge that, I suppose.)

However, we certainly understand the issues with doing too much in signal handlers, so if that ship has sailed I guess we'll just have to deal with it.

[vadmium]

One idea might be to synchronously poll the screen size each time before Readline is invoked. Would doing this be such a burden? The polling could be limited to once every 0.1 s or so if it was a big burden. These ways would avoid interfering with signal handlers entirely.

[EricPrice]

This patch seems to fix the issue for me, by installing a signal handler for SIGWINCH that sets a flag which is checked while waiting for input.

One could make a simpler patch that just calls rl_resize_terminal() from the signal handler. That would essentially replicate the pre-readline 6.2 behavior, which worked fine, but supposedly it's `dangerous'.

[MojoVampire]

Patch doesn't let me comment, but I believe to be strictly standards conformant, sigwinch_received should be declared as a volatile sig_atomic_t, not char. See: https://www.securecoding.cert.org/confluence/display/c/SIG31-C.+Do+not+access+shared+objects+in+signal+handlers

Also, to close an incredibly narrow race window, I believe sigwinch_received should be set to zero before calling rl_resize_terminal(), on the theory that otherwise, the signal could be received just after the call, but before setting the flag to 0, the signal handler would set it to 1, but we'd promptly squash that by setting it back to 0 (even though a resize occurred that should be handled).

[EricPrice]

Right, thanks, I've updated the patch.

[MojoVampire]

Okay, sorry for repeated nitpicks, but I think you can keep the visibility of sigwinch_received as static like it was as char (to avoid having it be exported for potential use by other modules, though it won't gain any optimization benefits of static, since volatile intentionally prevents any such optimizations). static volatile sig_atomic_t is kind of a mouthful, but keeps the limited visibility you want.

Also, I still don't see a review link for the patch (which limits the ability to review in context); you might need to update your checkout and regenerate the patch so the review tool can handle it.

[EricPrice]

Hmm, hopefully the review tool works now. (I was generating the patch manually from a source tarball, not a checkout.)

[vadmium]

How does this patch affect a user-defined SIGWINCH handler, or is that not really supported when Readline is in use?

[EricPrice]

SIGWINCH handler for readline. Fixed indentation issues.

[EricPrice]

At the moment, it just overwrites any existing SIGWINCH handler.

I don't know how to do anything better -- one could try to store an existing SIGWINCH handler and call it, but it still wouldn't really work for an application that wanted to add and remove handlers. I think such applications are rare, though -- readline mucks with the terminal in various ways, so it's unlikely that other applications will also muck with the terminal and expect to be compatible with it.