Parsl / parsl

Parsl - a Python parallel scripting library
http://parsl-project.org
Apache License 2.0
498 stars 195 forks source link

Multiprocessing 'fork' method is unsafe in the presence of threads #2343

Open benclifford opened 2 years ago

benclifford commented 2 years ago

Describe the bug Threads and multiprocessing cannot be arbitrarily intermingled. Parsl makes heavy use of both threads and multiprocessing.

If one thread in a process holds a lock, while another thread forks a new process, then that new process will start with the lock held, and with nothing to ever unlock it. Any attempt in the new process to acquire that lock will hang forever.

I have observed this in practice in a couple of situations, and issue #1467 is a note that caution needs to be taken in a third situation.

The first, most serious, case that I have observed in practice both in DESC, on perlmutter at NERSC and CSD3 at Cambridge, is a deadlock in glibc's nsswitch lock in nsswitch.c. In this case, in some circumstances when starting up, the WorkQueueExecutor hangs performing a name lookup.

I added debug statements in nsswitch.c around locks taken in _nss_lookup_function that show that this is because _nss_lookup_function cannot aquire a lock in the forked work queue process, due to it being taken by another parsl component in the parent process at fork time.

The second case I have observed occured when adding log statements around startup. In that case, I did not commit those log statements to master parsl, and moved onto other things.

Futher reading around this general problem:

To Reproduce There is a fairly convoluted reproduction of the DESC WorkQueuExecutor hang on perlmutter documented in a NERSC ticket, produced by Jim Chiang.

Expected behavior

Parsl components should not hang.

It might be possible to ensure that there are no threads (or at least threads that take any locks) beyond the main thread during a parsl startup stage where it would then be safe to fork processes; with any necessary threads launched in all components after all components have had a chance to fork.

This would mean that components could not be started up individually and ad-hoc, but that the startup of all components would need to be managed by parsl in two stages.

This separation into fork/thread stages might not be possible - I haven't looked at the code with that in mind yet.

It might also be possible to remove the fork style of process launching entirely - multiprocessing has other mechanisms, for example.

benclifford commented 2 years ago

In the short term, I have applied a bodge time.sleep(15) at a relevant place to peturb a race condition caused by this issue experienced on perlmutter (and hopefully), in the desc branch.

benclifford commented 1 year ago

related to this, #2628 and #2629 show some other fork-related parent-process-inherited bad behaviour, this time with signal handlers

benclifford commented 1 year ago

cpython has this issue about moving the default to spawn rather than fork, which is related to this. https://github.com/python/cpython/issues/84559

benclifford commented 1 year ago

Places where multiprocess is used at the moment in parsl:

htex: interchange starting - forced to be a ForkProcess

htex process worker pool: this is used to launch workers (as various process launch methods or threads) - this is already parameterised so would be straightforward, I think, to deprecate the fork default there.

monitoring: several message handling processes are launched with multiprocessing - some of them forced to be Fork.

on-worker monitoring remote: resource monitoring launches monitored function in a new process, lauunched with ForkProcess.

swift_t - this code doesn't seem to be used?

parsl/tests/test_regression/test_8parsl/tests/test_regression/test_854.py - test of macsafequeue - default mode, not fixed to fork

UsageTracking: one ForkProcess for each usage message

benclifford commented 1 year ago

One of the main impediments to easily switching everything to use spawn (and why fork is hard-coded in some places) is that when launching a spawn process, the original main module (aka the user workflow) will be re-imported. If this isn't written in a style that expects to be imported without "doing its stuff" (i..e doesn't have an `if name == "main" guard) then each spawned process will start the workflow again.

This applies to any process that is forked from the main user workflow (not, for example, process_worker_pool).

In those cases, it would probably be necessary to turn those forked processes into something that can be launched without using multiprocessing. (for example, the htex interchange already uses non-multiprocessing communication channels)

benclifford commented 7 months ago

from Python 3.12, the Python runtime issues warnings like this:

  /usr/local/lib/python3.12/multiprocessing/popen_fork.py:66: DeprecationWarning: This process (pid=6418) is multi-threaded, use of fork() may lead to deadlocks in the child.

in concurrence with this issue.

benclifford commented 5 months ago

moving resource monitoring away from using multiprocessing fork to something more like regular process launching with fork+exec would allow this awfulness to go away: Resource monitoring sometimes deadlocks when using threads, so this function returns false to disable it. but at some increased task startup cost (because starting a new python monitoring process rather than forking an already existing one...)

benclifford commented 2 months ago

parsl/__init__.py contains this global danger setting because MacOS became more angry about this kind of unsafety before Python did: https://github.com/Parsl/parsl/blob/4695c00ad9411967ee9c9ba7a7b2d771bb425603/parsl/__init__.py#L79

Part of fixing this issue should be removing this override - after Parsl has been made safe enough to not be doing the kind of bad thing that needs overriding here.