Merge pull request #31 from logeecom/dev

Optimize HttpClient

CHANGELOG.md

@@ -3,7 +3,14 @@ All notable changes to this project will be documented in this file.
 
 The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
 
-## [Unreleased](https://github.com/packlink-dev/ecommerce_module_core/compare/master...dev)
+## [2.0.10](https://github.com/packlink-dev/ecommerce_module_core/compare/v2.0.9...v2.0.10)
+### Added
+- Added configuration flag for async request progress callback usage (methods
+`Configuration::isAsyncRequestWithProgress` and `Configuration::setAsyncRequestWithProgress`)
+
+### Changed
+- Changed default timeout for async requests that utilize progress callback to be the same as for standard sync request.
+In this case progress callback is controlling request abort based on uploaded data so short timeout is undesirable.
 
 ## [2.0.9](https://github.com/packlink-dev/ecommerce_module_core/compare/v2.0.8...v2.0.9) - 2020-05-15
 ### Added

src/Infrastructure/Configuration/Configuration.php

@@ -451,6 +451,26 @@ public function setAsyncRequestTimeout($timeout)
         $this->saveConfigValue('asyncRequestTimeout', $timeout);
     }
 
+    /**
+     * Returns whether the async requests are aborted with progress callback (used in CurlHttpClient).
+     *
+     * @return bool TRUE if the async requests are aborted with progress callback; otherwise, FALSE.
+     */
+    public function isAsyncRequestWithProgress()
+    {
+        return (bool)$this->getConfigValue('asyncRequestWithProgress', false);
+    }
+
+    /**
+     * Sets flag for usage of progress callback abort mechanism inside CurlHttpClient
+     *
+     * @param bool $withProgress
+     */
+    public function setAsyncRequestWithProgress($withProgress)
+    {
+        $this->saveConfigValue('asyncRequestWithProgress', $withProgress);
+    }
+
     /**
      * Gets configuration value for given name.
      *

src/Infrastructure/Http/CurlHttpClient.php

@@ -17,6 +17,10 @@ class CurlHttpClient extends HttpClient
      * Default asynchronous request timeout value in milliseconds.
      */
     const DEFAULT_ASYNC_REQUEST_TIMEOUT = 1000;
+    /**
+     * Default asynchronous request timeout value in milliseconds when progress callback is used.
+     */
+    const DEFAULT_ASYNC_REQUEST_WITH_PROGRESS_TIMEOUT = 60000;
     /**
      * Default synchronous request timeout value in milliseconds.
      */
@@ -279,10 +283,20 @@ protected function setCurlSessionOptionsForAsynchronousRequest()
         // Always ensure the connection is fresh.
         $this->curlOptions[CURLOPT_FRESH_CONNECT] = true;
         // Timeout super fast once connected, so it goes into async.
-        $this->curlOptions[CURLOPT_TIMEOUT_MS] =
-            $this->getConfigService()->getAsyncRequestTimeout() ?: static::DEFAULT_ASYNC_REQUEST_TIMEOUT;
-        $this->curlOptions[CURLOPT_NOPROGRESS] = false;
-        $this->curlOptions[CURLOPT_PROGRESSFUNCTION] = array($this, 'abortAfterAsyncRequestCallback');
+        $asyncRequestTimeout = $this->getConfigService()->getAsyncRequestTimeout();
+        $this->curlOptions[CURLOPT_TIMEOUT_MS] = $asyncRequestTimeout ?: static::DEFAULT_ASYNC_REQUEST_TIMEOUT;
+
+        if ($this->getConfigService()->isAsyncRequestWithProgress()) {
+            // Use higher request timeout value by default if progress callback mechanism is used for async request
+            // aborting. Pay attention that fast timout is not desired in this case because request can go in timout
+            // before request upload finishes, therefore, making async requests less stable that it needs to be.
+            if (!$asyncRequestTimeout) {
+                $this->curlOptions[CURLOPT_TIMEOUT_MS] = static::DEFAULT_ASYNC_REQUEST_WITH_PROGRESS_TIMEOUT;
+            }
+
+            $this->curlOptions[CURLOPT_NOPROGRESS] = false;
+            $this->curlOptions[CURLOPT_PROGRESSFUNCTION] = array($this, 'abortAfterAsyncRequestCallback');
+        }
     }
 
     /**

tests/Infrastructure/Http/CurlHttpClientTest.php

@@ -57,7 +57,7 @@ public function testSyncCallWithDifferentTimeout()
     /**
      * Test an async call.
      */
