| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
…spawn via fork. This avoids potential deadlocks in the child processes due to forking from a multithreaded process.
|
cc @tomMoral for information and opinions |
Sorry, something went wrong.
|
When looking at the original bug report, I don't see why the deadlock would be linked to bad interaction between fork and threads. When I played with loky, spawning children with fork while having python threads have never lead to deadlocks. The fork+thread deadlock were mostly caused by C-level threadpool (such as openblas ones) that were thought to be present in the child process, but the work was never run as the threads were not started in the forked process. Maybe there is something that I am missing in the context of using tcmalloc that would lead to worst behavior in this context? That being said, when looking at the commit raised by bisect, I would be prone to suspect that something goes wrong with the feature introduced in #19453 and not with the thread+fork pattern, as the deadlock occurs with a concurrent call to shutdown and adjust_worker/ semaphore.release. I could not reproduce the deadlock on my machine as I don't know how to build with the tcmalloc but I would suggest investigating a bit why the deadlock is happening using the faulthandler to dump the stack trace when the deadlock happens: from concurrent import futures
import faulthandler
import sys
def work(iteration, item):
sys.stdout.write(f'working: iteration={iteration}, item={item}\n')
sys.stdout.flush()
for i in range(0, 10000):
faulthandler.dump_traceback_later(20, exit=True)
with futures.ProcessPoolExecutor(max_workers=2) as executor:
executor.submit(work, i, 1)
executor.submit(work, i, 2)
faulthandler.cancel_dump_traceback_later()This should give a traceback that could help understand if the deadlock is really due to fork+thread or if this is due to bad interaction between the semaphore and the shutdown. |
Sorry, something went wrong.
tcmalloc probably uses a background thread and doesn't check that the thread disappeared. This is a common problem with C libraries. |
Sorry, something went wrong.
If this was such a mechanism that is causing the deadlock, this should be deterministic as the C-level threads are never propagated. As the deadlock occurs randomly, I think this should have to do with some concurrency issue. |
Sorry, something went wrong.
|
Ah, then it's possible that the fork happens while a lock is held by another thread in the parent... |
Sorry, something went wrong.
Yep. This is the fundamental reason fork() must never be used in a process with threads running that are not wholly controlled and synchronized by the code doing the fork. Which means "never fork if threads exist" in the context of a library as it isn't in control of the threads. Unfortunately there is no nice API to determine "if threads exist" in a process. One common thing that surfaces this to observers is malloc implementations that use locks as literally everything uses malloc all the time. But that is merely one frequently observed scenario. Any lock or equivalent state anywhere in the program that may have its copy accessed from the forked child process is a potential problem if it was not guaranteed to be in a safe state before the fork via appropriate before/after fork handlers. Threaded libraries can never be assumed to have been designed to do that ...and designing for that can reduce performance. One reason some people wrongly assume "I use fork() in a threaded program and I never have problems" implies that it works is that glibc's malloc intentionally does not use locks (or if it does, it actively manages them during fork) because they aim for a level of legacy compatibility that includes attempting to support existing programs doing bad things that POSIX specs do not actually allow. glibc has a relatively poor malloc implementation for threaded process performance as a result. Thus tcmalloc, jemalloc, mimalloc, etc. all existing and being adopted by people serious about performance. So whenever you see threads knowingly existing and fork being used, there is a problem. Regardless of if you can reproduce it yourself. </preacher mode> |
Sorry, something went wrong.
Adds a comment describing why the lack of assertion means we still have a potential bug.
|
Thanks @gpshead for the PR 🌮🎉.. I'm working now to backport this PR to: 3.9, 3.10, 3.11. |
Sorry, something went wrong.
|
Sorry, @gpshead, I could not cleanly backport this to 3.10 due to a conflict. |
Sorry, something went wrong.
…ethod. (pythonGH-91598) Do not spawn ProcessPool workers on demand when they spawn via fork. This avoids potential deadlocks in the child processes due to forking from a multithreaded process. (cherry picked from commit ebb37fc) Co-authored-by: Gregory P. Smith <greg@krypto.org>
|
GH-92495 is a backport of this pull request to the 3.11 branch. |
Sorry, something went wrong.
|
Sorry @gpshead, I had trouble checking out the 3.9 backport branch. |
Sorry, something went wrong.
… fork method. (pythonGH-91598) Do not spawn ProcessPool workers on demand when they spawn via fork. This avoids potential deadlocks in the child processes due to forking from a multithreaded process.. (cherry picked from commit ebb37fc) Co-authored-by: Gregory P. Smith <greg@krypto.org>
|
GH-92497 is a backport of this pull request to the 3.10 branch. |
Sorry, something went wrong.
… fork method. (pythonGH-91598) (pythonGH-92497) Do not spawn ProcessPool workers on demand when they spawn via fork. This avoids potential deadlocks in the child processes due to forking from a multithreaded process.. (cherry picked from commit ebb37fc) Co-authored-by: Gregory P. Smith <greg@krypto.org> (cherry picked from commit b795376) Co-authored-by: Gregory P. Smith <greg@krypto.org>
…method. (GH-91598) (GH-92497) (#92499) Do not spawn ProcessPool workers on demand when they spawn via fork. This avoids potential deadlocks in the child processes due to forking from a multithreaded process.. (cherry picked from commit ebb37fc) Co-authored-by: Gregory P. Smith <greg@krypto.org> (cherry picked from commit b795376) Co-authored-by: Gregory P. Smith <greg@krypto.org> Co-authored-by: Gregory P. Smith <greg@krypto.org>
… fork method. (pythonGH-91598) (pythonGH-92497) (python#92499) Do not spawn ProcessPool workers on demand when they spawn via fork. This avoids potential deadlocks in the child processes due to forking from a multithreaded process.. (cherry picked from commit ebb37fc) Co-authored-by: Gregory P. Smith <greg@krypto.org> (cherry picked from commit b795376) Co-authored-by: Gregory P. Smith <greg@krypto.org> Co-authored-by: Gregory P. Smith <greg@krypto.org>
| Back | FazBrowse Home | New Git URL |
This avoids potential deadlocks in the child processes due to forking from
a multithreaded process.
This is the bugfix only PR suitable for backporting (manual intervention likely still required). See #91587 for the PR fixing the new 3.11 feature that builds on this. I expect to merge that one first and rebase this on top of it.