classification
Title: Readline not adjusting width after resize with 6.3
Type: behavior Stage: resolved
Components: Extension Modules Versions: Python 3.6, Python 3.5, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Carlos Pita, Christian Zommerfelds, Eric Price, count0, johnmorr, josh.r, martin.panter, neologix, python-dev, r.david.murray, serhiy.storchaka, takluyver
Priority: normal Keywords: patch

Created on 2015-03-22 00:42 by Carlos Pita, last changed 2016-04-03 10:04 by martin.panter. This issue is now closed.

Files
File name Uploaded Description Edit
rl_sigwinch_update.patch Eric Price, 2015-10-27 18:29 review
rl_sigwinch_calling_old_handler.patch Eric Price, 2016-03-23 03:04 Updated patch to call old handler
rl_sigwinch_version_3.patch Eric Price, 2016-03-27 04:40 Includes a config flag and reinstalls itself review
rl_sigwinch_version_4.patch Eric Price, 2016-03-27 13:43 Reinstall itself after calling python's signal handler review
Messages (29)
msg238857 - (view) Author: Carlos Pita (Carlos Pita) Date: 2015-03-22 00:42
See here:

https://github.com/ipython/ipython/issues/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.
msg238860 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-03-22 01:16
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?
msg238868 - (view) Author: Carlos Pita (Carlos Pita) Date: 2015-03-22 04:27
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.
msg238870 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-03-22 05:06
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.
msg253105 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-10-16 23:19
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.
msg253336 - (view) Author: Eric Price (Eric Price) * Date: 2015-10-22 16:25
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'.
msg253359 - (view) Author: Josh Rosenberg (josh.r) * Date: 2015-10-23 02:30
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).
msg253361 - (view) Author: Eric Price (Eric Price) * Date: 2015-10-23 02:54
Right, thanks, I've updated the patch.
msg253364 - (view) Author: Josh Rosenberg (josh.r) * Date: 2015-10-23 03:09
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.
msg253365 - (view) Author: Eric Price (Eric Price) * Date: 2015-10-23 03:30
Hmm, hopefully the review tool works now.  (I was generating the patch manually from a source tarball, not a checkout.)
msg253535 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-10-27 12:43
How does this patch affect a user-defined SIGWINCH handler, or is that not really supported when Readline is in use?
msg253559 - (view) Author: Eric Price (Eric Price) * Date: 2015-10-27 18:29
SIGWINCH handler for readline.  Fixed indentation issues.
msg253560 - (view) Author: Eric Price (Eric Price) * Date: 2015-10-27 18:36
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.
msg261848 - (view) Author: Christian Zommerfelds (Christian Zommerfelds) Date: 2016-03-16 10:39
This bug makes the REPL totally unusable because larger lines start to wrap in a very strange and unreadable way.
What has to be done in order for this to get accepted? It looks like there is no better way to define the signal handler and that the user has to know to properly call the old signal handler if it needs to.
msg262166 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-03-22 07:25
I think it is conceivable that people could use Readline _and_ do their own stuff with the terminal. Consider someone playing with terminal stuff in the interactive interpreter (which happens to use Readline). I understand Readline returns the terminal to a sensible state after it returns you a line.

Another concern with the patch is that signal.getsignal(SIGWINCH) returns SIG_DFL, although according to the documentation it should probably return None (meaning a signal handler was not installed by Python). Not sure if that is a bug with the patch or with the signal module though :)

Also it looks like this patch will fail with Editline (Gnu Readline substitute used on OS X), and therefore probably also older versions of Gnu Readline. With my patch from Issue 13501 to enable Editline on Linux, I get these errors:

/media/disk/home/proj/python/cpython/Modules/readline.c: In function ‘readline_until_enter_or_signal’:
/media/disk/home/proj/python/cpython/Modules/readline.c:1116:17: warning: implicit declaration of function ‘rl_resize_terminal’ [-Wimplicit-function-declaration]
                 rl_resize_terminal();
                 ^
*** WARNING: renaming "readline" since importing it failed: build/lib.linux-x86_64-3.6-pydebug/readline.cpython-36dm-x86_64-linux-gnu.so: undefined symbol: rl_resize_terminal

Following modules built successfully but were removed because they could not be imported:
readline
msg262170 - (view) Author: Eric Price (Eric Price) * Date: 2016-03-22 08:20
rl_resize_terminal was added in readline 4.0, released in 1999.  I think we can rely on it. =)

I don't know editline, but it probably doesn't include the readline 6.3 change that makes this necessary, so your patch for it could just IFDEF out that line.

I think SIG_DFL is a pretty reasonable return value, actually.  It's clearly not SIG_IGN or a python handle, it's not really "unknown", and it can be described as default behavior: it is the default way readline is supposed to work.

