fix guzzle promise handling (#393)

In certain circumstances (eg Curl handler and middlewares configured such that no additional promise chaining
occurs), our post transfer hook can receive an already-resolved Promise. Since it's already resolved, our
extra callbacks won't be applied in a timely fashion (they're instead added to a queue and will be processed
as part of a shutdown handler, if not triggered by something earlier).
The upshot of this is a lot of memory being held on to for each request, and it seems that the default config
when we use guzzle as our SDK exporter will trigger this. The spans for export are non-recording, so nothing
user-visible happens, but the non-recording spans and promise callbacks are held in memory, seemingly forever
or until shutdown.
The fix for this is to check is the promise is already resolved, and if so force our extra promise to resolve.

Author: brettmc

src/Instrumentation/Guzzle/src/GuzzleInstrumentation.php

@@ -7,6 +7,7 @@
 use function get_cfg_var;
 use GuzzleHttp\Client;
 use GuzzleHttp\Exception\BadResponseException;
+use GuzzleHttp\Promise\Is;
 use GuzzleHttp\Promise\PromiseInterface;
 use OpenTelemetry\API\Globals;
 use OpenTelemetry\API\Instrumentation\CachedInstrumentation;
@@ -101,7 +102,7 @@ public static function register(): void
                     $span->end();
                 }
 
-                $promise->then(
+                $p = $promise->then(
                     onFulfilled: function (ResponseInterface $response) use ($span) {
                         $span->setAttribute(TraceAttributes::HTTP_RESPONSE_STATUS_CODE, $response->getStatusCode());
                         $span->setAttribute(TraceAttributes::NETWORK_PROTOCOL_VERSION, $response->getProtocolVersion());
@@ -117,8 +118,6 @@ public static function register(): void
                             $span->setStatus(StatusCode::STATUS_ERROR);
                         }
                         $span->end();
-
-                        return $response;
                     },
                     onRejected: function (\Throwable $t) use ($span) {
                         if ($t instanceof BadResponseException && $t->hasResponse()) {
@@ -130,10 +129,13 @@ public static function register(): void
                         $span->recordException($t);
                         $span->setStatus(StatusCode::STATUS_ERROR, $t->getMessage());
                         $span->end();
-
-                        throw $t;
                     }
                 );
+
+                //if the original promise is already settled, force our additional promise to execute immediately
+                if (Is::settled($promise)) {
+                    $p->wait();
+                }
             }
         );
     }

src/Instrumentation/Guzzle/tests/Integration/GuzzleInstrumentationTest.php

@@ -9,12 +9,15 @@
 use GuzzleHttp\Exception\ConnectException;
 use GuzzleHttp\Handler\MockHandler;
 use GuzzleHttp\HandlerStack;
+use GuzzleHttp\Promise\FulfilledPromise;
 use GuzzleHttp\Promise\PromiseInterface;
+use GuzzleHttp\Promise\RejectedPromise;
 use GuzzleHttp\Promise\Utils;
 use Nyholm\Psr7\Request;
 use Nyholm\Psr7\Response;
 use OpenTelemetry\API\Instrumentation\Configurator;
 use OpenTelemetry\API\Trace\Propagation\TraceContextPropagator;
+use OpenTelemetry\API\Trace\StatusCode;
 use OpenTelemetry\Context\ScopeInterface;
 use OpenTelemetry\SDK\Trace\ImmutableSpan;
 use OpenTelemetry\SDK\Trace\SpanExporter\InMemoryExporter;
@@ -164,7 +167,58 @@ public function test_headers_propagation(): void
 
         $this->mock->append(new Response());
         $this->client->get('/');
+    }
+
+    /**
+     * @dataProvider promiseProvider
+     * @link https://github.com/open-telemetry/opentelemetry-php/issues/1623
+     */
+    public function test_transfer_promises_execute_for_sync_requests(PromiseInterface $promise, bool $expectedException): void
+    {
+        /**
+         * Approximate the Curl handler's behaviour of returning a FulfilledPromise without any
+         * additional promise chaining, so that post hook receives a FulfilledPromise.
+         */
+        $handler = function () use ($promise): PromiseInterface {
+            return $promise;
+        };
+        $handlerStack = HandlerStack::create($handler);
+
+        $client = new Client([
+            'handler' => $handlerStack,
+            'base_uri' => 'https://example.com/',
+            'http_errors' => false,
+            'allow_redirects' => false,
+        ]);
+        $this->assertCount(0, $this->storage);
+
+        $request = new Request('GET', 'https://example.com/foo');
 
+        try {
+            $client->send($request);
+        } catch (\Throwable $e) {
+            if (!$expectedException) {
+                throw $e;
+            }
+        }
+
+        $this->assertCount(1, $this->storage);
+        $span = $this->storage->offsetGet(0);
+        assert($span instanceof ImmutableSpan);
+        if ($expectedException) {
+            $this->assertSame(StatusCode::STATUS_ERROR, $span->getStatus()->getCode());
+            $this->assertSame('Test exception', $span->getStatus()->getDescription());
+        } else {
+            $this->assertSame(201, $span->getAttributes()->get(TraceAttributes::HTTP_RESPONSE_STATUS_CODE));
+        }
+    }
+
+    public static function promiseProvider(): array
+    {
+        return [
+            'fulfilled' => [new FulfilledPromise(new Response(201)), false],
+            'rejected' => [new RejectedPromise(new \RuntimeException('Test exception')), true],
+        ];
     }
 
     /**