On 04/13, Jonathan Nieder wrote: > Brandon Williams wrote: > > > From what I can see, there are now no calls in the child process (after fork > > and before exec/_exit) which are not Async-Signal-Safe. This means that > > fork/exec in a threaded context should work without deadlock > > I don't see why the former implies the latter. Can you explain > further? > > You already know my opinions about fork+threads by now. I continue to > think this is heading in a direction of decreased maintainability that > I dread.
I disagree here. No one thought it was a bad idea back when I was implementing grep to fork while running with threads. Id rather fix the problem in run_command so that we don't ever have to worry about this again, especially for contributors who are unaware of this issue while threading. > > That's not to say that this is wasted work. I would prefer an approach > like the following: > > 1. First, make grep work without running fork() from threads. > Jonathan Tan's approach would be one way to do this. Another way > would be to simply disable threads in --recurse-submodules mode. > > This would be the first thing to do because it would make tests > reliable again, without having to wait for deeper changes. I'm not much of a fan of Jonathan Tan's suggestion, id rather just fix the problem at its root instead of adding in an additional hack. If this series crashes and burns then yes, lets just shut off threading in grep with --recurse-submodules is uses, otherwise this series will fix that case. > > 2. Then, teaching run_command to prepare the environment and do $PATH > lookup before forking. This might make it possible for run_command > to use vfork or might not. > > 3. Teaching run_command to delegate chdir to the child, using -C for > git commands and 'cd' for shell commands, and using a shell where > necessary where it didn't before. > > 4. Switching run_command to use posix_spawn on platforms where it is > available, which would make it possible to use in a threaded > context on those platforms. After this series it should be easy enough to convert to posix_spawn if someone wants to follow up with a patch to do that. -- Brandon Williams
