Fix terminal exit

[takluyver]

This fixes the failure to exit in IPython terminal interpreter.

The problem was the history autosave thread: if a y/n confirmation of exiting was used (i.e. following Ctrl-D), the commands to stop the thread were called, but if "exit" was used, which doesn't require confirmation, they weren't.

I've overcome this simply by setting the thread to daemon mode, which means that Python doesn't attempt to wait for it after the main thread has exited. At present, I've removed the code that stops the autosave thread on shutdown, as it seems superfluous.

I've also refactored the sequence of commands used to stop it into a .stop() method on the thread object, although it's not currently used anywhere.

[takluyver]

I've updated it after Brian pointed out that the test suite was having trouble too. Now the thread's stop() method is called by the atexit handler. I've also rewritten the thread's stopping system to use a threading.Event, rather than a threading.Condition and boolean flag.

[ellisonbg]

I do think we should try to figure out a good way of handling this, rather than just removing it entirely. But I have a few questions about the history save mechanism:

• Prior to the recent history refactor, there was no need to to use a thread to save the history. Why is this approach needed now? Why can't we do what we always were?
• How does the kernel for the qtconsole save the history?

But, I do like the usage of the Event better than the condition variable approach. The only comment I have about the code is that the initialization of this stuff in interactiveshell should happen in the init_history method.

[takluyver]

I confess I'm not entirely sure about the reasons behind it--I don't really use history beyond hitting up once or twice. Presumably, the point of a periodic autosave is to preserve your history in case of a crash. In which case, we'd probably need a repeatable crash to test that it's working. I'll drop the author an e-mail.

[ellisonbg]

Thanks for following up on this.

[minrk]

I think this fix looks good. I don't ever use the exit command, so I missed this during the review (it came up in the prompted exit, and we fixed it there).

The reason behind it is periodic autosave, to preserve history even in case of a crash. It also means that if you have a long running session, and you start another session, it will have a more current history at load.

In answer to Brian:

• This is entirely new, so there's no 'do what we always did' option, unless you mean 'lose everything if it crashes', which is what we did before.
• qtconsole does not save the history, the Kernel does, and it still saves at exit as before. This background thread is purely in addition to the regular behavior.

I should probably stop reviewing IPython code, as there always seems to be a problem when I do.

[ellisonbg]

Min, thanks for the comments. So you are saying that the original behavior is that the history gets saved at exit and that this happens in both the terminal based ipython and the ipkernel. The new behavior simply adds autosaving that happens while the processes run. Is that a good description? If so, let's do the following:

• Add docs to the history saving class that indicate that this is what it does.
• Move the initialization of this stuff from init to init_history.

Once that is done I will give it a final review and merge into master.

I should probably stop reviewing IPython code, as there always seems to be a problem when I do

Min, oh no, please continue reviewing. IPython is a huge project and things like this are difficult to test. I can hardly keep track of things myself. My rule is that any review is better than none. Thanks for helping out with this stuff!

[mchandra]

Hi,
I'm sorry for the trouble caused by the patch I sent. I should have done an exhaustive check before sending a pull request. I intended for it to periodically backup the commands since I frequently use ipython remotely and have lost many sessions due to network problems. I tried to put it in the main kernel so that it will work for both the terminal and qt interfaces but it didn't work out. I can try again to fix it or perhaps the concept can be implemented in a better way?

[takluyver]

Regarding having two IPython instances open: what does history do in this case? It appears there is a common file for the user, in ~/.ipython/history.json. Does it just end up with whichever interpreter happened to last save it? If they exit normally, that should be the last one to close, which is probably sane behaviour. We probably don't want the two histories interleaved. And could the file get corrupted if two processes attempt to write to it simultaneously?

[ellisonbg]

I don't think anyone has thought about these issues yet. Minimally we need to add some notes to the code about this. Because the history is so extremely useful, I think we should leave it in, but continue to discuss the best model. Currently, it is entirely possible that the history file would be corrupted and multiple histories would definitely be interleaved. This definitely needs some further though. I would love to get Fernando and Santra's thoughts on this as they implemented the history functionality.

[takluyver]

I've filled out the docstrings a bit.

Brian: I agree, lets move discussion of that onto the mailing list.

Mani: Don't worry, these things happen. I don't know that I'd have spotted it, either--I almost always use Ctrl-D to exit. If you've got time, can you test my branch, and check that it's doing what you expect if you lose a connection?

[minrk]

As for multiple histories, it's the same for autosave as for regular save. If two processes try writing to a file at the exact same time, you can have corruption. We could add a lock file for this if we want to be fairly rigorous, but it's only during the time of a pending write that that can happen, which is quite short, unless you have an enormous history and/or incredibly slow filesystem.

Since the new history is now json, and not the line-based readline history, if they are interleaved it will no longer be valid, and thus corrupted, so it's more important to avoid than it used to be.

JSON was chosen since it can support multiline history. If you have ever tried to go reconstruct multiline entries from your history in 0.10 between sessions, it was pretty painful.

Brian - yes, your description of autosave was accurate. I don't know how to go about making the tweaks, since there is already this pull request.

[takluyver]

I've added a try/except block in reload_history, so that it will ignore previous history if the JSON file is corrupt. Without that, breaking the JSON left IPython unable to start.

At present, it will just silently ignore it, and the next save_history will overwrite it. I don't know if we want to print a warning, log the problem, or clear the history file straight away.

[mchandra]

Hi Thomas, I've tested out your branch and it works nicely. I have simulated crashes by running ipython in a screen session and killing the session without exiting ipython. The history was saved and I was able to recover all the commands.

[takluyver]

Thanks, Mani. If someone's got time to review this and merge it this weekend, then we should be passing the tests again.

[minrk]

I've reviewed this, and it looks solid, thanks!
But I think someone other than me should do the sign-off and merge, since it's my error that's being fixed here.

[takluyver]

Thanks, Min.

Brian, if you've got a few minutes, could you do a final review on this?

[ellisonbg]

Fernando had a great idea about this yesterday. Currently, the existing approach is not thread safe as the history save method is called from a thread. To make this thread safe, we can put a condition variable in the IPython "event loop" that the thread can set. When it is set, IPython will save the history, but that happens in the main thread. How does this sound?

[ellisonbg]

I will be on #ipython around 12:30 PST today if you want to chat about this.

[takluyver]

Autosave system modified following discussion with Brian on IRC.

I've refactored somewhat - the autosave timer thread is now an attribute of the history manager, rather than directly of the InteractiveShell. This led to an error on shutdown in the tests, as one test created a separate HistoryManager, thereby starting a second autosave thread, which wasn't stopped. So now the HistorySaveThread registers its stop method with atexit upon creation, separate from the cleanup in InteractiveShell.atexit_operations. I think this makes for a neater encapsulation.