Open beerendlauwers opened 1 year ago
Have you tested to see if this change would be innocuous if run with the Symfony fix already applied? If it checks out on that front, it looks safe enough to me, & I would welcome a PR with the recommended change..
@greg-1-anderson If you mean to ask whether I've actually tested a few commands with this change: yes, I have. These worked fine on remote aliases: ssh
, sql:cli
, cr
, status
. If you want, I can test run a few other complex ones I use regularly: sql:sync
, rsync
, cset
and php:eval
.
It seems the comment I put here a while back was lost. My question was actually even a little simpler than that. What I want to know is:
A PR with a comment linking to the Symfony issue would be helpful.
@beerendlauwers are you able to push a PR for this? I think this is biting us in https://github.com/acquia/cli/pull/1581 but it's hard to reproduce outside of that specific use case, which makes it hard to test a fix.
Is your feature request related to a problem? Please describe.
We're encountering an infinite loop when running Drush commands on remote site aliases when using Drall: https://github.com/jigarius/drall/issues/77
Describe the solution you'd like
The culprit is this line in
Symfony\Component\Process\Process::wait()
:The issue was identified and a solution was suggested in 2020 here: https://github.com/symfony/symfony/issues/21580
To prevent waiting for this upstream change, I propose overriding the
checkTimeout()
method inSiteProcess
:This function is called in the problematic part of the
wait()
method.updateStatus()
is the method thatisRunning()
uses to determine if the process is still running or not.Describe alternatives you've considered
I tried to find a way to modify Drush's injected services, so we can change
process.manager
to return a class that extendsSiteProcess
with the code change above. Ideally, this would be done via a command-line option that could be added by Drall.readPipes()
would be an even better fit, but it's private. :disappointed: