drush icon indicating copy to clipboard operation
drush copied to clipboard

Replace subshell with exec

Open webflo opened this issue 1 year ago • 4 comments

Related to https://github.com/drush-ops/drush/issues/6137


This replaces the sub-process with exec, so the signals are treated correctly. But it won't fix the issue if drush is invoked from ./vendor/bin/drush (the composer wrapper script, creates a sub-process too).

webflo avatar Oct 17 '24 17:10 webflo

LGTM. Lets see if anyone else has thoughts ... Please remove from draft when you are satisfied.

weitzman avatar Oct 18 '24 00:10 weitzman

./vendor/bin/drush is a primary endpoint for Drush. Given it does not handle signals you will have to update your Drush launcher, CI scripts, crontabs, etc. Everything that calls Drush...

Chi-teck avatar Oct 18 '24 04:10 Chi-teck

Also these shell wrappers increase the number of processes. Drush 13.3 for each command starts 3 processes. I have a project that runs ~30 Drush workers in background, which produces 90 processes. With this PR it'll be reduced to 60 processes I guess, but it is still not good.

Chi-teck avatar Oct 18 '24 04:10 Chi-teck

Filed a ticket for Composer https://github.com/composer/composer/issues/12164

Chi-teck avatar Oct 18 '24 04:10 Chi-teck

@weitzman Done, Composer adopted the same solution.

webflo avatar Oct 28 '24 23:10 webflo

@Chi-teck Could you do a test with composer self-update --snapshot? This has the new exec code

webflo avatar Oct 28 '24 23:10 webflo

Confirm. With this PR and updated composer no additional shell processes are created. Also signals are handled correctly.

Chi-teck avatar Oct 29 '24 05:10 Chi-teck