Finally, while I agree that it is conceivable that one could try to use readline + other terminal mucking with SIGWINCH, (a) this didn't work before the 6.3 change, (b) it doesn't work now, (c) it's far less common than the standard case of using readline normally, and (d) supporting such usage would be complicated and require a readline module API change.  Let's start by supporting normal usage.
msg262226 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-03-23 01:43
In configure.ac there are explicit checks for pre-2.1 Readline versions (and later versions). Maybe it is reasonable to require 4.0 these days. But I think this should only be done in the next Python version (3.6+).

To support Editline (and pre-4.0 Readline versions), I agree some conditional #ifdef thing would probably be best. My version of Editline claims to support 4.2, but doesn’t have rl_resize_terminal().

I played with Readline 6.2 where this resize bug is not present. Getsignal() also returns SIG_DFL there, so I won’t worry too much about that :)

But it seems everything else worked fairly smoothly with 6.2. If you register your own SIGWINCH handler, it seems to be chain called by Readline. However with the current patch there are three differences that I can see to the 6.2 behaviour:

* it overrides the SIGWINCH handler at import
* it does not chain to the old SIGWINCH handler
* the handler in “readline” is lost if another handler is set after import

You say it didn’t work before 6.3; what did you try? Also, I think a custom SIGWINCH handler does currently work with 6.3, and the patch would break that. How do you mean that doesn’t work now?

So I am still a bit concerned that in some circumstances the current patch could break working programs.

Maybe there is a solution that doesn’t need an API change: Only register SIGWINCH in readline_until_enter_or_signal(), and chain call to the original handler as well.

FWIW I am not a fan of signals, because they are hard to handle robustly. But in this case I don’t know of a good alternative. Perhaps we could call pselect() instead of select(), and mask SIGWINCH, but I doubt it is worth the effort (portablility problems?). Or maybe something similar to signal.set_wakeup_fd() would be better.
msg262230 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-03-23 02:31
Issue 13285 is open about tracking non-Python signal handlers.

I also found Issue 2675 about curses SIGWINCH handling in the interactive interpreter or when the readline module is imported, and Issue 3948 about someone trying to use both readline and SIGWINCH.
msg262233 - (view) Author: Eric Price (Eric Price) * Date: 2016-03-23 03:04
Hmm, OK.  I've made a new version of the patch that chains signal handlers.  I think that should make it match previous functionality, and work with basic combined usage or python signal handlers.

It probably still won't work if you, say, want to start and then stop a signal handler installed in C, but that's basically Issue 13285.
msg262235 - (view) Author: Eric Price (Eric Price) * Date: 2016-03-23 03:14
(To be clear, it now works in general for Python signal handlers, and when C handlers are inserted but not deleted.)

For a conditional ifdef, does that mean I should add another test/variable in configure.ac?
msg262482 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-03-26 03:34
Unfortunately, yes I think another test might be needed in configure.ac, unless you can piggy-back on one of the existing flags.

One more thing that I thought of: Looking at the main Python signal handler, it reinstalls itself, see <https://hg.python.org/cpython/file/v3.5.1/Modules/signalmodule.c#l306>. This is apparently because handlers installed by Posix signal() might be only called once. But if the handler was installed by sigaction(), it should not be reinstalled. Should our SIGWINCH handler do something similar?
msg262508 - (view) Author: Eric Price (Eric Price) * Date: 2016-03-27 04:40
Well, I could piggyback on the existing flags if I just wanted to support readline -- there are already two flags for readline 4.0 -- but if our real goal is to be compatible with editline, we probably need another flag.

I think you're right that it should reinstall itself, in case it's installed using signal().

I've made a new patch that includes a config flag and puts everything inside the ifdefs.
msg262518 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-03-27 11:25
I think this version is pretty good. I would move the signal restoring bit to the end of the handler (see review), and there is some funny indentation, but I can fix that up when I commit it.
msg262522 - (view) Author: Eric Price (Eric Price) * Date: 2016-03-27 13:43
Hmm, I'm not seeing comments in the review?

You're right about the order.  When we don't have sigaction, python's signal handler will reinstall itself, so we need to reinstall this one after calling the old handler.
msg262535 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-03-27 20:51
Sorry I forgot to send my draft. Sent now, but you figured it out :)
msg262813 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-04-03 04:21
New changeset b29edd0108ee by Martin Panter in branch '3.5':
Issue #23735: Add SIGWINCH handler for Readline 6.3+ support, by Eric Price
https://hg.python.org/cpython/rev/b29edd0108ee

New changeset 41c2f8742bfe by Martin Panter in branch 'default':
Issue #23735: Merge Readline resize handling from 3.5
https://hg.python.org/cpython/rev/41c2f8742bfe

