fix: reuse the MyEP api connection instead of a client per call

public/typo3conf/ext/ep_products/Classes/MyEP/ApiClient.php

@@ -49,8 +49,22 @@ class ApiClient implements LoggerAwareInterface
      */
     private const ACCESS_TOKEN_DEFAULT_TTL = 300;
 
+    /**
+     * Status codes that mean the token was rejected rather than the request being wrong.
+     */
+    private const UNAUTHORIZED_STATUS_CODES = [Response::HTTP_UNAUTHORIZED, Response::HTTP_FORBIDDEN];
+
     private static ?AccessToken $accessToken = null;
 
+    /**
+     * Shared on purpose. Symfony's CurlHttpClient keeps its connection pool and TLS session
+     * cache in a CurlClientState owned by the instance, so building a client per call throws
+     * both away and forces a fresh DNS lookup, TCP connect and TLS handshake every time -
+     * TravelinfoController alone makes two calls per page render. The bearer token is passed
+     * per request rather than as a client default so one instance can outlive one token.
+     */
+    private static ?HttpClientInterface $httpClient = null;
+
     private ?FrontendInterface $cache = null;
 
     private ?array $configuration = null;
@@ -103,6 +117,7 @@ class ApiClient implements LoggerAwareInterface
         // A conflict means the address is already registered, which is the normal answer to a
         // resubmitted contact form and not worth reporting.
         $this->assertStatusCode($response, [Response::HTTP_OK, Response::HTTP_CREATED, Response::HTTP_ACCEPTED, Response::HTTP_NO_CONTENT, Response::HTTP_CONFLICT], 'POST', 'contactform');
