See here:https://github.com/ipython/ipython/issues/6974Chet Ramey confirmed the bug is downstream:As I recall from looking briefly at the ipython/python source code, it hasto do with python not handling the SIGWINCH and expecting readline to doit even when readline is not active. In readline-6.3, readline onlyinstalls its signal handlers when rl_callback_read_char is called, whichpython does only when it receives a character and its select(2) callreturns. The way readline-6.2 and earlier did things, it could `steal'signals from an application without being active. I think python doesn'thandle SIGWINCH at all, which means that it expects readline's signalhandler to be active all the time, instead of just when python calls backinto readline to have it read a character.A possible fix would be to have python's readline module install a signalhandler for SIGWINCH and have it set readline's idea of the screen size.

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?

Chet again:Here's the story from the top.Prior to readline-6.3, readline could `steal' signals from the callingapplication in the sense that readline's signal handler got a crack atall signals in which readline was interested before the application did.Now, that was usually ok, since readline handled signals immediatelyupon receipt and resent the signal to the calling application.  It didall this in a signal handler context.It's dangerous to execute `unsafe' library functions and system calls froma signal handler. The Posix standard has a (short) list of signal-safefunctions.  Before bash-4.3/readline-6.3, readline and bash did far toomuch work in signal handlers.  The most you are supposed to do in a signalhandler is set variables, preferably of type sig_atomic_t, and nothingelse.  The biggest offenders are malloc and free, for two reasons:applications often want to provide their own memory allocation atopmalloc and free, and using them from signal handlers can interfere; andthe glibc versions use internal locks extensively, and calling, say, freefrom a signal handler can end up in a deadlock.I made some progress up to bash-4.2/readline-6.2 in deferring `real' workuntil readline wasn't in a signal handling context using theRL_CHECK_SIGNALS() macro, but there were still a few places left thathandled the signal immediately.  One of those places was the callbackhandling code; another was the SIGWINCH handling.The SIGWINCH code signal handling functions eventually generated the samesort of bug reports as other signals.  One representative report ishttp://lists.gnu.org/archive/html/bug-bash/2011-02/msg00291.htmlThe fix was to make SIGWINCH handling the same as other signals: set aflag 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 handlingsignals immediately instead of deferring handling until out of a signalhandler context; one such report ishttp://lists.gnu.org/archive/html/bug-readline/2011-07/msg00010.htmlSo now the SIGWINCH and the callback code had to be changed to avoidunsafe function calls from within a signal handler.That very quickly uncovered a problem: the issue of readline stealingthe application's signals became much worse.http://lists.gnu.org/archive/html/bug-readline/2011-07/msg00012.htmlis the first explanation of that phenomenon.  If readline's signalhandlers are called when the application is active (e.g., between thecalls to rl_callback_handler_install and rl_callback_read_char), and allthey do is set a private flag the application doesn't know about, thesignals will effectively be ignored until the next time the applicationcalls 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 onlywhen readline is active and can check the flag the handlers set.And so we reach where we are.  If a SIGWINCH arrives while readline isnot active, and the application using callback mode does not catch it andtell 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.3behavior was acceptable because they never encountered a problem, and thatany change must therefore be a bug.Please feel free to add this message to the python issue tracker.

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.

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.

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'.

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+handlersAlso, 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).

Right, thanks, I've updated the patch.

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.

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

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

SIGWINCH handler for readline.  Fixed indentation issues.

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.

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.

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 fromIssue 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_terminalFollowing modules built successfully but were removed because they could not be imported:readline

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.

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 importYou 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.

Issue 13285 is open about tracking non-Python signal handlers.I also foundIssue 2675 about curses SIGWINCH handling in the interactive interpreter or when the readline module is imported, andIssue 3948 about someone trying to use both readline and SIGWINCH.

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 basicallyIssue 13285.

(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?

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?

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.

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.

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.

Sorry I forgot to send my draft. Sent now, but you figured it out :)

New changesetb29edd0108ee by Martin Panter in branch '3.5':Issue#23735: Add SIGWINCH handler for Readline 6.3+ support, by Eric Pricehttps://hg.python.org/cpython/rev/b29edd0108eeNew changeset41c2f8742bfe by Martin Panter in branch 'default':Issue#23735: Merge Readline resize handling from 3.5https://hg.python.org/cpython/rev/41c2f8742bfeNew changeset9f237e81cd15 by Martin Panter in branch '2.7':Issue#23735: Add SIGWINCH handler for Readline 6.3+ support, by Eric Pricehttps://hg.python.org/cpython/rev/9f237e81cd15

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'

New changesete3f375047edf by Martin Panter in branch '3.5':Issue#23735: Avoid sighandler_t Gnu-ismhttps://hg.python.org/cpython/rev/e3f375047edfNew changeset0e576d094dc4 by Martin Panter in branch 'default':Issue#23735: Merge sighandler_t fix from 3.5https://hg.python.org/cpython/rev/0e576d094dc4New changeset596946c06a31 by Martin Panter in branch '2.7':Issue#23735: Avoid sighandler_t Gnu-ismhttps://hg.python.org/cpython/rev/596946c06a31

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 :)