Running a Drush command on a remote site can hang when called from external tools

[beerendlauwers]

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: jigarius/drall#77

Describe the solution you'd like

The culprit is this line in Symfony\Component\Process\Process::wait():

$running = '\\' === \DIRECTORY_SEPARATOR ? $this->isRunning() : $this->processPipes->areOpen();

The issue was identified and a solution was suggested in 2020 here: symfony/symfony#21580

To prevent waiting for this upstream change, I propose overriding the checkTimeout() method in SiteProcess:

public function checkTimeout()
    {
        $this->updateStatus(false);
        parent::checkTimeout();
    }

This function is called in the problematic part of the wait() method. updateStatus()is the method that isRunning() 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 extends SiteProcess 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. 😞

[greg-1-anderson]

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

[beerendlauwers]

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

[greg-1-anderson]

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:

• If your PR is applied / merged into site-process and Drush
• AND, later, Symfony makes a similar fix
• Then, is there any bad effect, or does the fix continue to function correctly?
  A casual visual inspection makes me think that's probably safe, but if you could run it through its paces a bit to make sure, then I would accept this change.

A PR with a comment linking to the Symfony issue would be helpful.

[danepowell]

@beerendlauwers are you able to push a PR for this? I think this is biting us in acquia/cli#1581 but it's hard to reproduce outside of that specific use case, which makes it hard to test a fix.