Stop HistorySavingThread before fork#15115
Conversation
|
I fixed some minor issues, but now this leaks instances. I'm wondering why we keep the thread running when saving and not start-stop the thread each time we save. |
|
Thanks 😃 Right, we're leaking instances because the atfork handler keeps a reference to HistoryManager. There's no way to unregister one of these handlers, but as we have a global registry of HistoryManager instances, we could register a single function that iterates through the registry and stops the thread for any instances (which should basically always be 1).
Keeping the thread running normally avoids having to reopen the DB file on every input, though IDK how significant that is. In the context of forking, running the thread only when needed would mostly make the issue invisible, but it would still be possible if you're unlucky to fork just when the thread was running. |
|
I'm just wondering as it's not the first time the HistoryThread is causing trouble; and it's nice to see you back contributing to IPython |
|
Thanks 😊 . It's fun to be looking at it again, especially the history code, which was one of the first bits I worked on. Git blame still shows some lines I wrote almost 15 years ago. 🦕 Out of interest, what other troubles have you had with the history thread? Are they all satisfactorily resolved, or all there still outstanding issues? |
|
iirc leak on ipykernel CI on windows creating some deadlocks (or something else, but thread related and one of the stuck thread was the history one) |
|
#14739 for reference |
Python 3.12+ issues a DeprecationWarning if
os.fork()is called while there are multiple threads, and in fact this is not strictly safe on any version of Python, although on Linux with glibc it mostly works well enough that we don't notice. The man page forfork()says:This is incompatible with executing any Python code, hence the warning. IPython is always multi-threaded because of HistorySavingThread, so it's never safe to call
os.fork()or use multiprocessing's fork context. However, I think we can easily resolve this by stopping the history saving thread before fork, and starting it again after, so at the moment of fork it's a single-threaded process. In principle, I think it's safe to start it again in theafter_in_parenthandler, but Python's check for threads to issue the warning runs after that callback, so to avoid triggering the warning, it's better to start it again when there's something for it to do.This unfortunately only solves the issue for IPython in the terminal, since IPykernel uses several more threads. But fixing it in the terminal is already useful, and this would also be one small step towards fixing it in IPykernel.
Testing: