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
Readline not adjusting width after resize with 6.3 #67923
Comments
See here: Chet Ramey confirmed the bug is downstream: As I recall from looking briefly at the ipython/python source code, it has A possible fix would be to have python's readline module install a signal |
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 calling It's dangerous to execute `unsafe' library functions and system calls from I made some progress up to bash-4.2/readline-6.2 in deferring `real' work The SIGWINCH code signal handling functions eventually generated the same 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 The gdb folks reported most of the problems with the callback code handling 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 That very quickly uncovered a problem: the issue of readline stealing http://lists.gnu.org/archive/html/bug-readline/2011-07/msg00012.html is the first explanation of that phenomenon. If readline's signal The fix for that is to make readline's signal handlers be active only And so we reach where we are. If a SIGWINCH arrives while readline is It seems like the issue is that people assume that the pre-readline-6.3 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 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). |
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. |
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 bpo-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’: Following modules built successfully but were removed because they could not be imported: |
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:
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. |
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 bpo-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 changeset b29edd0108ee by Martin Panter in branch '3.5': New changeset 41c2f8742bfe by Martin Panter in branch 'default': New changeset 9f237e81cd15 by Martin Panter in branch '2.7': |
/export/home/buildbot/64bits/3.x.cea-indiana-amd64/build/Modules/readline.c:939: warning: implicit declaration of function 'sigwinch_ohandler' |
New changeset e3f375047edf by Martin Panter in branch '3.5': New changeset 0e576d094dc4 by Martin Panter in branch 'default': New changeset 596946c06a31 by Martin Panter in branch '2.7': |
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 :) |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: