Skip to content

Commit

Permalink
Refactor how timeouts & exceptions are handled on websocket
Browse files Browse the repository at this point in the history
Does a few things:

* Removes (and removes use of) the `->canDevToolsConnectionBeEstablished`
  method. This was permanently returning false due to a code error (it's
  calling the wrong URL) and realistically if the socket has timed out but
  not EOF it's virtually certain it'll still be open.
* Allows waitFor to take a timeout, if provided then it will enforce that
  regardless of whether the socket is healthy (possibly sending packets
  we don't care about) or failing (timing out the socket because it has
  nothing to say).
* Refactors waitForLoad to just keep retrying & waiting with the new
  timeout implementation.
* Gets rid of us throwing ConnectionException at all, the data and message
  formats are too tied to the websocket. Throw a sensible DriverException.
  • Loading branch information
acoulton committed Sep 15, 2022
1 parent 9713f37 commit 93070bc
Show file tree
Hide file tree
Showing 6 changed files with 67 additions and 86 deletions.
2 changes: 1 addition & 1 deletion src/ChromeBrowser.php
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ public function close()
{
if ($this->headless) {
if (!$this->send('Target.disposeBrowserContext', ['browserContextId' => $this->context_id])) {
throw new ConnectionException('Unable to close browser context');
throw new DriverException('Unable to close browser context');
}
}
parent::close();
Expand Down
1 change: 0 additions & 1 deletion src/ChromeDriver.php
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,6 @@ public function stop()
$this->browser->close();
} catch (ConnectionException $exception) {
} catch (DriverException $exception) {
} catch (StreamReadException $exception) {
}

$this->is_started = false;
Expand Down
1 change: 1 addition & 0 deletions src/ChromeDriverDebugLogger.php
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ public function logConnectionException(
[
'client' => $this->nameConnection($connection),
'action' => 'connectionException',
'class' => \get_class($exception),
'waiting' => $wait_reason,
'message' => $exception->getMessage(),
'data' => $exception->getData(),
Expand Down
26 changes: 9 additions & 17 deletions src/ChromePage.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
namespace DMore\ChromeDriver;

use Behat\Mink\Exception\DriverException;
use WebSocket\ConnectionException;

class ChromePage extends DevToolsConnection
{
Expand Down Expand Up @@ -56,24 +55,17 @@ public function waitForLoad()
{
if (!$this->page_ready) {
try {
// @todo: need a better way of setting the timeout expiry based on the start of pageload rather than specific call
$timeout = new \DateTimeImmutable('+30 seconds');
$this->waitFor(function () {
return $this->page_ready;
}, 'wait-for-load');
} catch (StreamReadException $exception) {
if ($exception->isTimedOut() && false === $this->canDevToolsConnectionBeEstablished()) {
throw new \RuntimeException(
sprintf(
'Chrome is unreachable via "%s" and might have crashed. Please see docs/troubleshooting.md',
$this->getUrl()
)
);
}

if (!$exception->isEof() && $exception->isTimedOut()) {
$this->waitForLoad();
}
} catch (ConnectionException $exception) {
throw new DriverException("Page not loaded");
},
'wait-for-load',
$timeout
);
} catch (DriverException $exception) {
// This could then be decorated with more detail on what we're waiting for
throw new DriverException("Page not loaded: ".$exception->getMessage(), 0, $exception);
}
}
}
Expand Down
72 changes: 56 additions & 16 deletions src/DevToolsConnection.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
use Behat\Mink\Exception\DriverException;
use WebSocket\Client;
use WebSocket\ConnectionException;
use WebSocket\TimeoutException;

abstract class DevToolsConnection
{
Expand All @@ -27,14 +28,13 @@ public function __construct($url, $socket_timeout = null)

public function canDevToolsConnectionBeEstablished()
{
$url = $this->getUrl() . "/json/version";
$c = curl_init($url);
curl_setopt($c, CURLOPT_RETURNTRANSFER, 1);
curl_setopt($c, CURLOPT_FOLLOWLOCATION, 1);
$s = curl_exec($c);
curl_close($c);

return $s !== false && strpos($s, 'Chrome') !== false;
// It would need instead to get & call the HTTP API url, but it's actually pointless to do that anyway
// because realistically in almost every case the connection has only timed out because Chrome has nothing
// to say and connecting to an HTTP API endpoint doesn't really tell us anything anyway.
throw new \BadMethodCallException(
__METHOD__.
' has been removed because it always returned false due to adding a path to the end of the websocket URL'
);
}

protected function getUrl()
Expand Down Expand Up @@ -85,22 +85,62 @@ public function send($command, array $parameters = [])
return ['result' => ['type' => 'undefined']];
}

protected function waitFor(callable $is_ready, string $debug_reason)
protected function waitFor(callable $is_ready, string $debug_reason, ?\DateTimeImmutable $timeout = NULL)
{
$data = [];
while (true) {
while (TRUE) {
if ($timeout && (new \DateTimeImmutable > $timeout)) {
// Sometimes the socket itself doesn't time out (e.g. if a page is sending AJAX requests
// on a timer of some kind but the `load` event has never fired). I'm still not clear on
// underlying cause of us never getting to page load status, but we definitely don't want this
// to loop infinitely until build timeout.
throw new DriverException(
'Timed out waiting for Chrome state - websocket is healthy'
);
}

try {
$response = $this->client->receive();
} catch (ConnectionException $exception) {
} catch (TimeoutException $exception) {
$this->logger->logConnectionException($this, $exception, $debug_reason);
$message = $exception->getMessage();
if (false !== strpos($message, 'Empty read; connection dead?')) {
throw $exception;

// A stream read timeout generally just happens when Chrome has nothing to report for the duration
// of the stream timeout. This may mean that:
// a) the page is idle & waiting on user action / a timer to trigger some action
// b) a server-side pageload has taken unusually long (there are no websocket packets while the main
// document request is pending, so if the server-side pageload takes longer than the socket timeout
// then you will always get a read timeout.
// c) a page is loading but e.g. a child request is hanging so again there is no activity to report
// and no events to fire.
if ($timeout && ($timeout > new \DateTimeImmutable)) {
// We are still within the application level timeout that the caller provided. Just retry reading
continue;
}

$state = $exception->getData();
// If the caller did not provide a timeout, then we should not retry and should just throw.
throw new DriverException(
sprintf(
'Timed out reading from Chrome: %s %s',
$exception->getMessage(),
\json_encode($exception->getData())
),
0,
$exception
);
} catch (ConnectionException $exception) {
$this->logger->logConnectionException($this, $exception, $debug_reason);

throw new StreamReadException($state['eof'], $state['timed_out'], $state['blocked']);
// These are not just a simple timeout, they represent a more failure error on the socket, treat them
// as outright failures and just rethrow.
throw new DriverException(
sprintf(
'Error reading from Chrome: %s (%s)',
$exception->getMessage(),
\json_encode($exception->getData())
),
0,
$exception
);
}

if (is_null($response)) {
Expand Down
51 changes: 0 additions & 51 deletions src/StreamReadException.php

This file was deleted.

0 comments on commit 93070bc

Please sign in to comment.