New changeset 9f237e81cd15 by Martin Panter in branch '2.7':
Issue #23735: Add SIGWINCH handler for Readline 6.3+ support, by Eric Price
https://hg.python.org/cpython/rev/9f237e81cd15
msg262815 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-04-03 06:39
http://buildbot.python.org/all/builders/AMD64%20OpenIndiana%203.x/builds/10490/steps/compile/logs/warnings%20%282%29

/export/home/buildbot/64bits/3.x.cea-indiana-amd64/build/Modules/readline.c:939: warning: implicit declaration of function 'sigwinch_ohandler'
msg262819 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-04-03 08:28
New changeset e3f375047edf by Martin Panter in branch '3.5':
Issue #23735: Avoid sighandler_t Gnu-ism
https://hg.python.org/cpython/rev/e3f375047edf

New changeset 0e576d094dc4 by Martin Panter in branch 'default':
Issue #23735: Merge sighandler_t fix from 3.5
https://hg.python.org/cpython/rev/0e576d094dc4

New changeset 596946c06a31 by Martin Panter in branch '2.7':
Issue #23735: Avoid sighandler_t Gnu-ism
https://hg.python.org/cpython/rev/596946c06a31
msg262820 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-04-03 10:04
Thanks for pointing that out Serhiy. It turns out sighandler_t is non-standard. My latest tweak seems to have fixed things.

Also thankyou Eric for your persistence with this patch :)
History
Date User Action Args
2016-04-03 10:04:33martin.pantersetstatus: open -> closed
resolution: fixed
messages: + msg262820

stage: patch review -> resolved
2016-04-03 08:28:22python-devsetmessages: + msg262819
2016-04-03 06:39:09serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg262815
2016-04-03 04:21:04python-devsetnosy: + python-dev
messages: + msg262813
2016-03-27 20:51:48martin.pantersetmessages: + msg262535
2016-03-27 13:43:27Eric Pricesetfiles: + rl_sigwinch_version_4.patch

messages: + msg262522
2016-03-27 11:25:15martin.pantersetmessages: + msg262518
2016-03-27 04:40:59Eric Pricesetfiles: + rl_sigwinch_version_3.patch

messages: + msg262508
2016-03-26 03:34:54martin.pantersetmessages: + msg262482
versions: + Python 3.6, - Python 3.4
2016-03-23 03:14:46Eric Pricesetmessages: + msg262235
2016-03-23 03:04:45Eric Pricesetfiles: + rl_sigwinch_calling_old_handler.patch

messages: + msg262233
2016-03-23 02:31:10martin.pantersetmessages: + msg262230
2016-03-23 01:43:56martin.pantersetmessages: + msg262226
2016-03-22 08:20:43Eric Pricesetmessages: + msg262170
2016-03-22 07:25:12martin.pantersetmessages: + msg262166
2016-03-16 10:39:14Christian Zommerfeldssetmessages: + msg261848
2016-03-16 10:30:56Christian Zommerfeldssetnosy: + Christian Zommerfelds
2016-02-01 10:21:37count0setnosy: + count0
2015-10-27 18:36:14Eric Pricesetmessages: + msg253560
2015-10-27 18:29:55Eric Pricesetfiles: - rl_sigwinch_update.patch
2015-10-27 18:29:31Eric Pricesetfiles: + rl_sigwinch_update.patch

messages: + msg253559
2015-10-27 12:43:34martin.pantersetmessages: + msg253535
stage: needs patch -> patch review
2015-10-23 03:30:55Eric Pricesetfiles: - rl_sigwinch_update.patch
2015-10-23 03:30:47Eric Pricesetfiles: + rl_sigwinch_update.patch

messages: + msg253365
2015-10-23 03:09:21josh.rsetmessages: + msg253364
2015-10-23 02:54:41Eric Pricesetfiles: - rl_sigwinch_update.patch
2015-10-23 02:54:30Eric Pricesetfiles: + rl_sigwinch_update.patch

messages: + msg253361
2015-10-23 02:30:13josh.rsetnosy: + josh.r
messages: + msg253359
2015-10-22 16:25:52Eric Pricesetfiles: + rl_sigwinch_update.patch

nosy: + Eric Price
messages: + msg253336

keywords: + patch
2015-10-16 23:19:53martin.pantersetnosy: + martin.panter
messages: + msg253105

components: + Extension Modules
stage: needs patch
2015-10-16 17:29:25johnmorrsetnosy: + johnmorr
2015-03-22 05:14:26takluyversetnosy: + takluyver
2015-03-22 05:06:55r.david.murraysetversions: + Python 2.7, Python 3.4, Python 3.5
nosy: + neologix

messages: + msg238870

type: behavior
2015-03-22 04:27:08Carlos Pitasetmessages: + msg238868
2015-03-22 01:16:25r.david.murraysetnosy: + r.david.murray
messages: + msg238860
2015-03-22 00:42:54Carlos Pitacreate