Merge pull request #616 from bugsnag/default-guzzle-timeout

Add a default timeout & connect_timeout to Guzzle
bugsnag · Nov 18, 2020 · 9eee8d0 · 9eee8d0
2 parents 8d470f4 + e179e07
commit 9eee8d0
Show file tree

Hide file tree

Showing 7 changed files with 177 additions and 52 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,6 +1,13 @@
 Changelog
 =========
 
+## TBD
+
+### Enhancements
+
+* Add a default timeout & connect_timeout to Guzzle instances created by bugsnag-php. This does not apply if you are providing a custom Guzzle instance.
+  [#616](https://github.com/bugsnag/bugsnag-php/pull/616)
+
 ## 3.24.0 (2020-10-27)
 
 This release changes how Bugsnag detects the error suppression operator in combination with the `errorReportingLevel` configuration option, for PHP 8 compatibility. Bugsnag's `errorReportingLevel` must now be a subset of `error_reporting` — i.e. every error level in `errorReportingLevel` must also be in `error_reporting`

diff --git a/src/Client.php b/src/Client.php
@@ -9,6 +9,7 @@
 use Bugsnag\Callbacks\RequestMetaData;
 use Bugsnag\Callbacks\RequestSession;
 use Bugsnag\Callbacks\RequestUser;
+use Bugsnag\Internal\GuzzleCompat;
 use Bugsnag\Middleware\BreadcrumbData;
 use Bugsnag\Middleware\CallbackBridge;
 use Bugsnag\Middleware\NotificationSkipper;
@@ -73,6 +74,15 @@ class Client
      */
     protected $sessionTracker;
 
+    /**
+     * Default HTTP timeout, in seconds.
+     *
+     * @internal
+     *
+     * @var float
+     */
+    const DEFAULT_TIMEOUT_S = 15.0;
+
     /**
      * Make a new client instance.
      *
@@ -145,39 +155,35 @@ public function __construct(
      */
     public static function makeGuzzle($base = null, array $options = [])
     {
-        $key = self::getGuzzleBaseUriOptionName();
-
-        $options[$key] = $base ?: Configuration::NOTIFY_ENDPOINT;
-
-        if ($path = static::getCaBundlePath()) {
-            $options['verify'] = $path;
-        }
+        $options = self::resolveGuzzleOptions($base, $options);
 
         return new GuzzleHttp\Client($options);
     }
 
     /**
-     * Get the base URL/URI option name, which depends on the Guzzle version.
+     * @param string|null $base
+     * @param array $options
      *
-     * @return string
+     * @return array
      */
-    private static function getGuzzleBaseUriOptionName()
+    private static function resolveGuzzleOptions($base, array $options)
     {
-        return method_exists(GuzzleHttp\ClientInterface::class, 'request')
-            ? 'base_uri'
-            : 'base_url';
-    }
+        $key = GuzzleCompat::getBaseUriOptionName();
+        $options[$key] = $base ?: Configuration::NOTIFY_ENDPOINT;
 
-    /**
-     * Get the base URL/URI, which depends on the Guzzle version.
-     *
-     * @return mixed
-     */
-    private function getGuzzleBaseUri(GuzzleHttp\ClientInterface $guzzle)
-    {
-        return method_exists(GuzzleHttp\ClientInterface::class, 'getBaseUrl')
-            ? $guzzle->getBaseUrl()
-            : $guzzle->getConfig(self::getGuzzleBaseUriOptionName());
+        $path = static::getCaBundlePath();
+
+        if ($path) {
+            $options['verify'] = $path;
+        }
+
+        return GuzzleCompat::applyRequestOptions(
+            $options,
+            [
+                'timeout' => self::DEFAULT_TIMEOUT_S,
+                'connect_timeout' => self::DEFAULT_TIMEOUT_S,
+            ]
+        );
     }
 
     /**
@@ -199,7 +205,7 @@ private function syncNotifyEndpointWithGuzzleBaseUri(
             return;
         }
 
-        $base = $this->getGuzzleBaseUri($guzzle);
+        $base = GuzzleCompat::getBaseUri($guzzle);
 
         if (is_string($base) || (is_object($base) && method_exists($base, '__toString'))) {
             $configuration->setNotifyEndpoint((string) $base);

diff --git a/src/HttpClient.php b/src/HttpClient.php
@@ -3,6 +3,7 @@
 namespace Bugsnag;
 
 use Bugsnag\DateTime\Date;
+use Bugsnag\Internal\GuzzleCompat;
 use Exception;
 use GuzzleHttp\ClientInterface;
 use RuntimeException;
@@ -267,10 +268,10 @@ protected function getHeaders($version = self::NOTIFY_PAYLOAD_VERSION)
      */
     protected function post($uri, array $options = [])
     {
-        if (method_exists(ClientInterface::class, 'request')) {
-            $this->guzzle->request('POST', $uri, $options);
-        } else {
+        if (GuzzleCompat::isUsingGuzzle5()) {
             $this->guzzle->post($uri, $options);
+        } else {
+            $this->guzzle->request('POST', $uri, $options);
         }
     }
 

diff --git a/src/Internal/GuzzleCompat.php b/src/Internal/GuzzleCompat.php
@@ -0,0 +1,88 @@
+<?php
+
+namespace Bugsnag\Internal;
+
+use GuzzleHttp;
+
+/**
+ * @internal
+ */
+final class GuzzleCompat
+{
+    /**
+     * @return bool
+     */
+    public static function isUsingGuzzle5()
+    {
+        if (defined(GuzzleHttp\ClientInterface::class.'::VERSION')) {
+            $version = constant(GuzzleHttp\ClientInterface::class.'::VERSION');
+
+            return version_compare($version, '5.0.0', '>=')
+                && version_compare($version, '6.0.0', '<');
+        }
+
+        return false;
+    }
+
+    /**
+     * Get the base URL/URI option name, which depends on the Guzzle version.
+     *
+     * @return string
+     */
+    public static function getBaseUriOptionName()
+    {
+        return self::isUsingGuzzle5() ? 'base_url' : 'base_uri';
+    }
+
+    /**
+     * Get the base URL/URI, which depends on the Guzzle version.
+     *
+     * @param GuzzleHttp\ClientInterface $guzzle
+     *
+     * @return mixed
+     */
+    public static function getBaseUri(GuzzleHttp\ClientInterface $guzzle)
+    {
+        return self::isUsingGuzzle5()
+            ? $guzzle->getBaseUrl()
+            : $guzzle->getConfig(self::getBaseUriOptionName());
+    }
+
+    /**
+     * Apply the given $requestOptions to the Guzzle $options array, if they are
+     * not already set.
+     *
+     * The layout of request options differs in Guzzle 5 to 6/7; in Guzzle 5
+     * request options live in a 'defaults' array, but in 6/7 they are in the
+     * top level
+     *
+     * @param array $options
+     * @param array $requestOptions
+     *
+     * @return array
+     */
+    public static function applyRequestOptions(array $options, array $requestOptions)
+    {
+        if (self::isUsingGuzzle5()) {
+            if (!isset($options['defaults'])) {
+                $options['defaults'] = [];
+            }
+
+            foreach ($requestOptions as $key => $value) {
+                if (!isset($options['defaults'][$key])) {
+                    $options['defaults'][$key] = $value;
+                }
+            }
+
+            return $options;
+        }
+
+        foreach ($requestOptions as $key => $value) {
+            if (!isset($options[$key])) {
+                $options[$key] = $value;
+            }
+        }
+
+        return $options;
+    }
+}
diff --git a/tests/ClientTest.php b/tests/ClientTest.php
@@ -4,6 +4,7 @@
 
 use Bugsnag\Client;
 use Bugsnag\Configuration;
+use Bugsnag\Internal\GuzzleCompat;
 use Bugsnag\Report;
 use Bugsnag\Tests\Fakes\FakeShutdownStrategy;
 use Exception;
@@ -149,7 +150,7 @@ public function testTheApiKeyAndNotifyEndpointCanBeSetViaEnvSuperglobal()
     public function testTheNotifyEndpointCanBeSetBySettingItOnAGuzzleInstance()
     {
         $guzzle = new Guzzle([
-            $this->getGuzzleBaseOptionName() => 'https://example.com',
+            GuzzleCompat::getBaseUriOptionName() => 'https://example.com',
         ]);
 
         $client = new Client(new Configuration('abc'), null, $guzzle);
@@ -173,7 +174,7 @@ public function testTheNotifyEndpointWontBeOverwrittenByGivenGuzzleInstanceWhenO
         $config->setNotifyEndpoint('https://foo.com');
 
         $guzzle = new Guzzle([
-            $this->getGuzzleBaseOptionName() => 'https://example.com',
+            GuzzleCompat::getBaseUriOptionName() => 'https://example.com',
         ]);
 
         $client = new Client($config, null, $guzzle);
@@ -195,14 +196,14 @@ public function testTheNotifyEndpointWontBeOverwrittenByMakeGuzzleWhenOneIsAlrea
 
     public function testTheNotifyEndpointCanBeSetBySettingItOnAGuzzleInstanceWithAnArray()
     {
-        if (!$this->isUsingGuzzle5()) {
+        if (!GuzzleCompat::isUsingGuzzle5()) {
             $this->markTestSkipped(
                 'This test is not relevant on Guzzle >= 6 as arrays are not allowed'
             );
         }
 
         $guzzle = new Guzzle([
-            $this->getGuzzleBaseOptionName() => [
+            GuzzleCompat::getBaseUriOptionName() => [
                 'https://example.com/{version}', ['version' => '1.2'],
             ],
         ]);
@@ -215,7 +216,7 @@ public function testTheNotifyEndpointCanBeSetBySettingItOnAGuzzleInstanceWithAnA
     public function testTheNotifyEndpointCanBeSetBySettingItOnAGuzzleInstanceWithAUriInstance()
     {
         $guzzle = new Guzzle([
-            $this->getGuzzleBaseOptionName() => new Uri('https://example.com:8080/hello/world'),
+            GuzzleCompat::getBaseUriOptionName() => new Uri('https://example.com:8080/hello/world'),
         ]);
 
         $client = new Client(new Configuration('abc'), null, $guzzle);
@@ -1098,6 +1099,43 @@ public function testShutdownStrategyIsCalledWithinConstructor()
         $this->assertTrue($shutdownStrategy->wasRegistered());
     }
 
+    public function testMakeGuzzleCreatesAGuzzleInstanceWithATimeout()
+    {
+        $guzzle = Client::makeGuzzle();
+
+        $timeout = $this->getGuzzleOption($guzzle, 'timeout');
+        $connectTimeout = $this->getGuzzleOption($guzzle, 'connect_timeout');
+
+        $this->assertSame(Client::DEFAULT_TIMEOUT_S, $timeout);
+        $this->assertSame(Client::DEFAULT_TIMEOUT_S, $connectTimeout);
+    }
+
+    public function testMakeGuzzleCreatesTimeoutCanBeSpecified()
+    {
+        $options = ['timeout' => 1, 'connect_timeout' => 2];
+
+        if (GuzzleCompat::isUsingGuzzle5()) {
+            $options = ['defaults' => $options];
+        }
+
+        $guzzle = Client::makeGuzzle(null, $options);
+
+        $timeout = $this->getGuzzleOption($guzzle, 'timeout');
+        $connectTimeout = $this->getGuzzleOption($guzzle, 'connect_timeout');
+
+        $this->assertSame(1, $timeout);
+        $this->assertSame(2, $connectTimeout);
+    }
+
+    private function getGuzzleOption($guzzle, $name)
+    {
+        if (GuzzleCompat::isUsingGuzzle5()) {
+            return $guzzle->getDefaultOption($name);
+        }
+
+        return $guzzle->getConfig($name);
+    }
+
     private function expectGuzzlePostWith($uri, array $options = [])
     {
         $method = self::getGuzzleMethod();

diff --git a/tests/HttpClientTest.php b/tests/HttpClientTest.php
@@ -4,6 +4,7 @@
 
 use Bugsnag\Configuration;
 use Bugsnag\HttpClient;
+use Bugsnag\Internal\GuzzleCompat;
 use Bugsnag\Report;
 use Exception;
 use GuzzleHttp\Client;
@@ -41,7 +42,7 @@ protected function beforeEach()
 
     private function setExpectedGuzzleParameters($expectation, $callback)
     {
-        if ($this->isUsingGuzzle5()) {
+        if (GuzzleCompat::isUsingGuzzle5()) {
             $expectation->with(
                 $this->config->getNotifyEndpoint(),
                 $this->callback($callback)

diff --git a/tests/TestCase.php b/tests/TestCase.php
@@ -2,7 +2,7 @@
 
 namespace Bugsnag\Tests;
 
-use GuzzleHttp\ClientInterface;
+use Bugsnag\Internal\GuzzleCompat;
 use phpmock\phpunit\PHPMock;
 use PHPUnit\Framework\TestCase as BaseTestCase;
 use PHPUnit\Runner\Version as PhpUnitVersion;
@@ -43,23 +43,7 @@ protected function isPhpUnit4()
      */
     protected static function getGuzzleMethod()
     {
-        return method_exists(ClientInterface::class, 'request') ? 'request' : 'post';
-    }
-
-    /**
-     * @return string
-     */
-    protected function getGuzzleBaseOptionName()
-    {
-        return $this->isUsingGuzzle5() ? 'base_url' : 'base_uri';
-    }
-
-    /**
-     * @return bool
-     */
-    protected function isUsingGuzzle5()
-    {
-        return method_exists(ClientInterface::class, 'getBaseUrl');
+        return GuzzleCompat::isUsingGuzzle5() ? 'post' : 'request';
     }
 
     private function phpUnitVersion()