From cac82bbeef9dd80ce60a7735452a22a4f3653e8e Mon Sep 17 00:00:00 2001 From: AleMercadal Date: Thu, 14 Aug 2025 11:46:48 -0300 Subject: [PATCH 1/5] fix(consumer): respect 'timeout' config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * LibCurl: pass 'timeout' to HttpClient constructor (default 10000 ms). * ForkCurl: convert ms→integer seconds (ceil, min 1s) and add --max-time/--connect-timeout only when > 0. --- lib/Consumer/ForkCurl.php | 20 ++++++++++++++++++++ lib/Consumer/LibCurl.php | 3 ++- 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/lib/Consumer/ForkCurl.php b/lib/Consumer/ForkCurl.php index 5a16779..185dc96 100644 --- a/lib/Consumer/ForkCurl.php +++ b/lib/Consumer/ForkCurl.php @@ -32,6 +32,20 @@ public function getConsumer() return $this->type; } + /** + * Converts the configured timeout from milliseconds to integer seconds for curl. + * @return int|null Integer seconds or null for unlimited. + */ + private function timeoutSeconds(): ?int + { + $ms = isset($this->options['timeout']) ? (int)$this->options['timeout'] : 10000; + if ($ms <= 0) { + return null; + } + $seconds = (int)ceil($ms / 1000); + return max(1, $seconds); + } + /** * Make an async request to our API. Fork a curl process, immediately send * to the API. If debug is enabled, we wait for the response. @@ -89,6 +103,12 @@ public function flushBatch($messages) $libVersion = $messages[0]['library_version']; $cmd .= " -H 'User-Agent: $libName/$libVersion'"; + // Timeout + $seconds = $this->timeoutSeconds(); + if ($seconds !== null) { + $cmd .= " --max-time $seconds --connect-timeout $seconds"; + } + if (!$this->debug()) { $cmd .= " > /dev/null 2>&1 &"; } diff --git a/lib/Consumer/LibCurl.php b/lib/Consumer/LibCurl.php index 973e960..e0f90dd 100644 --- a/lib/Consumer/LibCurl.php +++ b/lib/Consumer/LibCurl.php @@ -30,7 +30,8 @@ public function __construct($apiKey, $options = [], ?HttpClient $httpClient = nu $this->maximum_backoff_duration, $this->compress_request, $this->debug(), - $this->options['error_handler'] ?? null + $this->options['error_handler'] ?? null, + $this->options['timeout'] ?? 10000 ); } From 66d44f3a1abaa0e3f721075354d7d29839060d35 Mon Sep 17 00:00:00 2001 From: AleMercadal Date: Thu, 14 Aug 2025 15:42:14 -0300 Subject: [PATCH 2/5] refactor(ForkCurl): add runCommand wrapper * ForkCurl: introduce protected runCommand() and use it instead of exec() (no runtime behavior change). --- lib/Consumer/ForkCurl.php | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/lib/Consumer/ForkCurl.php b/lib/Consumer/ForkCurl.php index 185dc96..00f367a 100644 --- a/lib/Consumer/ForkCurl.php +++ b/lib/Consumer/ForkCurl.php @@ -46,6 +46,11 @@ private function timeoutSeconds(): ?int return max(1, $seconds); } + protected function runCommand(string $cmd, ?array &$output, ?int &$exit): void + { + exec($cmd, $output, $exit); + } + /** * Make an async request to our API. Fork a curl process, immediately send * to the API. If debug is enabled, we wait for the response. @@ -72,7 +77,7 @@ public function flushBatch($messages) // Compress request to file $tmpfname = tempnam("/tmp", "forkcurl_"); $cmd2 = "echo " . $payload . " | gzip > " . $tmpfname; - exec($cmd2, $output, $exit); + $this->runCommand($cmd2, $output, $exit); if (0 != $exit) { $this->handleError($exit, $output); @@ -112,8 +117,8 @@ public function flushBatch($messages) if (!$this->debug()) { $cmd .= " > /dev/null 2>&1 &"; } - - exec($cmd, $output, $exit); + + $this->runCommand($cmd, $output, $exit); if (0 != $exit) { $this->handleError($exit, $output); From 27713c6b473d915fb8582968e5ecfc74e18d2161 Mon Sep 17 00:00:00 2001 From: AleMercadal Date: Thu, 14 Aug 2025 15:44:03 -0300 Subject: [PATCH 3/5] test(consumer): add coverage for timeout config * ForkCurl: asserts --max-time/--connect-timeout generation * LibCurl: asserts HttpClient receives config options --- test/ConsumerForkCurlTest.php | 67 +++++++++++++++++++++++++++++++++ test/ConsumerLibCurlTest.php | 49 ++++++++++++++++++++++++ test/MockedForkCurlConsumer.php | 18 +++++++++ 3 files changed, 134 insertions(+) create mode 100644 test/MockedForkCurlConsumer.php diff --git a/test/ConsumerForkCurlTest.php b/test/ConsumerForkCurlTest.php index 0e0536e..c266e79 100644 --- a/test/ConsumerForkCurlTest.php +++ b/test/ConsumerForkCurlTest.php @@ -4,6 +4,7 @@ use PHPUnit\Framework\TestCase; use PostHog\Client; +use ReflectionClass; class ConsumerForkCurlTest extends TestCase { @@ -60,4 +61,70 @@ public function testAlias(): void ) ); } + + public function testConfigurePositiveTimeout(): void + { + $consumer = new MockedForkCurlConsumer( + 'test_api_key', + array( + 'consumer' => 'fork_curl', + 'debug' => true, + 'timeout' => 1500, + ) + ); + + $rcClient = new ReflectionClass(Client::class); + $prop = $rcClient->getProperty('consumer'); + $prop->setAccessible(true); + $prop->setValue($this->client, $consumer); + + self::assertTrue( + $this->client->capture( + array( + 'distinctId' => 'some-user', + 'event' => "PHP Fork Curl'd\" Event", + ), + ) + ); + + $this->client->flush(); + + self::assertNotEmpty($consumer->commands); + $cmd = end($consumer->commands); + self::assertStringContainsString('--max-time 2', $cmd); + self::assertStringContainsString('--connect-timeout 2', $cmd); + } + + public function testConfigureUnlimitedTimeout(): void + { + $consumer = new MockedForkCurlConsumer( + 'test_api_key', + array( + 'consumer' => 'fork_curl', + 'debug' => true, + 'timeout' => 0, + ) + ); + + $rcClient = new ReflectionClass(Client::class); + $prop = $rcClient->getProperty('consumer'); + $prop->setAccessible(true); + $prop->setValue($this->client, $consumer); + + self::assertTrue( + $this->client->capture( + array( + 'distinctId' => 'some-user', + 'event' => "PHP Fork Curl'd\" Event", + ), + ) + ); + + $this->client->flush(); + + self::assertNotEmpty($consumer->commands); + $cmd = end($consumer->commands); + self::assertStringNotContainsString('max-time', $cmd); + self::assertStringNotContainsString('connect-timeout', $cmd); + } } diff --git a/test/ConsumerLibCurlTest.php b/test/ConsumerLibCurlTest.php index 8c47678..0f8c46a 100644 --- a/test/ConsumerLibCurlTest.php +++ b/test/ConsumerLibCurlTest.php @@ -4,6 +4,8 @@ use PHPUnit\Framework\TestCase; use PostHog\Client; +use PostHog\Consumer\LibCurl; +use ReflectionClass; class ConsumerLibCurlTest extends TestCase { @@ -21,6 +23,53 @@ public function setUp(): void ); } + public function testApplyConstructOptionsToHttpClient() + { + $client = new Client( + 'test_api_key', + array( + 'consumer' => 'lib_curl', + 'ssl' => false, + 'maximum_backoff_duration' => 5000, + 'compress_request' => true, + 'debug' => true, + 'timeout' => 1234, + ) + ); + + $rcClient = new ReflectionClass($client); + $consumerProp = $rcClient->getProperty('consumer'); + $consumerProp->setAccessible(true); + $consumer = $consumerProp->getValue($client); + + $this->assertInstanceOf(LibCurl::class, $consumer); + + $rcConsumer = new ReflectionClass($consumer); + $httpProp = $rcConsumer->getProperty('httpClient'); + $httpProp->setAccessible(true); + $httpClient = $httpProp->getValue($consumer); + + $rcHttp = new ReflectionClass($httpClient); + + $expectedValues = array( + 'useSsl' => false, + 'maximumBackoffDuration' => 5000, + 'compressRequests' => true, + 'debug' => true, + 'errorHandler' => null, + 'curlTimeoutMilliseconds' => 1234, + ); + + foreach ($expectedValues as $name => $expected){ + self::assertTrue($rcHttp->hasProperty($name)); + + $prop = $rcHttp->getProperty($name); + $prop->setAccessible(true); + $actual = $prop->getValue($httpClient); + self::assertSame($expected, $actual); + } + } + public function testCapture(): void { self::assertTrue( diff --git a/test/MockedForkCurlConsumer.php b/test/MockedForkCurlConsumer.php new file mode 100644 index 0000000..efd0e02 --- /dev/null +++ b/test/MockedForkCurlConsumer.php @@ -0,0 +1,18 @@ +commands[] = $cmd; + $output = []; + $exit = $this->fakeExit; + } +} From 00af3b3367effa0fb1d4a08d87e5751a59fc9ffd Mon Sep 17 00:00:00 2001 From: AleMercadal Date: Mon, 1 Sep 2025 09:40:50 -0300 Subject: [PATCH 4/5] refactor(http): centralize default cURL timeout in HttpClient constant Introduced HttpClient::DEFAULT_CURL_TIMEOUT and replaced hardcoded 10000ms timeout values in Client, ForkCurl, and LibCurl with the new constant. This improves maintainability by centralizing the default timeout configuration. --- lib/Client.php | 2 +- lib/Consumer/ForkCurl.php | 3 ++- lib/Consumer/LibCurl.php | 2 +- lib/HttpClient.php | 4 +++- 4 files changed, 7 insertions(+), 4 deletions(-) diff --git a/lib/Client.php b/lib/Client.php index 147859d..744beca 100644 --- a/lib/Client.php +++ b/lib/Client.php @@ -94,7 +94,7 @@ public function __construct( false, $options["debug"] ?? false, null, - (int) ($options['timeout'] ?? 10000) + (int) ($options['timeout'] ?? HttpClient::DEFAULT_CURL_TIMEOUT) ); $this->featureFlagsRequestTimeout = (int) ($options['feature_flag_request_timeout_ms'] ?? 3000); $this->featureFlags = []; diff --git a/lib/Consumer/ForkCurl.php b/lib/Consumer/ForkCurl.php index 00f367a..b0c2fb8 100644 --- a/lib/Consumer/ForkCurl.php +++ b/lib/Consumer/ForkCurl.php @@ -2,6 +2,7 @@ namespace PostHog\Consumer; +use PostHog\HttpClient; use PostHog\QueueConsumer; class ForkCurl extends QueueConsumer @@ -38,7 +39,7 @@ public function getConsumer() */ private function timeoutSeconds(): ?int { - $ms = isset($this->options['timeout']) ? (int)$this->options['timeout'] : 10000; + $ms = isset($this->options['timeout']) ? (int)$this->options['timeout'] : HttpClient::DEFAULT_CURL_TIMEOUT; if ($ms <= 0) { return null; } diff --git a/lib/Consumer/LibCurl.php b/lib/Consumer/LibCurl.php index e0f90dd..84aca3c 100644 --- a/lib/Consumer/LibCurl.php +++ b/lib/Consumer/LibCurl.php @@ -31,7 +31,7 @@ public function __construct($apiKey, $options = [], ?HttpClient $httpClient = nu $this->compress_request, $this->debug(), $this->options['error_handler'] ?? null, - $this->options['timeout'] ?? 10000 + $this->options['timeout'] ?? HttpClient::DEFAULT_CURL_TIMEOUT ); } diff --git a/lib/HttpClient.php b/lib/HttpClient.php index 7ca81f7..28a2c23 100644 --- a/lib/HttpClient.php +++ b/lib/HttpClient.php @@ -6,6 +6,8 @@ class HttpClient { + public const DEFAULT_CURL_TIMEOUT = 10000; + /** * @var string */ @@ -47,7 +49,7 @@ public function __construct( bool $compressRequests = false, bool $debug = false, ?Closure $errorHandler = null, - int $curlTimeoutMilliseconds = 10000 + int $curlTimeoutMilliseconds = self::DEFAULT_CURL_TIMEOUT ) { $this->host = $host; $this->useSsl = $useSsl; From c3dced514b11c24d3e6968c68d991b05ee028d94 Mon Sep 17 00:00:00 2001 From: AleMercadal Date: Mon, 1 Sep 2025 09:42:21 -0300 Subject: [PATCH 5/5] style(tests): fix spacing in foreach loop in ConsumerLibCurlTest Corrected the spacing in the foreach loop declaration in ConsumerLibCurlTest.php. --- test/ConsumerLibCurlTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/ConsumerLibCurlTest.php b/test/ConsumerLibCurlTest.php index 0f8c46a..3445c0a 100644 --- a/test/ConsumerLibCurlTest.php +++ b/test/ConsumerLibCurlTest.php @@ -60,7 +60,7 @@ public function testApplyConstructOptionsToHttpClient() 'curlTimeoutMilliseconds' => 1234, ); - foreach ($expectedValues as $name => $expected){ + foreach ($expectedValues as $name => $expected) { self::assertTrue($rcHttp->hasProperty($name)); $prop = $rcHttp->getProperty($name);