Conversation
ed26da6 to
49297b3
Compare
Right now the cleanup was mostly done at process exit time, but this leads to issues in downstream test that start to have dozen of threads blocking, and make finding deadlocks harder This PR try to cleanup the cleaning logic. - Try to seprate saving thread from history manager - Make sure to not keep reference to history manager in multiple places. - att a utility to refuse to create more then N concurrent history manager HistorySavingThread instances. - default to 1 in conftest.py - allow to set it to N via pytest fixture. Some of this should likely be either context managers, or finalisers.
|
Not without a major refactor and breaking a lot of APIs. Basically you would need There are a number of other things that should be context managers. Some objects are doing crazy things like having In conftest.py you can set HistorySavingThread's class attribute See how I'm doing it in this PR, and the fixture I use to temporarily increase the number of HistoryManager for specific tests. |
|
I think doing cleanup at garbage collection is very brittle because the object can only be garbage collected if there is no reference to it anymore, which is out of control. |
|
There are already methods that stop the thread, you can already call
finalizer will make sure those get called at most once, or be no-op, but still requires a lot of refactor and tracking. Like right now, closing the thread while there are still live instance of history manager meas that we likely have a bug. So yes I agree with you, but it will need some manpower to make sure the code is sound (right now it definitely isn't), and detecting leaks will help us making it sound. |
Right now the cleanup was mostly done at process exit time,
but this leads to issues in downstream test that start to have dozen
of threads blocking, and make finding deadlocks harder
This PR try to cleanup the cleaning logic.
concurrent history manager HistorySavingThread
instances.
Some of this should likely be either context managers, or finalisers.