+        $this->drainResponse($response);
     }
 
     /**
@@ -117,6 +132,7 @@ class ApiClient implements LoggerAwareInterface
         // A conflict means the submission is already known, which is the normal answer to a
         // double-clicked submit button and not worth reporting.
         $this->assertStatusCode($response, [Response::HTTP_OK, Response::HTTP_CREATED, Response::HTTP_ACCEPTED, Response::HTTP_NO_CONTENT, Response::HTTP_CONFLICT], 'POST', 'applications');
+        $this->drainResponse($response);
     }
 
     /**
@@ -199,6 +215,30 @@ class ApiClient implements LoggerAwareInterface
      */
     private function request(string $method, string $uri, array $options = []): ResponseInterface
     {
+        $response = $this->sendRequest($method, $uri, $options);
+
+        if (false === in_array($this->readStatusCode($response, $method, $uri), self::UNAUTHORIZED_STATUS_CODES, true)) {
+            return $response;
+        }
+
+        // The token outlives a single PHP request, so one the API has stopped accepting - after
+        // a MyEP restart or a revoked client - would keep failing every call for the rest of its
+        // cached lifetime unless it is actively discarded here.
+        $this->logWarning(sprintf('MyEP API rejected the cached access token for %s %s, re-authenticating once', $method, $uri));
+        $this->discardAccessToken();
+
+        return $this->sendRequest($method, $uri, $options);
+    }
+
+    /**
+     * @throws ApiException
+     */
+    private function sendRequest(string $method, string $uri, array $options): ResponseInterface
+    {
+        // Resolved here rather than baked into the client's default options so that the shared
+        // client survives a token change, and so the retry above picks up the fresh token.
+        $options['auth_bearer'] = $this->getAccessToken($this->getConfiguration())->getToken();
+
         try {
             return $this->getHttpClient()->request($method, $uri, $options);
         } catch (TransportExceptionInterface $e) {
@@ -215,13 +255,7 @@ class ApiClient implements LoggerAwareInterface
      */
     private function assertStatusCode(ResponseInterface $response, array $expectedStatusCodes, string $method, string $uri): void
     {
-        try {
+        $statusCode = $this->readStatusCode($response, $method, $uri);
-            $statusCode = $response->getStatusCode();
-        } catch (TransportExceptionInterface $e) {
-            $message = sprintf('MyEP API transport error while reading status for %s %s: %s', $method, $uri, $e->getMessage());
-            $this->logError($message, ['exception' => $e]);
-            throw new ApiException($message, 0, $e);
-        }
 
         if (true === in_array($statusCode, $expectedStatusCodes, true)) {
             return;
@@ -233,6 +267,39 @@ class ApiClient implements LoggerAwareInterface
         throw new ApiException($message, $statusCode);
     }
 
+    /**
+     * Reading the status completes the transfer, so this is also what turns the lazy response
+     * returned by the client into a finished request. Symfony keeps the status on the response,
+     * so calling this twice costs nothing.
+     *
+     * @throws ApiException
+     */
+    private function readStatusCode(ResponseInterface $response, string $method, string $uri): int
+    {
+        try {
+            return $response->getStatusCode();
+        } catch (TransportExceptionInterface $e) {
+            $message = sprintf('MyEP API transport error while reading status for %s %s: %s', $method, $uri, $e->getMessage());
+            $this->logError($message, ['exception' => $e]);
+            throw new ApiException($message, 0, $e);
+        }
+    }
+
+    /**
+     * Reads and discards a body nobody else is going to look at. Destroying an unconsumed
+     * response cancels the transfer, which closes the connection instead of returning it to
+     * the pool, so the next call would have to handshake again.
+     */
+    private function drainResponse(ResponseInterface $response): void
+    {
+        try {
+            $response->getContent(false);
+        } catch (TransportExceptionInterface $e) {
+            // The status has already been asserted, so a failure here costs nothing but the
+            // connection reuse this method exists for.
+        }
+    }
+
     /**
      * @throws ApiException
      */
@@ -253,13 +320,13 @@ class ApiClient implements LoggerAwareInterface
 
     private function getHttpClient(): HttpClientInterface
     {
+        if (null !== static::$httpClient) {
+            return static::$httpClient;
+        }
+
         $config = $this->getConfiguration();
 
-        static::$accessToken = $this->getAccessToken($config);
+        $options = [];
-
-        $options = [
-            'auth_bearer' => static::$accessToken->getToken(),
-        ];
 
         // Without these the client falls back to PHP's default_socket_timeout (commonly 60s),
         // so a stalled API pins a PHP worker for a minute per request. The defaults live in
@@ -277,7 +344,7 @@ class ApiClient implements LoggerAwareInterface
             $options['max_duration'] = $maxDuration;
         }
 
-        return HttpClient::createForBaseUri($config['myEpApiBaseUrl'], $options);
+        return static::$httpClient = HttpClient::createForBaseUri($config['myEpApiBaseUrl'], $options);
     }
 
     /**
@@ -312,7 +379,7 @@ class ApiClient implements LoggerAwareInterface
      */
     private function getAccessToken(array $config): AccessToken
     {
-        if (null !== static::$accessToken && false === static::$accessToken->hasExpired()) {
+        if (true === $this->isTokenUsable(static::$accessToken)) {
             return static::$accessToken;
         }
 
@@ -321,7 +388,7 @@ class ApiClient implements LoggerAwareInterface
         $cachedToken = $this->getCache()->get(self::ACCESS_TOKEN_CACHE_IDENTIFIER);
         if (is_array($cachedToken) && isset($cachedToken['access_token'])) {
             $token = new AccessToken($cachedToken);
-            if (false === $token->hasExpired()) {
+            if (true === $this->isTokenUsable($token)) {
                 static::$accessToken = $token;
 
                 return static::$accessToken;
@@ -329,7 +396,7 @@ class ApiClient implements LoggerAwareInterface
         }
 
         try {
-            static::$accessToken = $this->getProvider($config)->getAccessToken('client_credentials');
+            static::$accessToken = $this->normalizeToken($this->getProvider($config)->getAccessToken('client_credentials'));
             $this->cacheAccessToken(static::$accessToken);
         } catch (IdentityProviderException $e) {
             $message = sprintf('MyEP OAuth token request failed: %s', $e->getMessage());
@@ -348,16 +415,53 @@ class ApiClient implements LoggerAwareInterface
         return static::$accessToken;
     }
 
-    private function cacheAccessToken(AccessToken $token): void
+    /**
+     * AccessToken::hasExpired() throws for a token that reports no expiry, so it cannot be used
+     * as the check - a token without one is simply treated as unusable and replaced. The same
+     * margin as the cache lifetime is applied on read as well, so a token cannot lapse between
+     * this check and the API call that uses it, whichever node wrote the cache entry.
+     */
+    private function isTokenUsable(?AccessToken $token): bool
     {
+        if (null === $token) {
+            return false;
+        }
+
         $expires = $token->getExpires();
 
         if (null === $expires) {
-            $lifetime = self::ACCESS_TOKEN_DEFAULT_TTL;
+            return false;
-        } else {
-            $lifetime = $expires - time() - self::ACCESS_TOKEN_EXPIRY_MARGIN;
         }
 
+        return $expires - self::ACCESS_TOKEN_EXPIRY_MARGIN > time();
+    }
+
+    /**
+     * A token that reports no expiry cannot be validated on read - isTokenUsable() rejects it
+     * and AccessToken::hasExpired() throws for it - so it is given the fallback lifetime here,
+     * once, and the rest of the class can treat every token the same way.
+     */
+    private function normalizeToken(AccessToken $token): AccessToken
+    {
+        if (null !== $token->getExpires()) {
+            return $token;
+        }
+
+        return new AccessToken($token->jsonSerialize() + ['expires_in' => self::ACCESS_TOKEN_DEFAULT_TTL]);
+    }
+
+    private function discardAccessToken(): void
+    {
+        static::$accessToken = null;
+        $this->getCache()->remove(self::ACCESS_TOKEN_CACHE_IDENTIFIER);
+    }
+
+    private function cacheAccessToken(AccessToken $token): void
+    {
+        // normalizeToken() has already given a token without a reported expiry the fallback
+        // lifetime, so there is always a real expiry to subtract the margin from here.
+        $lifetime = $token->getExpires() - time() - self::ACCESS_TOKEN_EXPIRY_MARGIN;
+
         if ($lifetime <= 0) {
             return;
         }