Workaround so only one CTRL-C is required for a new prompt in --gui=qt

[siyuz]

I have a habit of using CTRL-C to clear lines in a terminal (and MATLAB), so it is really annoying to have to press it twice in ipython.

This is a kludgy, posix only workaround, and I only tested it for Python 2.7 on Linux and Windows.

[minrk]

why the sleep? Can it work with no delay?

[minrk]

Also, move any stdlib imports to the top of the file.

[siyuz]

It is unreliable without sleep because sometimes the signal comes through before it can exit the ctypes callback. Though I've tested it with sleeps as short as .01 sec and it worked fine.

[ellisonbg]

I am -1 on this. Adding threads into the mix is just asking for problems...in general with try to avoid threads at all possible cost and I I don't view this semi-mis-feature as warranting it.

[ellisonbg]

@minrk how do you feel about using threads here. @fperez can you have a look at this? I am -1 on the using threads like this but don't want to close this until other's weight in. Thanks.

[minrk]

Using threads does worry me a little bit, since I don't really understand it, but for this Qt stuff, I'm inclined to let people with proposed fixes make them, since this is not an area of the code that has any relevance to me.

[siyuz]

I've switched to python Timers though it doesn't really address your concern since Timer is a subclass of Thread. Qt timers won't work in this case because the eventloop is disabled after the first Ctrl-C.

[minrk]

what would happen if you did join here - I've never understood how this particular bit works, where does the code need to be for this to be properly handled?

[siyuz]

The way ipython seem to integrate with qt eventloop is by registering a callback that gets executed periodically when Python is waiting for input. The callback runs the qt eventloop for 50 ms so all the mouse/windowing stuff gets handled. The problem is you can't raise exceptions in this callback, so CTRL-C won't work at all.

The original author got around this by catching the SIGINT, and the disabling the callback (which means the next CTRL-C will be caught by python proper and not the callback).

I tried to get around that by making it send a ctrl-c to itself after the first ctrl-c, but this SIGINT has to arrive after it exits the inputhook callback, hence the need for threads/timers.

[minrk]

Assuming that the join I mentioned above is not appropriate, let's keep track of the timer instance, and cancel (Timer.cancel()) the previous one before starting the next, just so we can be sure we don't have more than one waiting, however unlikely that may be with 10ms timeout.

[minrk]

Great, thanks for the explanation - I figured it was something like that, I just wanted to understand.

[fperez]

OK, I've been going over this with a fine-tooth comb for a few hours, trying alternative modes that will simulate deadlocks, introduce delays in the os.kill call by wrapping it in functions that make blocking calls, etc. I went as far as doing something this nasty:

                def stopme():
                    import numpy as np
                    import sys
                    print('blocking work first...')
                    sys.stdout.flush()
                    n = 2000
                    a = np.random.random((n,n))
                    np.linalg.svd(a)
                    print('all done! ^C now...')
                    sys.stdout.flush()
                    os.kill(pid, signal.SIGINT)

                if(not sigint_timer[0]):
                    sigint_timer[0] = threading.Timer(.01, stopme)

and it still behaves as expected. The worst that happens, if the user frantically hits ^C a zillion times, is that an extra unhandled KBINT comes out and prints a short traceback, and that's only with artificial nastiness like the above.

Furthermore, I tested this by forcing my system to work under a load over 12, which is the kind of scenario that often makes weird problems in threading appear, as context switching gets sluggish (and my machine was indeed pretty miserable).

I actually think this is one of those cases where using threads is a reasonable thing to do: threads aren't evil per se, they are just very delicate instruments that need to be used very judiciously.

But the logic here is extremely simple, and the failure modes can't corrupt state, produce non-deterministic output that will later cause problems, or deadlock. So I don't see any problem with it.

Because this logic is a little delicate, though, I'd like to see some comments in there. See inline review.

Once those small cosmetic issues are addressed, I'm happy to merge it, unless anyone else can point out an actual failure mode that would make this a problem.

[fperez]

Add a short descriptive comment before this if that indicates what the intent is and why it's done with os.kill and a timer. It will help anyone who in the future has to revisit this code.

[siyuz]

@fperez I found if I mash CTRL-C and Enter, I can get the git master to generate a traceback as well (which I'm assuming is the same traceback you're seeing). It appears to be because the try: statement that was suppose to catch the KeyboardInterrupt itself was interrupted. Not sure if anything can be done about that.

[fperez]

Oh, certainly: mad mashing of the keyboard will produce a visible traceback, but it doesn't (as far as I have seen) crash ipython itself. It's always true that if you hit C-C fast enough, you can also crash IPython itself. Simply try running:

In [1]: import signal

In [2]: while True:
   ...:     os.kill(10291, signal.SIGINT)

in one shell where 10291 was the PID of another IPython process. It pretty quickly died :)

That's just the reality, a mad sequence of interrupts can eventually leak out of the nested try/excepts we have, and that will always be the case.

But I can't find any pattern where this PR makes it worse or more brittle than it already is, and the fact that these pathological abuses can be done, doesn't worry me.

So, do you see any remaining issues?

[ellisonbg]

Is there a reason that sigint_timer needs to be a list? Everywhere it looks like you always access the first element. Why not just set it to None here and use it as a scalar elsewhere?

[fperez]

He's following the pattern we already had with got_kbdinit: because they are grabbed from the surrounding scope, and the nonlocal keyword is only python-3, this is the only way to do it.

I considered asking a full re-work of this code into a class to better manage the state, but I'm more worried about doing a big refactor for a small change, that can potentially introduce new bugs inadvertently (especially with code as delicate as this).

Once we switch to python3 only, we can just use nonlocal and get rid of this hack.

[fperez]

I guess we could also turn those things into module globals, since code like this is never going to be reused by more than one thing at the same time... A module global would be a bit less ugly than the list trick.

[fperez]

But I'm always worried of turning a simple PR into a big refactoring job...

[ellisonbg]

Yes, I was thinking that a module global is the best approach. Otherwise, there is a hack to get around a very non-obvious aspect of the language.

[fperez]

Yes, now that there's two, it might be better to make them module globals. @siyuz, could you move both got_kbdint and sigint_timer to being scalar module globals? Just assign them to None in a well-marked block at the top, and use the keyword global inside the function.

That's a good balance between asking for a full class refactoring and at least removing this ugly hack.

@siyuz, once that's done I'll gladly merge this. Thanks for your patience with the review process!

[ellisonbg]

I think this is ready to go, @fperez any other things that need to be done?

[fperez]

Yup. The if(..) is slightly non-idiomatic (parens not really needed for a single variable evaluation), but let's just go with it, we've put enough energy into this one already :) @siyuz, thanks for your patience with the review!

Merging now.