Fix thread shutdown on PY39+ (fix #1121) #1122
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1122 +/- ##
==========================================
- Coverage 91.35% 91.30% -0.05%
==========================================
Files 80 80
Lines 9159 9166 +7
==========================================
+ Hits 8367 8369 +2
- Misses 792 797 +5
|
There was a problem hiding this comment.
I don't have enough context here to give a proper review, but LGTM. Maybe @doctrino wants to have a look - I believe he added this module initially?
|
This PR is probably a good change either way, but I found what seems to be the root cause of all this; I'll investigate some more tomorrow @erlendvollset |
| for thread, queue in items: | ||
| queue.put(NULL_ENTRY) | ||
| for thread, queue in items: | ||
| thread.join() | ||
|
|
||
|
|
||
| atexit.unregister(_python_exit) | ||
| atexit.register(python_exit) | ||
| # We should register an at-exit-handler to clean up our threads gracefully at interpreter shutdown. |
There was a problem hiding this comment.
I don't really think there is a need for "cleaning up the threads gracefully". Essentially you're just waiting for work to finish, and then tossing the results out, so the only thing you achieve is slower shutdown of the interpreter. Unless I'm missing something here, I think we can remove the atexit hook altogether.
There was a problem hiding this comment.
With current tasks, yes, however, that gives us a "dependency" on the future use of the PTPE to never have tasks with side effects; not something I would want to perpetually make sure is the case.
There was a problem hiding this comment.
I've gone down the deep rabbit hole here and I really think this implementation is the way way to do it. Separate thread pools, separate thread-safe queues, and separate cleanup functions registered that take the python version into account! 🍀
There was a problem hiding this comment.
Fair point (although YAGNI 😄) - let's go for this approach then! 🤝
There was a problem hiding this comment.
Lol YAGNI, learned something new today 😂 Btw, if you really want it removed why don’t we also remove from the standard library by monkeypatching? 😜 I feel good knowing we mimic the exact same approach as some code written by people way smarter than myself 😂
There was a problem hiding this comment.
Also; the code do crash without it 😉
Description
Starting in 3.9, ThreadPoolExecutor no longer uses daemon threads, but instead, an internal function hook very similar to atexit.register(), which gets called at 'threading' shutdown instead of interpreter shutdown. Check out python/cpython#83993
Some links:
This sounds like an argument for testing multiple Python versions (with
toxperhaps)? 🤔Checklist:
If a new method has been added it should be referenced in cognite.rst in order to generate docs based on its docstring.