Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Replace subshell with exec #6140

Merged
merged 1 commit into from
Oct 29, 2024
Merged

Conversation

webflo
Copy link
Contributor

@webflo webflo commented Oct 17, 2024

Related to #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 webflo changed the title Draft / WIP - Replace subshell with exec Replace subshell with exec Oct 17, 2024
@weitzman
Copy link
Member

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

@Chi-teck
Copy link
Collaborator

./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
Copy link
Collaborator

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
Copy link
Collaborator

Filed a ticket for Composer
composer/composer#12164

@webflo webflo marked this pull request as ready for review October 28, 2024 23:25
@webflo
Copy link
Contributor Author

webflo commented Oct 28, 2024

@weitzman Done, Composer adopted the same solution.

@webflo
Copy link
Contributor Author

webflo commented Oct 28, 2024

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

@Chi-teck
Copy link
Collaborator

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

@weitzman weitzman merged commit 15d60d9 into drush-ops:13.x Oct 29, 2024
1 check passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants