From 1198e433cda24282536f29326a196c1b8c9f0ed4 Mon Sep 17 00:00:00 2001 From: Goran Stamenkovski Date: Mon, 18 May 2020 14:39:27 +0200 Subject: [PATCH 1/5] Use standard request timout with progress callback ISSUE MAIN-466 --- CHANGELOG.md | 7 ++ .../Configuration/Configuration.php | 20 +++++ src/Infrastructure/Http/CurlHttpClient.php | 15 +++- .../Http/CurlHttpClientTest.php | 89 ++++++++++++++++++- 4 files changed, 125 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5e1d9422..29ca74d3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,13 @@ 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) +### 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 diff --git a/src/Infrastructure/Configuration/Configuration.php b/src/Infrastructure/Configuration/Configuration.php index 0a33b070..1a836a88 100644 --- a/src/Infrastructure/Configuration/Configuration.php +++ b/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. * diff --git a/src/Infrastructure/Http/CurlHttpClient.php b/src/Infrastructure/Http/CurlHttpClient.php index f1b01a2f..699ededd 100644 --- a/src/Infrastructure/Http/CurlHttpClient.php +++ b/src/Infrastructure/Http/CurlHttpClient.php @@ -279,10 +279,17 @@ 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 sync request timeout by default if progress callback mechanism is used for async request aborting. + // We do not need/want fast timeouts in this case because the abort mechanism relays on the amount of + // uploaded data. + $this->curlOptions[CURLOPT_TIMEOUT_MS] = $asyncRequestTimeout ?: static::DEFAULT_REQUEST_TIMEOUT; + $this->curlOptions[CURLOPT_NOPROGRESS] = false; + $this->curlOptions[CURLOPT_PROGRESSFUNCTION] = array($this, 'abortAfterAsyncRequestCallback'); + } } /** diff --git a/tests/Infrastructure/Http/CurlHttpClientTest.php b/tests/Infrastructure/Http/CurlHttpClientTest.php index 07bc4484..581fa901 100644 --- a/tests/Infrastructure/Http/CurlHttpClientTest.php +++ b/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_REQUEST_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.' + ); + } } From 8419873c35a92f7f84acb1b050206b2d489222c0 Mon Sep 17 00:00:00 2001 From: Goran Stamenkovski Date: Mon, 18 May 2020 16:55:54 +0200 Subject: [PATCH 2/5] Uniform async requests timout default value ISSUE MAIN-466 --- src/Infrastructure/Http/CurlHttpClient.php | 12 +++++------- tests/Infrastructure/Http/CurlHttpClientTest.php | 2 +- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/Infrastructure/Http/CurlHttpClient.php b/src/Infrastructure/Http/CurlHttpClient.php index 699ededd..c043a455 100644 --- a/src/Infrastructure/Http/CurlHttpClient.php +++ b/src/Infrastructure/Http/CurlHttpClient.php @@ -278,15 +278,13 @@ 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. - $asyncRequestTimeout = $this->getConfigService()->getAsyncRequestTimeout(); - $this->curlOptions[CURLOPT_TIMEOUT_MS] = $asyncRequestTimeout ?: static::DEFAULT_ASYNC_REQUEST_TIMEOUT; + // Timeout super fast once connected, so it goes into async. Pay attention that fast timout is not desired when + // callback progress method is used for request aborting because request can go in timout before request + // upload finishes therefore making async requests less stable that it needs to be. + $this->curlOptions[CURLOPT_TIMEOUT_MS] = + $this->getConfigService()->getAsyncRequestTimeout() ?: static::DEFAULT_ASYNC_REQUEST_TIMEOUT; if ($this->getConfigService()->isAsyncRequestWithProgress()) { - // Use sync request timeout by default if progress callback mechanism is used for async request aborting. - // We do not need/want fast timeouts in this case because the abort mechanism relays on the amount of - // uploaded data. - $this->curlOptions[CURLOPT_TIMEOUT_MS] = $asyncRequestTimeout ?: static::DEFAULT_REQUEST_TIMEOUT; $this->curlOptions[CURLOPT_NOPROGRESS] = false; $this->curlOptions[CURLOPT_PROGRESSFUNCTION] = array($this, 'abortAfterAsyncRequestCallback'); } diff --git a/tests/Infrastructure/Http/CurlHttpClientTest.php b/tests/Infrastructure/Http/CurlHttpClientTest.php index 581fa901..33c0a0d5 100644 --- a/tests/Infrastructure/Http/CurlHttpClientTest.php +++ b/tests/Infrastructure/Http/CurlHttpClientTest.php @@ -103,7 +103,7 @@ public function testAsyncCallWithProgressCallback() $this->httpClient->requestAsync('POST', 'test.url.com'); $this->assertProgressCallback(); - $this->assertCallTimeout(CurlHttpClient::DEFAULT_REQUEST_TIMEOUT); + $this->assertCallTimeout(CurlHttpClient::DEFAULT_ASYNC_REQUEST_TIMEOUT); } /** From 96ca7316f1837ce3be3e9a310299c913d2d4e456 Mon Sep 17 00:00:00 2001 From: Goran Stamenkovski Date: Tue, 19 May 2020 09:26:15 +0200 Subject: [PATCH 3/5] Change default timeout for progress callback ISSUE MAIN-466 --- src/Infrastructure/Configuration/Configuration.php | 11 ++++++++++- tests/Infrastructure/Http/CurlHttpClientTest.php | 3 ++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/Infrastructure/Configuration/Configuration.php b/src/Infrastructure/Configuration/Configuration.php index 1a836a88..d2bf86c8 100644 --- a/src/Infrastructure/Configuration/Configuration.php +++ b/src/Infrastructure/Configuration/Configuration.php @@ -38,6 +38,10 @@ abstract class Configuration extends Singleton * Default HTTP method to use for async call. */ const ASYNC_CALL_METHOD = 'POST'; + /** + * Default asynchronous request timeout value in milliseconds when progress callback is used. + */ + const DEFAULT_ASYNC_REQUEST_WITH_PROGRESS_TIMEOUT = 60000; /** * System user context. * @@ -438,7 +442,12 @@ public function setSyncRequestTimeout($timeout) */ public function getAsyncRequestTimeout() { - return $this->getConfigValue('asyncRequestTimeout'); + $configValue = $this->getConfigValue('asyncRequestTimeout'); + if ($configValue || !$this->isAsyncRequestWithProgress()) { + return $configValue; + } + + return static::DEFAULT_ASYNC_REQUEST_WITH_PROGRESS_TIMEOUT; } /** diff --git a/tests/Infrastructure/Http/CurlHttpClientTest.php b/tests/Infrastructure/Http/CurlHttpClientTest.php index 33c0a0d5..421f9589 100644 --- a/tests/Infrastructure/Http/CurlHttpClientTest.php +++ b/tests/Infrastructure/Http/CurlHttpClientTest.php @@ -2,6 +2,7 @@ namespace Logeecom\Tests\Infrastructure\Http; +use Logeecom\Infrastructure\Configuration\Configuration; use Logeecom\Infrastructure\Http\CurlHttpClient; use Logeecom\Infrastructure\Http\HttpClient; use Logeecom\Tests\Infrastructure\Common\BaseInfrastructureTestWithServices; @@ -103,7 +104,7 @@ public function testAsyncCallWithProgressCallback() $this->httpClient->requestAsync('POST', 'test.url.com'); $this->assertProgressCallback(); - $this->assertCallTimeout(CurlHttpClient::DEFAULT_ASYNC_REQUEST_TIMEOUT); + $this->assertCallTimeout(Configuration::DEFAULT_ASYNC_REQUEST_WITH_PROGRESS_TIMEOUT); } /** From 8d4a0af63f6db98c4a2668f800e47b8332477f7e Mon Sep 17 00:00:00 2001 From: Goran Stamenkovski Date: Tue, 19 May 2020 15:57:13 +0200 Subject: [PATCH 4/5] Move timout constant to http client ISSUE MAIN-466 --- .../Configuration/Configuration.php | 11 +---------- src/Infrastructure/Http/CurlHttpClient.php | 19 ++++++++++++++----- .../Http/CurlHttpClientTest.php | 3 +-- 3 files changed, 16 insertions(+), 17 deletions(-) diff --git a/src/Infrastructure/Configuration/Configuration.php b/src/Infrastructure/Configuration/Configuration.php index d2bf86c8..1a836a88 100644 --- a/src/Infrastructure/Configuration/Configuration.php +++ b/src/Infrastructure/Configuration/Configuration.php @@ -38,10 +38,6 @@ abstract class Configuration extends Singleton * Default HTTP method to use for async call. */ const ASYNC_CALL_METHOD = 'POST'; - /** - * Default asynchronous request timeout value in milliseconds when progress callback is used. - */ - const DEFAULT_ASYNC_REQUEST_WITH_PROGRESS_TIMEOUT = 60000; /** * System user context. * @@ -442,12 +438,7 @@ public function setSyncRequestTimeout($timeout) */ public function getAsyncRequestTimeout() { - $configValue = $this->getConfigValue('asyncRequestTimeout'); - if ($configValue || !$this->isAsyncRequestWithProgress()) { - return $configValue; - } - - return static::DEFAULT_ASYNC_REQUEST_WITH_PROGRESS_TIMEOUT; + return $this->getConfigValue('asyncRequestTimeout'); } /** diff --git a/src/Infrastructure/Http/CurlHttpClient.php b/src/Infrastructure/Http/CurlHttpClient.php index c043a455..9f428332 100644 --- a/src/Infrastructure/Http/CurlHttpClient.php +++ b/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. */ @@ -278,13 +282,18 @@ 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. Pay attention that fast timout is not desired when - // callback progress method is used for request aborting because request can go in timout before request - // upload finishes therefore making async requests less stable that it needs to be. - $this->curlOptions[CURLOPT_TIMEOUT_MS] = - $this->getConfigService()->getAsyncRequestTimeout() ?: static::DEFAULT_ASYNC_REQUEST_TIMEOUT; + // Timeout super fast once connected, so it goes into async. + $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'); } diff --git a/tests/Infrastructure/Http/CurlHttpClientTest.php b/tests/Infrastructure/Http/CurlHttpClientTest.php index 421f9589..6626281a 100644 --- a/tests/Infrastructure/Http/CurlHttpClientTest.php +++ b/tests/Infrastructure/Http/CurlHttpClientTest.php @@ -2,7 +2,6 @@ namespace Logeecom\Tests\Infrastructure\Http; -use Logeecom\Infrastructure\Configuration\Configuration; use Logeecom\Infrastructure\Http\CurlHttpClient; use Logeecom\Infrastructure\Http\HttpClient; use Logeecom\Tests\Infrastructure\Common\BaseInfrastructureTestWithServices; @@ -104,7 +103,7 @@ public function testAsyncCallWithProgressCallback() $this->httpClient->requestAsync('POST', 'test.url.com'); $this->assertProgressCallback(); - $this->assertCallTimeout(Configuration::DEFAULT_ASYNC_REQUEST_WITH_PROGRESS_TIMEOUT); + $this->assertCallTimeout(CurlHttpClient::DEFAULT_ASYNC_REQUEST_WITH_PROGRESS_TIMEOUT); } /** From fca38dbbc67c19c03e52aa12bcd87a6a0bf00186 Mon Sep 17 00:00:00 2001 From: "predrag.krstojevic" Date: Tue, 19 May 2020 17:43:01 +0200 Subject: [PATCH 5/5] Prepare 2.0.10 release --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 29ca74d3..c3b4e11d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,7 +3,7 @@ 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`)