Replace multiprocessing with threading by coleaeason · Pull Request #10 · Expensify/fab-classic

coleaeason · 2026-02-17T22:04:18Z

Problem

On macOS() calling fork() after exec() can cause seg faults in system libraries because they are not thread safe. This causes segfaults because the memory locations copied from the parent into the child process via fork() are no longer valid.

One suggested fix is to use spawn() instead of fork() but that only works if all code can be pickled, which isn't always the case. The other option, which is implemented here, is to use threading instead.

Note, I implemented thread local versions of the env and other shared variables to replicate the current behavior. I know we internally have some desire to be able to share globals across execution threads for parallel tasks, but I didn't want to scope creep that into this PR, so we can address it later with a more thought out design.

This should also have some added benefits when run on ft python.

Fixes https://github.com/Expensify/Expensify/issues/601176
Fixes https://github.com/Expensify/Expensify/issues/591584

coleaeason · 2026-02-17T22:25:40Z

@rafecolton as far as I can tell this works perfectly fine. I tested locally with saltfab. Salting servers, etc in parallel and serial all works.

rafecolton

Surprisingly simple change! Biggest concerns are around eating exit codes and setting .daemon = True on all threads

rafecolton · 2026-02-20T01:45:10Z

fabric/exceptions.py

-    # a multiprocessing.Queue.
-    def __init__(self, message=None, wrapped=None):
+    # Must allow for calling with zero args/kwargs for consistency.
+    def __init__(self, message, wrapped):


Signature change seems extraneous, so unless it's needed, I'd rather see it reverted to reduce the diff and chances of bugs. Seems reasonable to update the comment, though would like to see it be a little more informative (i.e. consistency with what)

Updating the comment without changing the code doesn't really make sense. The only reason this exists is a hack for pickling to work in a multiprocessing queue which we removed entirely. i can clarify the comment

👍 I'm inclined to leave the code as it was then, but if you're confident this works, then we can move forward with an updated comment

fabric/job_queue.py

rafecolton · 2026-02-20T01:48:58Z

fabric/network.py

+
+    def __init__(self, *args, **kwargs):
+        super().__init__(*args, **kwargs)
+        dict.__setattr__(self, '_tl', threading.local())


Prefer unabbreviated variable names. This whole class seems a bit over-engineered - e.g. why do you need the _local() function as opposed to just naming the variable _local? Any why use this syntax vs just self._thread_local = threading.local()?

I looked at this originally. I'm wondering if it's needed at all. I know claude made it so create support for non-local versions of the variables but I don't know that that actually is needed... I will re-review.

fabric/tasks.py

rafecolton · 2026-02-20T02:00:13Z

fabric/tasks.py

            'env': local_env,
        }
-        p = multiprocessing.Process(target=_parallel_wrap, kwargs=kwarg_dict)
+        p = threading.Thread(target=_parallel_wrap, kwargs=kwarg_dict)


The name p is no longer applicable, consider something more relevant (an unabbreviated) like thred

rafecolton · 2026-02-20T02:00:45Z

fabric/tasks.py

        }
-        p = multiprocessing.Process(target=_parallel_wrap, kwargs=kwarg_dict)
+        p = threading.Thread(target=_parallel_wrap, kwargs=kwarg_dict)
+        p.daemon = True


Hm, I think this can result in background threads sticking around even if the main process is killed, is that right? Are you sure we want this?

interesting point, i'll think about this a little more

I think the main process waits for all the threads to finish, so I don't think there is a concern about it killing threads prematurely. If somebody ^C's the main process, we actively want it to kill threads.

In daemon mode the main process does not wait for all threads to finish, it kills them all immediately if the main process terminates, so orphan threads cannot be created, which is what we want. The only difference between the current fork() setup and the thread replacement is that in fork() the subprocesses receive the same signal as the parent, so ctrl+c is propagated down to all of the children and they can also handle the signal whereas here, the threads are just killed. Both stop execution, but the current implementation allows for cleanup processes such as finally blocks to run where as the new implementation does not.

This can be fixed by using stop events, but that may slow shut down for interrupts if a thread is blocked on i/o and would require a little more rewriting here. That may be preferred, though.

Interesting 🤔 do we actually do any signal handling in anything we expect to run in parallel? I can't think of anything. If we do, then we should probably handle it in this PR. If not, then I agree events may still be better, but we can defer it to a follow-up

Signal handling would only be for ctl +c otherwise we do not do any.

Right, do we do any for ^C for tasks we run in parallel? I'll thing I can think of is deploy prep, which runs in serial

No we don't do any today

rafecolton · 2026-02-20T02:03:31Z

fabric/utils.py

+        super().__init__(*args, **kwargs)
+        # Use dict.__setattr__ to avoid triggering _AttributeDict's __setattr__
+        # which would create a key in the dict.
+        dict.__setattr__(self, '_tl', threading.local())


prefer unabbreviated variable name

rafecolton · 2026-02-20T02:04:21Z

fabric/utils.py

        return ret


+class _ThreadLocalAttributeDict(_AttributeDict):


This seems somewhat redundant with _ThreadLocalHostConnectionCache? Not sure if they can be combined somehow

Only sort of. These thread local wrappers are wrapped around the previously defined classes, which are different from each other. The wrappers look similar because they are just making each of the classes thread local-able.

👍 would be great to have something less redundant but if that's not feasible for this round, then please consider the same comments as above - unabbreviated variable names & confirming if all the functions AI added are necessary

os-botify · 2026-03-03T22:42:18Z

🚀 Released in version 1.20.6 🚀

coleaeason added 7 commits February 17, 2026 16:45

Ignore claude dir

8be9729

Update job queue to use threading only

ac39f60

Update network layer to support threading

26a940e

Add thread local implementations for threading replacement

06189a4

use thread local variables in state

2ecc94d

replace multiprocessing core for parallel with threading

b038b65

Update comment and exception implementation for threading from mp

b8aea39

coleaeason self-assigned this Feb 17, 2026

coleaeason changed the title ~~Cole/threading~~ Replace multiprocessing with threading Feb 17, 2026

Missed a find and replace

31d5d63

coleaeason marked this pull request as ready for review February 17, 2026 22:24

coleaeason requested a review from rafecolton February 17, 2026 22:24

rafecolton reviewed Feb 20, 2026

View reviewed changes

Clean up thread local code to be DRY

3a0a58e

rafecolton approved these changes Mar 3, 2026

View reviewed changes

coleaeason merged commit 90e2139 into main Mar 3, 2026
3 checks passed

coleaeason deleted the cole/threading branch March 3, 2026 22:42

Conversation

coleaeason commented Feb 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Uh oh!

coleaeason commented Feb 17, 2026

Uh oh!

rafecolton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

os-botify bot commented Mar 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

coleaeason commented Feb 17, 2026 •

edited

Loading