search

drush-ops / drush

Drush is a command-line shell and scripting interface for Drupal, a veritable Swiss Army knife designed to make life easier for those who spend their working hours hacking away at the command prompt.
https://www.drush.org
2.34k stars 1.08k forks source link

Replace subshell with exec #6140

Closed webflo closed 3 weeks ago

webflo commented 1 month ago

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).

weitzman commented 1 month ago

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

Chi-teck commented 1 month ago

./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 commented 1 month ago

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 commented 1 month ago

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

webflo commented 3 weeks ago

@weitzman Done, Composer adopted the same solution.

webflo commented 3 weeks ago

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

Chi-teck commented 3 weeks ago

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