Thread: Question about Ctrl-C and less
This behaviour has been around so long that I've gotten used to it but I've always considered it a bug. Yet it has never been fixed so I'm going to ask if anybody else has issues with this behaviour. Reproducing it is easy: 1. Set PAGER=less 2. Start psql 3. Type: \df 4. When output appears, hit Control-C Now watch as both psql and less try to read your characters, each getting about half and getting very confused. Even if you manage to get one of the two to quit, your terminal is still left in a scrambled state, generally requiring a soft-reset on the terminal. The fix is easy: where currently the code with popen/pclose ignores SIGPIPE, tell it to also ignore SIGINT and restore the normal signal handler on quit. This is almost in line with the system() function which blocks SIGQUIT and SIGINT while the subprocess is running. This problem has been around for ever yet obviously not everybody runs into it all the time like I do. Would patch to fix this be accepted or is there a reason why not? Actually, I'm somewhat in favour if getting rid of the longjmp from the signal handler and instead setting a flag to be checked at strategic points in the code to abort whatever it is doing. The current way leaks memory like crazy. If you're worried about infinite loops there's always SIGQUIT. Thanks in advance, -- Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/ > Patent. n. Genius is 5% inspiration and 95% perspiration. A patent is a > tool for doing 5% of the work and then sitting around waiting for someone > else to do the other 95% so you can sue them.
Martijn van Oosterhout <kleptog@svana.org> writes:
> This problem has been around for ever yet obviously not everybody runs
> into it all the time like I do. Would patch to fix this be accepted or
> is there a reason why not?
I guess everybody else uses "q" not control-C to get out of less ;-)
I'm not sure I agree with the concept of blocking SIGINT in this context
--- you would not, I trust, propose blocking SIGQUIT or SIGHUP, but then
why is it ok to block SIGINT?
AFAICT it wouldn't fix the problem anyway, as there's still a bug in
less: it doesn't restore the terminal settings on exit.
regards, tom lane
On Sun, Oct 16, 2005 at 02:44:41PM -0400, Tom Lane wrote: > Martijn van Oosterhout <kleptog@svana.org> writes: > > This problem has been around for ever yet obviously not everybody runs > > into it all the time like I do. Would patch to fix this be accepted or > > is there a reason why not? > > I guess everybody else uses "q" not control-C to get out of less ;-) Umm, Ctrl-C doesn't quit less, it aborts the current command. So if it's doing a search or you've hit "F" (follow) or any number of other things then the only way out is to press Ctrl-C. You have to press "q" to quit less like you say. > I'm not sure I agree with the concept of blocking SIGINT in this context > --- you would not, I trust, propose blocking SIGQUIT or SIGHUP, but then > why is it ok to block SIGINT? The system() function blocks -INT and -QUIT, as does the shell when you run a program, so why not? > AFAICT it wouldn't fix the problem anyway, as there's still a bug in > less: it doesn't restore the terminal settings on exit. The point is, less is still running but psql it pulling the keystrokes from under it... When you finally quit less it does restore the settings, but now readline doesn't expect that as it's changed them again... Have a nice day, -- Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/ > Patent. n. Genius is 5% inspiration and 95% perspiration. A patent is a > tool for doing 5% of the work and then sitting around waiting for someone > else to do the other 95% so you can sue them.
Martijn van Oosterhout wrote: >The point is, less is still running but psql it pulling the keystrokes >from under it... When you finally quit less it does restore the >settings, but now readline doesn't expect that as it's changed them >again... > > > Martin, Let's see the patch. I assume it should be fairly small. If we could get it in early in the 8.2 cycle we would have plenty of time to bang on it. In principle this sounds reasonable to me, but psql can be broken quite easily - I know as I've done it a couple of times ;-) cheers andrew
Martjin, > This problem has been around for ever yet obviously not everybody runs > into it all the time like I do. Would patch to fix this be accepted or > is there a reason why not? Actually, I run into it fairly often when I'm being hasty. I'd imagine most Linux-based newbies do as well. -- Josh Berkus Aglio Database Solutions San Francisco
On Sun, Oct 16, 2005 at 04:06:04PM -0400, Andrew Dunstan wrote: > Martin, > > Let's see the patch. I assume it should be fairly small. If we could get > it in early in the 8.2 cycle we would have plenty of time to bang on it. > In principle this sounds reasonable to me, but psql can be broken quite > easily - I know as I've done it a couple of times ;-) Very well, patch attached. It's quite simple actually. However, there seems to be some push back from people suggesting that jumping back to the main loop before the pager has quit is not buggy behaviour. Assuming that a ^C will kill the pager is just folly. Tom asked if we should be blocking SIGQUIT and SIGHUP too. Standard procedure for spawning external interactive processes includes blocking SIGQUIT too (see system() manpage). Logically speaking, when the user sends an interrupt from the keyboard they expect to interrupt the currently active *interaxtive* process. Hence, once psql has spawned the pager, it should ignore such interrupts until control is returned (after pclose). So yes, I would suggest blocking SIGQUIT also, if only to prevent terminal corruption problems. Interactive programs like less trap SIGQUIT to restore the terminal settings on exit, but the exit anyway. SIGHUP is different. It is sent by the system, not the user, indicating the terminal has disconnected. psql is not a daemon expecting to survive that and hence should quit as normal, along with all other processes on that terminal. If this isn't going to make 8.1 anyway I'd probably go for a patch that got rid of the longjmp altogether (if people will accept that), fixing the memory leaks at the same time. At the moment I'm just looking for a concensus that it's a problem to be solved. Have a nice day, -- Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/ > Patent. n. Genius is 5% inspiration and 95% perspiration. A patent is a > tool for doing 5% of the work and then sitting around waiting for someone > else to do the other 95% so you can sue them.
Attachment
On Sun, Oct 16, 2005 at 01:57:13PM -0700, Josh Berkus wrote:
> Martjin,
> > This problem has been around for ever yet obviously not everybody runs
> > into it all the time like I do. Would patch to fix this be accepted or
> > is there a reason why not?
> Actually, I run into it fairly often when I'm being hasty. I'd imagine most
> Linux-based newbies do as well.
I believe many such applications (including less?) are following the
Emacs tradition(?) of having Control-C, or often, Control-G (with
Control-G actually being re-bound as the interrupt character) - mean
interrupt the current operation within the application - not cause
the application to exit. If I wanted to quit, I'd hit 'q'. The times
I do hit Control-C, or Control-G, are when the application I believe
I am using (my keystrokes are interacting with) is performing a
lengthy operation, that I wish to interrupt.
One such operation that has come up with me in less, is when it is
attempting to calculate the line numbers. It becomes non-responsive
during this time, and if I don't care what the line number is, I
interrupt less to tell it 'no - don't calculate line numbers - get
ready for my next command'.
So, I'm with Martijn. It's not right for psql to die from SIGINT
during the execution of another command. SIGINT is a user signal
that can be generated from a keyboard. Psql should not be making
assumptions about what the signal is meant to mean, to the current
application in control of the keyboard.
I agree with Martijn on SIGQUIT (another keyboard signal!) being
ignored, but leaving SIGHUP (not a keyboard signal!) cause psql to
exit. SIGHUP is usually sent to everybody, with everybody paying
attention to it. SIGINT and SIGQUIT are intended for the application
in control of the terminal.
Cheers,
mark
--
mark@mielke.cc / markm@ncf.ca / markm@nortel.com __________________________
. . _ ._ . . .__ . . ._. .__ . . . .__ | Neighbourhood Coder
|\/| |_| |_| |/ |_ |\/| | |_ | |/ |_ |
| | | | | \ | \ |__ . | | .|. |__ |__ | \ |__ | Ottawa, Ontario, Canada
One ring to rule them all, one ring to find them, one ring to bring them all and in the darkness
bindthem...
http://mark.mielke.cc/
I run into this problem sometimes, especially when I realize that the query I've just started is going to run for a very long time and not really provide anything useful. I find that I have to close the shell window to get out of it, and I'm always a bit uncomforatble doing that. -Kevin >>> Martijn van Oosterhout <kleptog@svana.org> >>> At the moment I'm just looking for a concensus that it's a problem to be solved.
On Tue, Oct 18, 2005 at 05:15:20PM -0500, Kevin Grittner wrote: > I run into this problem sometimes, especially when I realize that > the query I've just started is going to run for a very long time and > not really provide anything useful. I find that I have to close the > shell window to get out of it, and I'm always a bit uncomforatble > doing that. Hmm, I'm glad it isn't just me experiencing this on a regular basis. It's possibly also because Debian sets the default pager to less, which makes it happen on every machine I use. The simple patch I posted seems unlikely to make it into 8.1 so I'll add a more comprehensive patch to my list hopefully for 8.2... Have a nice day, -- Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/ > Patent. n. Genius is 5% inspiration and 95% perspiration. A patent is a > tool for doing 5% of the work and then sitting around waiting for someone > else to do the other 95% so you can sue them.
Martijn van Oosterhout wrote: > Very well, patch attached. It's quite simple actually. However, there > seems to be some push back from people suggesting that jumping back to > the main loop before the pager has quit is not buggy behaviour. > Assuming that a ^C will kill the pager is just folly. Making assumptions about what the pager will do upon receipt of SIGINT is folly as well. Setting up SIGINT to be ignored may be the right answer (I don't believe it is -- see below), but if so then it needs to be done properly. If it gets ignored prior to the popen(), then the child will also end up ignoring it by default, because signal disposition is inherited by child processes. If we ignore SIGINT, it should be after the popen(), not before. When the user sends SIGINT, he means to interrupt whatever processing is currently occurring. He expects to regain control of the terminal. If psql is in the process of sending data to the pager, then a SIGINT should cause psql to stop doing so. So I think the right answer here is for psql to handle SIGINT internally by doing a pclose() first (and at this point, it probably should ignore SIGINT altogether), then returning to the main loop (and, of course, cleaning up anything that needs it along the way). If the child hasn't exited then pclose() will block until it has. The end result should be the semantics you want: if psql is in the middle of sending a bunch of rows of output to the pager, this will interrupt that process. If the pager remains running then it will hopefully give the user the ability to scroll through whatever results were sent to it. > Tom asked if we should be blocking SIGQUIT and SIGHUP too. Standard > procedure for spawning external interactive processes includes blocking > SIGQUIT too (see system() manpage). SIGQUIT has a different standard meaning in Unix than SIGINT: it causes the process to drop core. We should not be blocking it -- we should be leaving it alone. The reason is that it's quite possible that the user wants to have psql generate a core file while it's writing output to the pager. > Logically speaking, when the user sends an interrupt from the > keyboard they expect to interrupt the currently active *interaxtive* > process. They expect to interrupt the currently active processing. Not quite the same thing. > Hence, once psql has spawned > the pager, it should ignore such interrupts until control is returned > (after pclose). So yes, I would suggest blocking SIGQUIT also, if only > to prevent terminal corruption problems. Interactive programs like less > trap SIGQUIT to restore the terminal settings on exit, but the exit > anyway. They should be dropping core upon receipt of SIGQUIT. It might be nice if they cleaned up the terminal first, but receipt of a SIGQUIT generally means that the user wants to run the resulting core file through a debugger, and trapping the signal could alter the stack such that the resulting core would be less useful. I'd rather have to clean up the terminal manually than have an unusable core file. -- Kevin Brown kevin@sysexperts.com
On Tue, Oct 18, 2005 at 09:32:25PM -0700, Kevin Brown wrote: > So I think the right answer here is for psql to handle SIGINT > internally by doing a pclose() first (and at this point, it probably > should ignore SIGINT altogether), then returning to the main loop > (and, of course, cleaning up anything that needs it along the way). > If the child hasn't exited then pclose() will block until it has. The > end result should be the semantics you want: if psql is in the middle > of sending a bunch of rows of output to the pager, this will interrupt > that process. If the pager remains running then it will hopefully > give the user the ability to scroll through whatever results were sent > to it. That's what I meant by "more comprehensive patch". Basically, the longjmp has to go because it leaks memory (and file descriptors) and doesn't allow you to control things at the right level. My little patch basically set the signal handler *after* the popen so everything works as expected. My plan is to have the interrupt handler set a flag "control_c_pressed" and check it at strategic points. Then memory can be deallocated and returned properly. It's a lot more invasive and the corners cases will be "interesting". Your point about SIGQUIT is valid. I didn't include it in my patch since I wasn't sure what the expected behaviour would be. If I got core files everytime I pressed SIGQUIT, I'd have a lot of core files scattered around my disk; one of the reasons I disable core files by default. OTOH, if I wanted to trap psql, I'd run it under a debugger or attach one which would catch SIGQUIT even if the program ignores it. Anyway, thanks for the response, hopefully we can get this sorted out in a later release... -- Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/ > Patent. n. Genius is 5% inspiration and 95% perspiration. A patent is a > tool for doing 5% of the work and then sitting around waiting for someone > else to do the other 95% so you can sue them.
Martijn van Oosterhout wrote: > On Tue, Oct 18, 2005 at 09:32:25PM -0700, Kevin Brown wrote: > > So I think the right answer here is for psql to handle SIGINT > > internally by doing a pclose() first (and at this point, it probably > > should ignore SIGINT altogether), then returning to the main loop > > (and, of course, cleaning up anything that needs it along the way). > > If the child hasn't exited then pclose() will block until it has. The > > end result should be the semantics you want: if psql is in the middle > > of sending a bunch of rows of output to the pager, this will interrupt > > that process. If the pager remains running then it will hopefully > > give the user the ability to scroll through whatever results were sent > > to it. > > That's what I meant by "more comprehensive patch". Basically, the > longjmp has to go because it leaks memory (and file descriptors) and > doesn't allow you to control things at the right level. My little patch > basically set the signal handler *after* the popen so everything works > as expected. It does? It looked to me like it was setting the signal handler to SIG_IGN before doing the popen(), and resetting it to the internal signal handler after doing the pclose(). Am I examining the wrong patch? > My plan is to have the interrupt handler set a flag "control_c_pressed" > and check it at strategic points. Then memory can be deallocated and > returned properly. It's a lot more invasive and the corners cases will > be "interesting". If I were to take that approach, I'd do the check immediately after writing to the pipe for sure, and possibly after reading from the database handle. I'd have to look at the code to see what's going on and thus where the best places to stick the checks are. But in general, you want to put a check anywhere within the loop that is immediately after a blocking operation such as a read or write. However, that said... The semantics for handling SIGINT here should be pretty much the same as they are when there's no pager at all (which I know works because I can interrupt the output anytime and get the psql prompt back). In fact, this suggests that the proper course of action would be to store the pipe's file descriptor someplace accessible to the normal SIGINT handler, and have the SIGINT handler check its value upon receipt of SIGINT. If the value is nonzero then pclose() it and then proceed as before (and zero the descriptor after any pclose() so that the handler always does the right thing). If there's any additional memory that has to be freed, then do that too, of course. That'll take care of the descriptor leak and it'll get you the right semantics as a nice bonus. > Your point about SIGQUIT is valid. I didn't include it in my patch > since I wasn't sure what the expected behaviour would be. If I got core > files everytime I pressed SIGQUIT, I'd have a lot of core files > scattered around my disk; one of the reasons I disable core files by > default. If you're hitting SIGQUIT a lot then perhaps you need to assign it to a different key. :-) Seriously, you shouldn't be using SIGQUIT unless you want a core file. Otherwise, use SIGINT. If the application is unresponsive to SIGINT, my normal course of action is to suspend it via job control signals (i.e., send SIGTSTP) and then send it a SIGTERM via the kill command. If that doesn't do it, then it gets a SIGKILL. And if it doesn't stop upon receipt of SIGTSTP I kill it externally and complain to its developers (assuming it isn't hung in some uninterruptible sleep in the kernel or something). > OTOH, if I wanted to trap psql, I'd run it under a debugger or attach > one which would catch SIGQUIT even if the program ignores it. Suppose it's hung on a really hard to reproduce condition? Point being that you don't necessarily know ahead of time that you're going to want a core file. Otherwise SIGQUIT wouldn't have the default semantics it has, and it wouldn't have a terminal key associated with it. The guys that designed this stuff knew what they were doing. -- Kevin Brown kevin@sysexperts.com
On 2005-10-19, Kevin Brown <kevin@sysexperts.com> wrote: > Making assumptions about what the pager will do upon receipt of SIGINT > is folly as well. > > Setting up SIGINT to be ignored may be the right answer (I don't > believe it is -- see below), but if so then it needs to be done > properly. If it gets ignored prior to the popen(), then the child > will also end up ignoring it by default, because signal disposition is > inherited by child processes. If we ignore SIGINT, it should be after > the popen(), not before. I do not believe it is possible to do the signal disposition correctly and still use popen() to run the pager. (You would need to reimplement popen using raw syscalls.) > So I think the right answer here is for psql to handle SIGINT > internally by doing a pclose() first The chances that psql can do this safely approach zero. pclose() is not a signal-safe function, so it can only be called from a signal handler if you _know_ that the signal did not interrupt any non-signal-safe function. (Nor can the signal handler longjmp out in such a case, unless the code is never again going to call any unsafe function.) -- Andrew, Supernews http://www.supernews.com - individual and corporate NNTP services
On Wed, Oct 19, 2005 at 04:24:21AM -0700, Kevin Brown wrote: > It does? It looked to me like it was setting the signal handler to > SIG_IGN before doing the popen(), and resetting it to the internal > signal handler after doing the pclose(). Oops, you're right. It works because less sets it's own signal handler, as does more. Sure, the status is inherited but I doubt there's a pager out there that doesn't set its own signal handlers. Still, best do it properly. > If I were to take that approach, I'd do the check immediately after > writing to the pipe for sure, and possibly after reading from the > database handle. I'd have to look at the code to see what's going on > and thus where the best places to stick the checks are. But in > general, you want to put a check anywhere within the loop that is > immediately after a blocking operation such as a read or write. > However, that said... Well, the pager is only run after all the data has been collected from the server so none of this is an issue while the query is being processed. Send the cancel request and toss the data. Once we start the pager just check after each line is printed. > The semantics for handling SIGINT here should be pretty much the same > as they are when there's no pager at all (which I know works because I <snip> You can't do a pclose in a signal handler, it's not one of the "reeentrant safe" functions and could lead to deadlocks. The signal manpage documents the ones you can use. Just set a flag. Setting the descriptor to NULL is worse because then we have check before every output function. fprintf(NULL, ...) will segfault on most architechtures I wager. Also, the signal handler can't free the memory, there's way too many arrays. A flag allows the output functions to exit sanely. > If you're hitting SIGQUIT a lot then perhaps you need to assign it to > a different key. :-) I use SIGQUIT to get out of the situation you get currently when you press Ctrl-C to interrupt less. Somehow all the "q"s go to psql and all the Ctrl-Ds to less. SIGQUIT just kills them both from where I can clean the terminal up. So I will press SIGQUIT less once we fix up psql to not do stupid things on SIGINT. :) Have a nice day, -- Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/ > Patent. n. Genius is 5% inspiration and 95% perspiration. A patent is a > tool for doing 5% of the work and then sitting around waiting for someone > else to do the other 95% so you can sue them.
Martijn van Oosterhout wrote: > You can't do a pclose in a signal handler, it's not one of the > "reeentrant safe" functions and could lead to deadlocks. The signal > manpage documents the ones you can use. Just set a flag. Setting the > descriptor to NULL is worse because then we have check before every > output function. fprintf(NULL, ...) will segfault on most > architechtures I wager. Yeah, I was thinking that you'd do the check for the flag and invoke a cleanup handler after the write() to the output file descriptor. It's not clear that you'd need to do the check anyplace else. It's been a while since I've messed with this stuff, but if I recall correctly, the write() will return immediately after receipt of a signal, and will indicate how much was actually written. So receipt of a SIGINT should wind up being handled in a reasonably timely fashion. Additionally the normal SIGINT signal handler (the one that gets invoked when the pager is turned off) can be called from the cleanup handler in order to maintain the proper semantics. -- Kevin Brown kevin@sysexperts.com
On Thu, Oct 20, 2005 at 03:42:10PM -0700, Kevin Brown wrote:
> Martijn van Oosterhout wrote:
> > You can't do a pclose in a signal handler, it's not one of the
> > "reeentrant safe" functions and could lead to deadlocks. The signal
> > manpage documents the ones you can use. Just set a flag. Setting the
> > descriptor to NULL is worse because then we have check before every
> > output function. fprintf(NULL, ...) will segfault on most
> > architechtures I wager.
> Yeah, I was thinking that you'd do the check for the flag and invoke a
> cleanup handler after the write() to the output file descriptor. It's
> not clear that you'd need to do the check anyplace else. It's been a
> while since I've messed with this stuff, but if I recall correctly,
> the write() will return immediately after receipt of a signal, and
> will indicate how much was actually written. So receipt of a SIGINT
> should wind up being handled in a reasonably timely fashion.
> Additionally the normal SIGINT signal handler (the one that gets
> invoked when the pager is turned off) can be called from the cleanup
> handler in order to maintain the proper semantics.
I disagree that psql should make *any* assumptions about what SIGINT
means to the child process. Consider less again, and Control-C used
to abort a search. You are suggesting that Control-C should not only
abort the search, but should also cut off the input from less. Less
won't die. Less will just see a terminated input stream. What has been
gained from this? Is this intuitive behaviour?
If the pager does die in response to SIGINT, the write() will fail with
SIGPIPE. Completely clean, without any need for psql to pay attention
to SIGINT.
I think the only reasonable behaviour is to ignore SIGINT within the
parent, until the child exits. I don't see why other behaviours are
even being considered. To me, it points at a misunderstanding of the
problem.
Cheers,
mark
--
mark@mielke.cc / markm@ncf.ca / markm@nortel.com __________________________
. . _ ._ . . .__ . . ._. .__ . . . .__ | Neighbourhood Coder
|\/| |_| |_| |/ |_ |\/| | |_ | |/ |_ |
| | | | | \ | \ |__ . | | .|. |__ |__ | \ |__ | Ottawa, Ontario, Canada
One ring to rule them all, one ring to find them, one ring to bring them all and in the darkness
bindthem...
http://mark.mielke.cc/
On Thu, Oct 20, 2005 at 08:11:14PM -0400, mark@mark.mielke.cc wrote: > I disagree that psql should make *any* assumptions about what SIGINT > means to the child process. Consider less again, and Control-C used > to abort a search. You are suggesting that Control-C should not only > abort the search, but should also cut off the input from less. Less > won't die. Less will just see a terminated input stream. What has been > gained from this? Is this intuitive behaviour? I must say I agree with the idea that Ctrl-C shouldn't stop the stream from psql, but I'm willing to let it slide because a lot of other programs work this way. I imagine asking it to be configurable will meet even more resistance. > I think the only reasonable behaviour is to ignore SIGINT within the > parent, until the child exits. I don't see why other behaviours are > even being considered. To me, it points at a misunderstanding of the > problem. I've been playing with a version of psql which on Ctrl-C doesn't longjmp() but politely frees everything, waits for the pager and then back to the main loop with the message "Interrupted". But now we have another behaviour change: How to abort the gets() when you don't have readline? Doing it with a flag is a lot more susceptable to subtle behaviour changes, but I'll see if I can make it work. Have a nice day, -- Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/ > Patent. n. Genius is 5% inspiration and 95% perspiration. A patent is a > tool for doing 5% of the work and then sitting around waiting for someone > else to do the other 95% so you can sue them.
On Fri, Oct 21, 2005 at 01:53:32PM +0200, Martijn van Oosterhout wrote:
> On Thu, Oct 20, 2005 at 08:11:14PM -0400, mark@mark.mielke.cc wrote:
> > I disagree that psql should make *any* assumptions about what SIGINT
> > means to the child process. Consider less again, and Control-C used
> > to abort a search. You are suggesting that Control-C should not only
> > abort the search, but should also cut off the input from less. Less
> > won't die. Less will just see a terminated input stream. What has been
> > gained from this? Is this intuitive behaviour?
> I must say I agree with the idea that Ctrl-C shouldn't stop the stream
> from psql, but I'm willing to let it slide because a lot of other
> programs work this way. I imagine asking it to be configurable will
> meet even more resistance.
Which other ones? I can't think of one. The ones that don't handle
SIGINT, or that are not designed for this scenario certainly don't count -
as that is how psql works right now without the patch. Of the remaining
programs that are designed to work with an application such as less, which
ones purposefully close the input stream to less, and continue running?
I think 0.
> > I think the only reasonable behaviour is to ignore SIGINT within the
> > parent, until the child exits. I don't see why other behaviours are
> > even being considered. To me, it points at a misunderstanding of the
> > problem.
> I've been playing with a version of psql which on Ctrl-C doesn't
> longjmp() but politely frees everything, waits for the pager and then
> back to the main loop with the message "Interrupted". But now we have
> another behaviour change: How to abort the gets() when you don't have
> readline?
> Doing it with a flag is a lot more susceptable to subtle behaviour
> changes, but I'll see if I can make it work.
SIG_IGN is the easiest, and the most effective. Certainly the easiest
to maintain, and the least intrusive to the core.
I don't understand.
Cheers,
mark
--
mark@mielke.cc / markm@ncf.ca / markm@nortel.com __________________________
. . _ ._ . . .__ . . ._. .__ . . . .__ | Neighbourhood Coder
|\/| |_| |_| |/ |_ |\/| | |_ | |/ |_ |
| | | | | \ | \ |__ . | | .|. |__ |__ | \ |__ | Ottawa, Ontario, Canada
One ring to rule them all, one ring to find them, one ring to bring them all and in the darkness
bindthem...
http://mark.mielke.cc/
On Fri, Oct 21, 2005 at 08:48:31AM -0400, mark@mark.mielke.cc wrote: > Which other ones? I can't think of one. The ones that don't handle > SIGINT, or that are not designed for this scenario certainly don't count - > as that is how psql works right now without the patch. Of the remaining > programs that are designed to work with an application such as less, which > ones purposefully close the input stream to less, and continue running? The one run into all the time is: rgrep "something" . | less If you do a search and it doesn't find anything, it'll keep reading in data until it finds it or reaches the end. Hit Ctrl-C to abort the search and you kill the rgrep too. Now, arguably rgrep is not "designed to work with an application such as less" so doesn't meet your criteria. So the argument is about whether psql is. Is the pager an integral part of the output mechanism of psql, or just a bolt-on like a shell pipe? I suspect you can argue either way, which is why I'm trying to find a way to have it both ways. > SIG_IGN is the easiest, and the most effective. Certainly the easiest > to maintain, and the least intrusive to the core. Yeah, but doesn't fix the memory leaks and other random leakage and breakage from the longjmp(). But I'm getting there... Have a nice day, -- Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/ > Patent. n. Genius is 5% inspiration and 95% perspiration. A patent is a > tool for doing 5% of the work and then sitting around waiting for someone > else to do the other 95% so you can sue them.
mark@mark.mielke.cc wrote: > On Thu, Oct 20, 2005 at 03:42:10PM -0700, Kevin Brown wrote: > > Martijn van Oosterhout wrote: > > > You can't do a pclose in a signal handler, it's not one of the > > > "reeentrant safe" functions and could lead to deadlocks. The signal > > > manpage documents the ones you can use. Just set a flag. Setting the > > > descriptor to NULL is worse because then we have check before every > > > output function. fprintf(NULL, ...) will segfault on most > > > architechtures I wager. > > Yeah, I was thinking that you'd do the check for the flag and invoke a > > cleanup handler after the write() to the output file descriptor. It's > > not clear that you'd need to do the check anyplace else. It's been a > > while since I've messed with this stuff, but if I recall correctly, > > the write() will return immediately after receipt of a signal, and > > will indicate how much was actually written. So receipt of a SIGINT > > should wind up being handled in a reasonably timely fashion. > > > Additionally the normal SIGINT signal handler (the one that gets > > invoked when the pager is turned off) can be called from the cleanup > > handler in order to maintain the proper semantics. > > I disagree that psql should make *any* assumptions about what SIGINT > means to the child process. Consider less again, and Control-C used > to abort a search. You are suggesting that Control-C should not only > abort the search, but should also cut off the input from less. Less > won't die. Less will just see a terminated input stream. What has been > gained from this? Is this intuitive behaviour? It's behaviour that's consistent with every other pipeline application set I've ever seen. The only difference between those and this situation is that for a standard pipeline set, SIGINT will kill all the processes in the pipeline. Less explicitly ignores (or traps) SIGINT, so the effect of generating a SIGINT where less is at the end of the pipeline is to kill everything that precedes it, and less will then show what results it received. And obviously, that implies that the pipeline gets closed. If psql does not close the pipeline right then and there, then its behaviour will obviously be different from what the user likely expects, based on other pipelined uses of less. After all, if they wanted to see the entire result set then they wouldn't have sent SIGINT, would they? > If the pager does die in response to SIGINT, the write() will fail with > SIGPIPE. Completely clean, without any need for psql to pay attention > to SIGINT. We're not talking about the semantics of the pager, we're talking about the semantics of psql. You said it yourself: psql can't make any assumptions about what SIGINT means to the child process. So it has to consider what to do if the child does *not* die in response to SIGINT. What are the proper semantics for psql + the child in that situation? Well, I think it's clear that having psql ignore SIGINT in that situation is *not* correct, because it implies that whether or not SIGINT causes processing to stop (as the user would expect) depends entirely on the child. More specifically, it would depend on the child either dying or explicitly closing its stdin upon receipt of SIGINT. The bottom line is that, at least in my opinion, the semantics of SIGINT as regards psql should be the same whether or not there's a pager involved, with one crucial difference: if there's a pager involved, psql should wait for it to terminate before showing the prompt. But otherwise, the semantics should be identical, because they are what the user expects. > I think the only reasonable behaviour is to ignore SIGINT within the > parent, until the child exits. I don't see why other behaviours are > even being considered. To me, it points at a misunderstanding of the > problem. Not at all. When you send SIGINT to a process, you want that process to stop doing whatever it's doing and return control to you. That's what it means, and that's what it's for. If we ignore SIGINT then obviously we will *not* be heeding the wishes of the user who sends SIGINT, and that is not likely what the user expects. -- Kevin Brown kevin@sysexperts.com
Andrew - Supernews wrote: > On 2005-10-19, Kevin Brown <kevin@sysexperts.com> wrote: > > Making assumptions about what the pager will do upon receipt of SIGINT > > is folly as well. > > > > Setting up SIGINT to be ignored may be the right answer (I don't > > believe it is -- see below), but if so then it needs to be done > > properly. If it gets ignored prior to the popen(), then the child > > will also end up ignoring it by default, because signal disposition is > > inherited by child processes. If we ignore SIGINT, it should be after > > the popen(), not before. > > I do not believe it is possible to do the signal disposition correctly > and still use popen() to run the pager. (You would need to reimplement > popen using raw syscalls.) I'm not sure I see why this is so. popen() just creates the pipeline, fork()s, closes the proper file descriptor (depending on whether it's in the parent or the child and whether the pipe was open for read or write) and then exec()s in the child. In the parent, it returns control after the fork() and file descriptor cleanup. So the parent can set up its own internal signal disposition immediately after popen() returns. This sequence of events is exactly what we'd end up doing if we did everything ourselves using raw syscalls, save for the use of stdio instead of direct syscalls for the file operations. > > So I think the right answer here is for psql to handle SIGINT > > internally by doing a pclose() first > > The chances that psql can do this safely approach zero. pclose() is not a > signal-safe function, so it can only be called from a signal handler if > you _know_ that the signal did not interrupt any non-signal-safe function. > (Nor can the signal handler longjmp out in such a case, unless the code is > never again going to call any unsafe function.) I agree. I guess I need to be a little more explicit about what I envision here. There would be two possible signal handlers. The first is the one we have now, which cleans up various things upon receipt of SIGINT. The second simply sets a flag that says that SIGINT has been received. The signal handler that gets assigned to SIGINT depends on whether or not a pager is going to be used. If it's not, then we point it to the first signal handler. If it is, then we point it to the second, and clear the flag. When a pager is being used, we check for the flag immediately after doing a write()/fwrite() to the pipe. If it's set, we pclose(), clear the flag, and then manually invoke the non-pager signal handler. SIGINT should cause the write() to return immediately, possibly with EINTR. Make sense? -- Kevin Brown kevin@sysexperts.com
If you send a recent version of vim a CONTROL-C, and you're just sitting there at a prompt, it gives you a hint: Type :quit<Enter> to exit Vim Any reason not to just trap the CONTROL-C in psql when paging and offer a hint? Especially since we don't really know that the user really wanted to type CONTROL-C instead of q for quit. I know that I have always meant to type q and was just distracted whenever I've typed CONTROL-C in the pager, and so passing the CONTROL-C on to less is not actually "heeding my wishes", it is instead giving me enough rope to shoot myself in the foot. Sean (and mixed metaphors really make by hair boil) [big snippage] > Not at all. When you send SIGINT to a process, you want that process > to stop doing whatever it's doing and return control to you. That's > what it means, and that's what it's for. If we ignore SIGINT then > obviously we will *not* be heeding the wishes of the user who sends > SIGINT, and that is not likely what the user expects. > > > -- > Kevin Brown kevin@sysexperts.com > > ---------------------------(end of broadcast)--------------------------- > TIP 6: explain analyze is your friend > >
I failed to mention that I also tend to type CONTROL-C when I forget that putty acts like an xterm, and doesn't need CONTROL-C to copy text into the clipboard. In that case, aborting the pager, and leaving the terminal trashed requiring me to exit psql, stty sane, and start up psql again is really not "heeding my wishes", since what I was trying to do was copy some text from the screen in a moment of brain-deadness. Sean
Sean Utt wrote: > > If you send a recent version of vim a CONTROL-C, and you're just sitting > there at a prompt, it gives you a hint: > > Type :quit<Enter> to exit Vim > > Any reason not to just trap the CONTROL-C in psql when paging and offer a > hint? Especially since we don't really know that the user really wanted to > type CONTROL-C instead of q for quit. I know that I have always meant to > type q and was just distracted whenever I've typed CONTROL-C in the pager, > and so passing the CONTROL-C on to less is not actually "heeding my wishes", > it is instead giving me enough rope to shoot myself in the foot. It won't work properly that way. SIGINT gets sent to all the members of the process group, not just the child. Psql isn't responsible for sending ctrl-c through to the child. When you're in an editor such as vi that makes use of the terminal, the editor itself is likely the only program that is doing anything. Its parent is doing a wait() on the editor. The parent in that instance can ignore SIGINT because it's not involved at all at that point. That's not the case here. Psql and the pager are really two cooperating parts of the same task. They just happen to be running in two different process spaces. Because they're both cooperatively active at the same time, they both need to handle SIGINT, because when the user invokes SIGINT, he intends for the overall task to return some kind of control to him. For psql, which is gathering data and sending it to the pager, that means that it needs to stop doing so and wait for the pager to finish. For the pager, it means at a minimum that it needs to display what it has so far and give interactive control to the user (it may or may not attempt to continue to read what's being sent to it). Some pagers (like "more") will just exit. -- Kevin Brown kevin@sysexperts.com
On Fri, Oct 21, 2005 at 05:28:49PM -0700, Kevin Brown wrote: > When a pager is being used, we check for the flag immediately after > doing a write()/fwrite() to the pipe. If it's set, we pclose(), clear > the flag, and then manually invoke the non-pager signal handler. > SIGINT should cause the write() to return immediately, possibly with > EINTR. You wish. PostgreSQL uses BSD signal semantics, which means system calls get restarted. Neither read nor write will return when user presses Ctrl-C... Hence my question about POSIX signals... It doesn't matter though, if write blocks there's no processing happening anyway and we can check the flag after write returns success (pager accepted more data) or failure (pager died). Have a nice day, -- Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/ > Patent. n. Genius is 5% inspiration and 95% perspiration. A patent is a > tool for doing 5% of the work and then sitting around waiting for someone > else to do the other 95% so you can sue them.
On Sun, Oct 16, 2005 at 03:25:49PM +0200, Martijn van Oosterhout wrote: > This behaviour has been around so long that I've gotten used to it but > I've always considered it a bug. Yet it has never been fixed so I'm > going to ask if anybody else has issues with this behaviour. I've posted a patch to -patches which fixes all the memory leak and file descriptor leak issues and well as making psql handle ^C more gracefully in general. It doesn't address this particular issue though, thats for another time. If people want to try it out, please do. it will appear in the archives soon, otherwise you can download it directly from: http://svana.org/kleptog/pgsql/psql-ctrlc.patch Have a nice day, -- Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/ > Patent. n. Genius is 5% inspiration and 95% perspiration. A patent is a > tool for doing 5% of the work and then sitting around waiting for someone > else to do the other 95% so you can sue them.
On Sat, Oct 22, 2005 at 09:46:32PM +0200, Martijn van Oosterhout wrote: > I've posted a patch to -patches which fixes all the memory leak and > file descriptor leak issues and well as making psql handle ^C more > gracefully in general. It doesn't address this particular issue though, > thats for another time. Clarification, it does fix the screen corruption you currently get when you press Ctrl-C while running less. what I meant it that it preserves current semantics w.r.t. aborting output if you press ctrl-c even though the pager is still running. Have a nice day, -- Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/ > Patent. n. Genius is 5% inspiration and 95% perspiration. A patent is a > tool for doing 5% of the work and then sitting around waiting for someone > else to do the other 95% so you can sue them.
> It won't work properly that way. SIGINT gets sent to all the members > of the process group, not just the child. Psql isn't responsible for > sending ctrl-c through to the child. Except that if I am in less, and I do CONTROL-C, it doesn't do anything at all. It doesn't exit. If I send a kill -2 to the process, it doesn't exit. less ignores SIGINT completely. So, whether or not they are working together in cooperation, the result we have been seeing when typing CONTROL-C is completely under the control of psql. I do realize that the problem is one of when and how to cancel the query if CONTROL-C is pressed. If the query is still building and the output hasn't, or won't be, sent to the pager, cancel the query. If we are in the pager, don't respond to CONTROL-C, and instead output a helpful hint telling people to use q to quit, which will do what they really wanted anyway. In theory, we already deal gracefully with q being pressed in the pager. Sean
On Fri, Oct 21, 2005 at 05:06:45PM -0700, Kevin Brown wrote:
> > I disagree that psql should make *any* assumptions about what SIGINT
> > means to the child process. Consider less again, and Control-C used
> > to abort a search. You are suggesting that Control-C should not only
> > abort the search, but should also cut off the input from less. Less
> > won't die. Less will just see a terminated input stream. What has been
> > gained from this? Is this intuitive behaviour?
> It's behaviour that's consistent with every other pipeline application
> set I've ever seen.
I think you are looking at 'every other pipeline application that does
nothing', and treating them as role models. :-)
I'm completely writing off applications that don't handle SIGINT in some
way, as psql already does this, and wouldn't need *any* change to work
this way. Psql already works the way you are saying every other application
you know of does. So, I'm not seeing your point.
If psql is to change, it is because the current behaviour is impractical,
and not for any other reason. What is the practical behaviour?
I think there are other, better ways, to know that the pager is
dead. One is SIGPIPE/EPIPE. A write to the pipe to the pager will
error with EPIPE, and throw a SIGPIPE signal. The parent will also
be notified with SIGCHILD when the child exits. In this way, we
have two ways of knowing that the child is dead, without preventing
SIGINT from being handled fully by the child, in a pager such as
less, that defines SIGINT to mean something *different* than die
or abort reading the input stream.
The case that this would be a problem for me is - psql is outputting
pages and pages of data. The pipe buffer becomes full (8192 bytes on
Linux?), and psql blocks. The rest of the data is available, however,
it has not yet been transferred to less. The user does some operation
in less that might take some time, and then hits control-C. Less
continues, but under your scheme, is faced with a terminated input
stream. I'm not seeing why this is practical. I don't see the benefit
of this.
Other than the argument that other applications don't handle Control-C
at all, and you think this is compatible with their behaviour, do you
have any practical arguments for doing it this way? How is it working
better than it does today? Why won't we have the same situation of
less and psql both waiting for STDIN from the user exist after your
suggestion to make psql close off the input stream sooner?
I might just be really dense. But I want you to show me where I'm wrong
before I give in. :-)
Cheers,
mark
--
mark@mielke.cc / markm@ncf.ca / markm@nortel.com __________________________
. . _ ._ . . .__ . . ._. .__ . . . .__ | Neighbourhood Coder
|\/| |_| |_| |/ |_ |\/| | |_ | |/ |_ |
| | | | | \ | \ |__ . | | .|. |__ |__ | \ |__ | Ottawa, Ontario, Canada
One ring to rule them all, one ring to find them, one ring to bring them all and in the darkness
bindthem...
http://mark.mielke.cc/
On Sat, Oct 22, 2005 at 02:48:53PM -0700, Sean Utt wrote: > Except that if I am in less, and I do CONTROL-C, it doesn't do anything at > all. > > It doesn't exit. > > If I send a kill -2 to the process, it doesn't exit. less ignores SIGINT > completely. Not quite, It interprets it as "abort command". Obviously if you're not doing anything at the time it appears to do nothing. Start a search (/) or turn on follow mode (F) and you'll see you can (or indeed in the latter case, must) use Ctrl-C to get back to the command prompt. Indeed, in follow mode it says: Waiting for data... (interrupt to abort) > If we are in the pager, don't respond to CONTROL-C, and instead output a > helpful hint telling people to use q to quit, which will do what they > really wanted anyway. That's wrong too, who says q exits the pager? Not to mention the message screws the screen also, needing a ^L to redraw the screen. Best do nothing at all (ie ignore). If the user doesn't know how to use less, then they shouldn't use it as pager. Besides, less comes with online help (press h), with how to quit being the first thing you read. > In theory, we already deal gracefully with q being pressed in the pager. Yes, by not reading stdin. Ergo, we should also not respond to ^C either. Have a nice day, -- Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/ > Patent. n. Genius is 5% inspiration and 95% perspiration. A patent is a > tool for doing 5% of the work and then sitting around waiting for someone > else to do the other 95% so you can sue them.
This has been saved for the 8.2 release: http://momjian.postgresql.org/cgi-bin/pgpatches_hold --------------------------------------------------------------------------- Martijn van Oosterhout wrote: -- Start of PGP signed section. > On Sun, Oct 16, 2005 at 04:06:04PM -0400, Andrew Dunstan wrote: > > Martin, > > > > Let's see the patch. I assume it should be fairly small. If we could get > > it in early in the 8.2 cycle we would have plenty of time to bang on it. > > In principle this sounds reasonable to me, but psql can be broken quite > > easily - I know as I've done it a couple of times ;-) > > Very well, patch attached. It's quite simple actually. However, there > seems to be some push back from people suggesting that jumping back to > the main loop before the pager has quit is not buggy behaviour. > Assuming that a ^C will kill the pager is just folly. > > Tom asked if we should be blocking SIGQUIT and SIGHUP too. Standard > procedure for spawning external interactive processes includes blocking > SIGQUIT too (see system() manpage). Logically speaking, when the user > sends an interrupt from the keyboard they expect to interrupt the > currently active *interaxtive* process. Hence, once psql has spawned > the pager, it should ignore such interrupts until control is returned > (after pclose). So yes, I would suggest blocking SIGQUIT also, if only > to prevent terminal corruption problems. Interactive programs like less > trap SIGQUIT to restore the terminal settings on exit, but the exit > anyway. > > SIGHUP is different. It is sent by the system, not the user, indicating > the terminal has disconnected. psql is not a daemon expecting to > survive that and hence should quit as normal, along with all other > processes on that terminal. > > If this isn't going to make 8.1 anyway I'd probably go for a patch that > got rid of the longjmp altogether (if people will accept that), fixing > the memory leaks at the same time. At the moment I'm just looking for a > concensus that it's a problem to be solved. > > Have a nice day, > -- > Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/ > > Patent. n. Genius is 5% inspiration and 95% perspiration. A patent is a > > tool for doing 5% of the work and then sitting around waiting for someone > > else to do the other 95% so you can sue them. [ Attachment, skipping... ] -- End of PGP section, PGP failed! -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001+ If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania19073
On Mon, Oct 24, 2005 at 08:20:07AM -0400, Bruce Momjian wrote: > > This has been saved for the 8.2 release: > > http://momjian.postgresql.org/cgi-bin/pgpatches_hold Is this official "blessing" for the idea that psql should ignore SIGINT while the pager is running? Or does this mean the idea will be revisited again later? If the former, I'll rework it into my psql fixing patch on -patches, since this one as posted is not 100% correct (as someone pointed out) although the chance that someone will notice is about nil. Have a nice day, -- Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/ > Patent. n. Genius is 5% inspiration and 95% perspiration. A patent is a > tool for doing 5% of the work and then sitting around waiting for someone > else to do the other 95% so you can sue them.
Martijn van Oosterhout wrote: >On Mon, Oct 24, 2005 at 08:20:07AM -0400, Bruce Momjian wrote: > > >>This has been saved for the 8.2 release: >> >> http://momjian.postgresql.org/cgi-bin/pgpatches_hold >> >> > >Is this official "blessing" for the idea that psql should ignore SIGINT >while the pager is running? Or does this mean the idea will be >revisited again later? > > > > AFAIK it's Bruce's way of not losing track of the patch, so more a case of "revisit later" than "officially blessed". By all means submit a revised patch if you improve on it in the meantime. cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> Martijn van Oosterhout wrote:
>> On Mon, Oct 24, 2005 at 08:20:07AM -0400, Bruce Momjian wrote:
>>> This has been saved for the 8.2 release:
>> Is this official "blessing" for the idea
> AFAIK it's Bruce's way of not losing track of the patch,
Exactly. "Held for future review" would be a more accurate reading.
regards, tom lane
Modified version of this patch applied by Tom. --------------------------------------------------------------------------- Martijn van Oosterhout wrote: -- Start of PGP signed section. > On Sun, Oct 16, 2005 at 04:06:04PM -0400, Andrew Dunstan wrote: > > Martin, > > > > Let's see the patch. I assume it should be fairly small. If we could get > > it in early in the 8.2 cycle we would have plenty of time to bang on it. > > In principle this sounds reasonable to me, but psql can be broken quite > > easily - I know as I've done it a couple of times ;-) > > Very well, patch attached. It's quite simple actually. However, there > seems to be some push back from people suggesting that jumping back to > the main loop before the pager has quit is not buggy behaviour. > Assuming that a ^C will kill the pager is just folly. > > Tom asked if we should be blocking SIGQUIT and SIGHUP too. Standard > procedure for spawning external interactive processes includes blocking > SIGQUIT too (see system() manpage). Logically speaking, when the user > sends an interrupt from the keyboard they expect to interrupt the > currently active *interaxtive* process. Hence, once psql has spawned > the pager, it should ignore such interrupts until control is returned > (after pclose). So yes, I would suggest blocking SIGQUIT also, if only > to prevent terminal corruption problems. Interactive programs like less > trap SIGQUIT to restore the terminal settings on exit, but the exit > anyway. > > SIGHUP is different. It is sent by the system, not the user, indicating > the terminal has disconnected. psql is not a daemon expecting to > survive that and hence should quit as normal, along with all other > processes on that terminal. > > If this isn't going to make 8.1 anyway I'd probably go for a patch that > got rid of the longjmp altogether (if people will accept that), fixing > the memory leaks at the same time. At the moment I'm just looking for a > concensus that it's a problem to be solved. > > Have a nice day, > -- > Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/ > > Patent. n. Genius is 5% inspiration and 95% perspiration. A patent is a > > tool for doing 5% of the work and then sitting around waiting for someone > > else to do the other 95% so you can sue them. [ Attachment, skipping... ] -- End of PGP section, PGP failed! -- Bruce Momjian http://candle.pha.pa.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +