Implement support for flushable clients (#813)

CHANGELOG.md

@@ -6,6 +6,7 @@
 - Changed type hint for both parameter and return value of `HubInterface::getCurrentHub` and `HubInterface::setCurrentHub()` methods (#849)
 - Add the `setTags`, `setExtras` and `clearBreadcrumbs` methods to the `Scope` class (#852)
 - Silently cast numeric values to strings when trying to set the tags instead of throwing (#858)
+- Support force sending events on-demand and fix sending of events in long-running processes (#813)
 
 ## 2.1.1 (2019-06-13)
 

composer.json

@@ -15,6 +15,7 @@
         "php": "^7.1",
         "ext-json": "*",
         "ext-mbstring": "*",
+        "guzzlehttp/promises": "^1.3",
         "jean85/pretty-package-versions": "^1.2",
         "php-http/async-client-implementation": "^1.0",
         "php-http/client-common": "^1.5|^2.0",

src/Client.php

@@ -4,17 +4,20 @@
 
 namespace Sentry;
 
+use GuzzleHttp\Promise\FulfilledPromise;
+use GuzzleHttp\Promise\PromiseInterface;
 use Sentry\Integration\Handler;
 use Sentry\Integration\IntegrationInterface;
 use Sentry\State\Scope;
+use Sentry\Transport\ClosableTransportInterface;
 use Sentry\Transport\TransportInterface;
 
 /**
  * Default implementation of the {@see ClientInterface} interface.
  *
  * @author Stefano Arlandini <[email protected]>
  */
-final class Client implements ClientInterface
+final class Client implements FlushableClientInterface
 {
     /**
      * The version of the protocol to communicate with the Sentry server.
@@ -79,11 +82,13 @@ public function captureMessage(string $message, ?Severity $level = null, ?Scope
             'level' => $level,
         ];
 
-        if ($event = $this->prepareEvent($payload, $scope, $this->options->shouldAttachStacktrace())) {
-            return $this->transport->send($event);
+        $event = $this->prepareEvent($payload, $scope, $this->options->shouldAttachStacktrace());
+
+        if (null === $event) {
+            return null;
         }
 
-        return null;
+        return $this->transport->send($event);
     }
 
     /**
@@ -95,21 +100,21 @@ public function captureException(\Throwable $exception, ?Scope $scope = null): ?
             return null;
         }
 
-        $payload['exception'] = $exception;
-
-        return $this->captureEvent($payload, $scope);
+        return $this->captureEvent(['exception' => $exception], $scope);
     }
 
     /**
      * {@inheritdoc}
      */
     public function captureEvent(array $payload, ?Scope $scope = null): ?string
     {
-        if ($event = $this->prepareEvent($payload, $scope)) {
-            return $this->transport->send($event);
+        $event = $this->prepareEvent($payload, $scope);
+
+        if (null === $event) {
+            return null;
         }
 
-        return null;
+        return $this->transport->send($event);
     }
 
     /**
@@ -136,6 +141,18 @@ public function getIntegration(string $className): ?IntegrationInterface
         return $this->integrations[$className] ?? null;
     }
 
+    /**
+     * {@inheritdoc}
+     */
+    public function flush(?int $timeout = null): PromiseInterface
+    {
+        if (!$this->transport instanceof ClosableTransportInterface) {
+            return new FulfilledPromise(true);
+        }
+
+        return $this->transport->close($timeout);
+    }
+
     /**
      * Assembles an event and prepares it to be sent of to Sentry.
      *

src/FlushableClientInterface.php

@@ -0,0 +1,26 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Sentry;
+
+use GuzzleHttp\Promise\PromiseInterface;
+
+/**
+ * This interface can be implemented by clients to support flushing of events
+ * on-demand.
+ *
+ * @author Stefano Arlandini <[email protected]>
+ */
+interface FlushableClientInterface extends ClientInterface
+{
+    /**
+     * Flushes the queue of events pending to be sent. If a timeout is provided
+     * and the queue takes longer to drain, the promise resolves with `false`.
+     *
+     * @param int|null $timeout Maximum time in seconds the client should wait
+     *
+     * @return PromiseInterface
+     */
+    public function flush(?int $timeout = null): PromiseInterface;
+}

src/Transport/ClosableTransportInterface.php

@@ -0,0 +1,25 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Sentry\Transport;
+
+use GuzzleHttp\Promise\PromiseInterface;
+
+/**
+ * All classes implementing this interface will support sending events on-demand.
+ *
+ * @author Stefano Arlandini <[email protected]>
+ */
+interface ClosableTransportInterface
+{
+    /**
+     * Waits until all pending requests have been sent or the timeout expires.
+     *
+     * @param int|null $timeout Maximum time in seconds before the sending
+     *                          operation is interrupted
+     *
+     * @return PromiseInterface
+     */
+    public function close(?int $timeout = null): PromiseInterface;
+}

src/Transport/HttpTransport.php

@@ -4,15 +4,16 @@
 
 namespace Sentry\Transport;
 
-use Http\Client\HttpAsyncClient;
-use Http\Message\RequestFactory;
-use Http\Promise\Promise;
+use Http\Client\HttpAsyncClient as HttpAsyncClientInterface;
+use Http\Message\RequestFactory as RequestFactoryInterface;
+use Http\Promise\Promise as PromiseInterface;
 use Sentry\Event;
 use Sentry\Options;
 use Sentry\Util\JSON;
 
 /**
- * This transport sends the events using an HTTP client.
+ * This transport sends the events using a syncronous HTTP client that will
+ * delay sending of the requests until the shutdown of the application.
  *
  * @author Stefano Arlandini <[email protected]>
  */
@@ -24,32 +25,47 @@ final class HttpTransport implements TransportInterface
     private $config;
 
     /**
-     * @var HttpAsyncClient The HTTP client
+     * @var HttpAsyncClientInterface The HTTP client
      */
     private $httpClient;
 
     /**
-     * @var RequestFactory The PSR-7 request factory
+     * @var RequestFactoryInterface The PSR-7 request factory
      */
     private $requestFactory;
 
     /**
-     * @var Promise[] The list of pending requests
+     * @var PromiseInterface[] The list of pending promises
      */
     private $pendingRequests = [];
 
+    /**
+     * @var bool Flag indicating whether the sending of the events should be
+     *           delayed until the shutdown of the application
+     */
+    private $delaySendingUntilShutdown = false;
+
     /**
      * Constructor.
      *
-     * @param Options         $config         The Raven client configuration
-     * @param HttpAsyncClient $httpClient     The HTTP client
-     * @param RequestFactory  $requestFactory The PSR-7 request factory
+     * @param Options                  $config                    The Raven client configuration
+     * @param HttpAsyncClientInterface $httpClient                The HTTP client
+     * @param RequestFactoryInterface  $requestFactory            The PSR-7 request factory
+     * @param bool                     $delaySendingUntilShutdown This flag controls whether to delay
+     *                                                            sending of the events until the shutdown
+     *                                                            of the application. This is a legacy feature
+     *                                                            that will stop working in version 3.0.
      */
-    public function __construct(Options $config, HttpAsyncClient $httpClient, RequestFactory $requestFactory)
+    public function __construct(Options $config, HttpAsyncClientInterface $httpClient, RequestFactoryInterface $requestFactory, bool $delaySendingUntilShutdown = true)
     {
+        if ($delaySendingUntilShutdown) {
+            @trigger_error(sprintf('Delaying the sending of the events using the "%s" class is deprecated since version 2.2 and will not work in 3.0.', __CLASS__), E_USER_DEPRECATED);
+        }
+
         $this->config = $config;
         $this->httpClient = $httpClient;
         $this->requestFactory = $requestFactory;
+        $this->delaySendingUntilShutdown = $delaySendingUntilShutdown;
 
         // By calling the cleanupPendingRequests function from a shutdown function
         // registered inside another shutdown function we can be confident that it
@@ -80,37 +96,23 @@ public function send(Event $event): ?string
 
         $promise = $this->httpClient->sendAsyncRequest($request);
 
-        // This function is defined in-line so it doesn't show up for type-hinting
-        $cleanupPromiseCallback = function ($responseOrException) use ($promise) {
-            $index = array_search($promise, $this->pendingRequests, true);
-
-            if (false !== $index) {
-                unset($this->pendingRequests[$index]);
-            }
-
-            return $responseOrException;
-        };
-
-        $promise->then($cleanupPromiseCallback, $cleanupPromiseCallback);
-
-        $this->pendingRequests[] = $promise;
+        if ($this->delaySendingUntilShutdown) {
+            $this->pendingRequests[] = $promise;
+        } else {
+            $promise->wait(false);
+        }
 
         return $event->getId();
     }
 
     /**
-     * Cleanups the pending requests by forcing them to be sent. Any error that
-     * occurs will be ignored.
+     * Cleanups the pending promises by awaiting for them. Any error that occurs
+     * will be ignored.
      */
     private function cleanupPendingRequests(): void
     {
-        foreach ($this->pendingRequests as $pendingRequest) {
-            try {
-                $pendingRequest->wait();
-            } catch (\Throwable $exception) {
-                // Do nothing because an exception thrown from a destructor
-                // can't be catched in PHP (see http://php.net/manual/en/language.oop5.decon.php#language.oop5.decon.destructor)
-            }
+        while ($promise = array_pop($this->pendingRequests)) {
+            $promise->wait(false);
         }
     }
 }

tests/ClientBuilderTest.php

@@ -29,13 +29,10 @@
 
 final class ClientBuilderTest extends TestCase
 {
-    public function testCreate(): void
-    {
-        $clientBuilder = ClientBuilder::create();
-
-        $this->assertInstanceOf(ClientBuilder::class, $clientBuilder);
-    }
-
+    /**
+     * @group legacy
+     * @expectedDeprecation Delaying the sending of the events using the "Sentry\Transport\HttpTransport" class is deprecated since version 2.2 and will not work in 3.0.
+     */
     public function testHttpTransportIsUsedWhenServeIsConfigured(): void
     {
         $clientBuilder = ClientBuilder::create(['dsn' => 'http://public:[email protected]/sentry/1']);
@@ -65,6 +62,10 @@ public function testSetUriFactory(): void
         $this->assertAttributeSame($uriFactory, 'uriFactory', $clientBuilder);
     }
 
+    /**
+     * @group legacy
+     * @expectedDeprecation Delaying the sending of the events using the "Sentry\Transport\HttpTransport" class is deprecated since version 2.2 and will not work in 3.0.
+     */
     public function testSetMessageFactory(): void
     {
         /** @var MessageFactory|MockObject $messageFactory */
@@ -92,6 +93,10 @@ public function testSetTransport(): void
         $this->assertAttributeSame($transport, 'transport', $clientBuilder->getClient());
     }
 
+    /**
+     * @group legacy
+     * @expectedDeprecation Delaying the sending of the events using the "Sentry\Transport\HttpTransport" class is deprecated since version 2.2 and will not work in 3.0.
+     */
     public function testSetHttpClient(): void
     {
         /** @var HttpAsyncClient|MockObject $httpClient */
@@ -141,6 +146,10 @@ public function testRemoveHttpClientPlugin(): void
         $this->assertSame($plugin2, reset($plugins));
     }
 
+    /**
+     * @group legacy
+     * @expectedDeprecation Delaying the sending of the events using the "Sentry\Transport\HttpTransport" class is deprecated since version 2.2 and will not work in 3.0.
+     */
     public function testGetClient(): void
     {
         $clientBuilder = ClientBuilder::create(['dsn' => 'http://public:[email protected]/sentry/1']);
@@ -258,11 +267,15 @@ public function testClientBuilderSetsSdkIdentifierAndVersion(): void
 
     /**
      * @dataProvider getClientTogglesCompressionPluginInHttpClientDataProvider
+     *
+     * @group legacy
+     * @expectedDeprecation Delaying the sending of the events using the "Sentry\Transport\HttpTransport" class is deprecated since version 2.2 and will not work in 3.0.
      */
     public function testGetClientTogglesCompressionPluginInHttpClient(bool $enabled): void
     {
         $options = new Options(['dsn' => 'http://public:[email protected]/sentry/1']);
         $options->setEnableCompression($enabled);
+
         $builder = new ClientBuilder($options);
         $builder->getClient();
 

tests/ClientTest.php

@@ -4,6 +4,7 @@
 
 namespace Sentry\Tests;
 
+use Http\Discovery\MessageFactoryDiscovery;
 use Http\Mock\Client as MockClient;
 use PHPUnit\Framework\MockObject\MockObject;
 use PHPUnit\Framework\TestCase;
@@ -17,6 +18,7 @@
 use Sentry\Serializer\SerializerInterface;
 use Sentry\Severity;
 use Sentry\Stacktrace;
+use Sentry\Transport\HttpTransport;
 use Sentry\Transport\TransportInterface;
 
 class ClientTest extends TestCase
@@ -268,12 +270,11 @@ public function testSendChecksBeforeSendOption(): void
     public function testSampleRateAbsolute(float $sampleRate): void
     {
         $httpClient = new MockClient();
-
         $options = new Options(['dsn' => 'http://public:[email protected]/1']);
         $options->setSampleRate($sampleRate);
 
         $client = (new ClientBuilder($options))
-            ->setHttpClient($httpClient)
+            ->setTransport(new HttpTransport($options, $httpClient, MessageFactoryDiscovery::find(), false))
             ->getClient();
 
         for ($i = 0; $i < 10; ++$i) {