-    public function testAsyncCall()
+    public function testDefaultAsyncCall()
     {
         $responses = array($this->getResponse(200));
 
@@ -78,7 +78,7 @@ public function testAsyncCall()
     /**
      * Test an async call with custom timeout.
      */
-    public function testAsyncCallDifferentTimeout()
+    public function testDefaultAsyncCallDifferentTimeout()
     {
         $responses = array($this->getResponse(200));
 
@@ -91,6 +91,54 @@ public function testAsyncCallDifferentTimeout()
         $this->assertCallTimeout($newTimeout);
     }
 
+    /**
+     * Test async call with progress callback
+     */
+    public function testAsyncCallWithProgressCallback()
+    {
+        $responses = array($this->getResponse(200));
+        $this->httpClient->setMockResponses($responses);
+
+        $this->shopConfig->setAsyncRequestWithProgress(true);
+        $this->httpClient->requestAsync('POST', 'test.url.com');
+
+        $this->assertProgressCallback();
+        $this->assertCallTimeout(CurlHttpClient::DEFAULT_ASYNC_REQUEST_WITH_PROGRESS_TIMEOUT);
+    }
+
+    /**
+     * Test async call without progress callback
+     */
+    public function testAsyncCallWithoutProgressCallback()
+    {
+        $responses = array($this->getResponse(200));
+
+        $this->httpClient->setMockResponses($responses);
+
+        $this->shopConfig->setAsyncRequestWithProgress(false);
+        $this->httpClient->requestAsync('POST', 'test.url.com');
+
+        $this->assertProgressCallback(false);
+        $this->assertCallTimeout(CurlHttpClient::DEFAULT_ASYNC_REQUEST_TIMEOUT);
+    }
+
+    /**
+     * Test an async call with custom timeout and progress callback.
+     */
+    public function testDAsyncCallWithProgressCallbackDifferentTimeout()
+    {
+        $responses = array($this->getResponse(200));
+
+        $this->httpClient->setMockResponses($responses);
+
+        $newTimeout = 200;
+        $this->shopConfig->setAsyncRequestWithProgress(true);
+        $this->shopConfig->setAsyncRequestTimeout($newTimeout);
+        $this->httpClient->requestAsync('POST', 'test.url.com');
+
+        $this->assertCallTimeout($newTimeout);
+    }
+
     /**
      * Test parsing plain text response.
      */
@@ -181,4 +229,41 @@ private function assertCallTimeout($timeout)
             'Curl default timeout should be set for async call.'
         );
     }
+
+    private function assertProgressCallback($isOn = true)
+    {
+        $curlOptions = $this->httpClient->getCurlOptions();
+        $this->assertNotEmpty($curlOptions, 'Curl options should be set.');
+        if (!$isOn) {
+            $this->assertFalse(
+                isset($curlOptions[CURLOPT_NOPROGRESS]),
+                'Curl progress callback should not be set.'
+            );
+            $this->assertFalse(
+                isset($curlOptions[CURLOPT_PROGRESSFUNCTION]),
+                'Curl progress callback should not be set.'
+            );
+
+            return;
+        }
+
+
+        $this->assertTrue(
+            isset($curlOptions[CURLOPT_NOPROGRESS]),
+            'Curl progress callback should be set for async call.'
+        );
+        $this->assertFalse(
+            $curlOptions[CURLOPT_NOPROGRESS],
+            'Curl progress callback should be set for async call.'
+        );
+        $this->assertTrue(
+            isset($curlOptions[CURLOPT_PROGRESSFUNCTION]),
+            'Curl progress callback should be set for async call.'
+        );
+        $this->assertEquals(
+            array($this->httpClient, 'abortAfterAsyncRequestCallback'),
+            $curlOptions[CURLOPT_PROGRESSFUNCTION],
+            'Curl progress callback should be set for async call.'
+        );
+    }
 }