Repository: symfony/panther (language: php)

Issue:
Avoid `webDriver->quit()` on `__destruct()` when scraping *remote* websites
When connecting to *remote* webpages, I'm sometimes getting this exception:

> Curl error thrown for http DELETE to /session/5db262bc-961f-4cbf-9983-8d602f00d89a
> Operation timed out after 30000 milliseconds with 0 bytes received

And at the bottom of Symfony's exception page:

> Curl error thrown for http POST to /session/5db262bc-961f-4cbf-9983-8d602f00d89a/url with params: {"url":"https://www.example.com"}
Operation timed out after 30001 milliseconds with 0 bytes received

As far as I can see, the cause is:
When done, Panther tries to cleanup and [`Client::quit()`](https://github.com/symfony/panther/blob/main/src/Client.php#L585) calls `$this->webDriver->quit();`.
And from this I'm *guessing*:
* Some servers just respond with `5xx`. Possible side effect: After doing this "forbidden" request repeatedly, I might get blocked.
* Some don't send a response at all. Side effect: Panther waits for 30 seconds (=general timeout), i.e. my command hangs.

So the solution looks pretty clear to me: Don't send that request remotely ;-)

So the first question towards a PR would be: Do you want an *automatic* check, or rather some user-configurable option to suppress this cleanup?

Related: https://github.com/symfony/panther/issues/169 (don't know if it's really the same, or some Docker-related problem)
Tests pass, but throw a FatalError at end (Curl error thrown for http DELETE to /session/xxxxxx)
When I run tests using Panther, all check pass, but the return code is not 0 because a fatal error is thrown at the end. When I run it from my machine this error is reached randomly, on CI machines (self-hosted, github-hosted) it's near 100% of the times.

```
PHPUnit 9.5.20 #StandWithUkraine

Testing 
..........                                                        10 / 10 (100%)

Time: 02:23.562, Memory: 211.00 MB

OK (10 tests, 71 assertions)

Remaining indirect deprecation notices (11)

  2x: Since gesdinet/jwt-refresh-token-bundle 1.0: The "firewall" node is deprecated without replacement.
    2x in AppWebTestCase::_resetDatabase from App\Tests

  2x: Since gesdinet/jwt-refresh-token-bundle 1.0: The "user_provider" node is deprecated without replacement.
    2x in AppWebTestCase::_resetDatabase from App\Tests

  1x: The "DAMA\DoctrineTestBundle\Doctrine\DBAL\AbstractStaticDriver" class implements "Doctrine\DBAL\VersionAwarePlatformDriver" that is deprecated All drivers will have to be aware of the server version in the next major release.
    1x in PHPUnitExtension::executeBeforeFirstTest from DAMA\DoctrineTestBundle\PHPUnit

  1x: Since gesdinet/jwt-refresh-token-bundle 1.0: The "Gesdinet\JWTRefreshTokenBundle\Entity\AbstractRefreshToken" class is deprecated, use "Gesdinet\JWTRefreshTokenBundle\Model\AbstractRefreshToken" instead.
    1x in AppWebTestCase::_resetDatabase from App\Tests

  1x: The "Symfony\Bridge\Doctrine\Logger\DbalLogger" class implements "Doctrine\DBAL\Logging\SQLLogger" that is deprecated Use {@see \Doctrine\DBAL\Logging\Middleware} or implement {@see \Doctrine\DBAL\Driver\Middleware} instead.
    1x in AppWebTestCase::_resetDatabase from App\Tests

  1x: Method "Symfony\Component\EventDispatcher\EventSubscriberInterface::getSubscribedEvents()" might add "array" as a native return type declaration in the future. Do the same in implementation "Stof\DoctrineExtensionsBundle\EventListener\BlameListener" now to avoid errors or add an explicit @return annotation to suppress this message.
    1x in AppWebTestCase::_resetDatabase from App\Tests

  1x: Method "Symfony\Component\EventDispatcher\EventSubscriberInterface::getSubscribedEvents()" might add "array" as a native return type declaration in the future. Do the same in implementation "Stof\DoctrineExtensionsBundle\EventListener\LoggerListener" now to avoid errors or add an explicit @return annotation to suppress this message.
    1x in AppWebTestCase::_resetDatabase from App\Tests

  1x: Method "JsonSerializable::jsonSerialize()" might add "mixed" as a native return type declaration in the future. Do the same in implementation "Facebook\WebDriver\Firefox\FirefoxOptions" now to avoid errors or add an explicit @return annotation to suppress this message.
    1x in ProcessControllerE2ETest::testAddStep from App\Tests\Controller\ProductDevelopment\Probe

  1x: The "Facebook\WebDriver\Local\LocalWebDriver::createBySessionID()" method will require a new "bool $isW3cCompliant" argument in the next major version of its parent class "Facebook\WebDriver\Remote\RemoteWebDriver", not defining it is deprecated.
    1x in ProcessControllerE2ETest::testAddStep from App\Tests\Controller\ProductDevelopment\Probe

PHP Fatal error:  Uncaught Facebook\WebDriver\Exception\WebDriverCurlException: Curl error thrown for http DELETE to /session/27efacf1a6c0ed11e569bc5e75f757e2

Failed to connect to 127.0.0.1 port 9515: Connection refused in /home/mpiot/Documents/apps/vendor/php-webdriver/webdriver/lib/Remote/HttpCommandExecutor.php:333
Stack trace:
#0 /home/mpiot/Documents/apps/vendor/php-webdriver/webdriver/lib/Remote/RemoteWebDriver.php(585): Facebook\WebDriver\Remote\HttpCommandExecutor->execute()
#1 /home/mpiot/Documents/apps/vendor/php-webdriver/webdriver/lib/Remote/RemoteWebDriver.php(312): Facebook\WebDriver\Remote\RemoteWebDriver->execute()
#2 /home/mpiot/Documents/apps/vendor/symfony/panther/src/Client.php(582): Facebook\WebDriver\Remote\RemoteWebDriver->quit()
#3 /home/mpiot/Documents/apps/vendor/symfony/panther/src/Client.php(108): Symfony\Component\Panther\Client->quit()
#4 [internal function]: Symfony\Component\Panther\Client->__destruct()
#5 {main}
  thrown in /home/mpiot/Documents/apps/vendor/php-webdriver/webdriver/lib/Remote/HttpCommandExecutor.php on line 333

Fatal error: Uncaught Facebook\WebDriver\Exception\WebDriverCurlException: Curl error thrown for http DELETE to /session/27efacf1a6c0ed11e569bc5e75f757e2

Failed to connect to 127.0.0.1 port 9515: Connection refused in /home/mpiot/Documents/apps/vendor/php-webdriver/webdriver/lib/Remote/HttpCommandExecutor.php:333
Stack trace:
#0 /home/mpiot/Documents/apps/vendor/php-webdriver/webdriver/lib/Remote/RemoteWebDriver.php(585): Facebook\WebDriver\Remote\HttpCommandExecutor->execute()
#1 /home/mpiot/Documents/apps/vendor/php-webdriver/webdriver/lib/Remote/RemoteWebDriver.php(312): Facebook\WebDriver\Remote\RemoteWebDriver->execute()
#2 /home/mpiot/Documents/apps/vendor/symfony/panther/src/Client.php(582): Facebook\WebDriver\Remote\RemoteWebDriver->quit()
#3 /home/mpiot/Documents/apps/vendor/symfony/panther/src/Client.php(108): Symfony\Component\Panther\Client->quit()
#4 [internal function]: Symfony\Component\Panther\Client->__destruct()
#5 {main}
  thrown in /home/mpiot/Documents/apps/vendor/php-webdriver/webdriver/lib/Remote/HttpCommandExecutor.php on line 333
```


Discussion / hints:
I run 30 instances of Panther in parallel using different ports and each of them connects to different proxy. I often get that error and I'm not sure why
@ThomasLandauer what client for scraping do you use? Curl ? Chrome? Firefox?
Firefox.
It's maybe related to the bug I try to fix in https://github.com/symfony/panther/pull/425. However I didn't manage to get this patch working and I'm not sure of when I'll have the time yo work on it again.

Help welcome on this one (yes, destructors are hard to deal with).
I think I'm experiencing the same issue (i.e. I get the delete error when using Panther on remote sites)

Is there any fix suggested? Or where should I look to try and patch it myself?

Maybe we could add some setting on Client to tell it not to call `$this->webDriver->close()` from `Client::close()`?

Hi @gravitiq-cm, as explained previously, this error is most likely caused by the bug I tried to fix in https://github.com/symfony/panther/pull/425

Unfortunately, I didn't find the time to finish it and it's at the very end of my todo list. Help on fixing this would be much appreciated!
I think a valid solution would be to allow users to define a different class to use instead of the hard-coded `RemoteWebDriver`.

For example, change from:
```
/**
 * @throws \RuntimeException
 */
public function start(): WebDriver
{
    // ...
    return RemoteWebDriver::create(...);
}
```

to:
```
/**
 * @throws \RuntimeException
 */
public function start(): WebDriver
{
    // ...
    $webDriverClass = $this->options['web_driver_class'];
    return $webDriverClass::create(...);
    // (or could use call_user_func_array() if preferred style)
}

```

Users could then create a custom class which extends RemoteWebDriver and has their own customisation. RemoteWebDriver looks extensible... it has no private methods or functions, so would be easy to extend then (in this case) override `CustomRemoteWebDriver::quit()` to not try and delete the session.
IMHO it would be better to fix the bug for everybody without asking the user to write custom code.
Any insight into what causes this or how to fix it?
Hi, in my case I've do a shell script that handle the execution of phpunit, and override the return code by reading the message of phpunit. This is only a workarround, because don't know why this issue appear.

```bash
#/bin/sh
result=$(symfony php vendor/bin/phpunit | tee /dev/stderr)

if echo "$result" | grep -E 'OK \([0-9]+ test'; then
    exit 0;
fi

exit 1
```
I'm not using phpunit... I'm just using `Symfony\Component\Panther\Client::createFirefoxClient();` and running things from there. It all works, except for the fatal errors at the end (after everything has finished, so I guess related to the destructor or connection closing).
Maybe we could add some setting on Client to tell it not to call `$this->webDriver->close()` from `Client::close()`?

Fix patch:
diff --git a/src/Client.php b/src/Client.php
index 6b25cef6..3285eda2 100644
--- a/src/Client.php
+++ b/src/Client.php
@@ -15,6 +15,7 @@
 
 use Facebook\WebDriver\Exception\NoSuchElementException;
 use Facebook\WebDriver\Exception\TimeoutException;
+use Facebook\WebDriver\Exception\WebDriverCurlException;
 use Facebook\WebDriver\JavaScriptExecutor;
 use Facebook\WebDriver\Remote\RemoteWebDriver;
 use Facebook\WebDriver\WebDriver;
@@ -105,7 +106,11 @@ public function __wakeup(): void
 
     public function __destruct()
     {
-        $this->quit();
+        try {
+            $this->quit();
+        } catch (WebDriverCurlException) {
+            // ignore
+        }
     }
 
     public function start(): void


Test patch: