diff --git a/lib/php/README.md b/lib/php/README.md index 28841d36126..e004cd7a0b1 100644 --- a/lib/php/README.md +++ b/lib/php/README.md @@ -59,6 +59,8 @@ apcu_fetch(), apcu_store() 2. `TCurlClient` follows a redirect only within the origin of the URL it is configured with, that is to the same scheme, host and port. It sends the request again there, with its headers and body, and follows at most one redirect, as before. A redirect to another origin, including one from `http` to `https`, now fails the request with a `TTransportException`, as any redirect does with `THttpClient`. Configure the client with the scheme, host and port that serve the requests. +3. Each `TCurlClient` now owns its curl handle, so timeout settings and connection reuse are independent between client instances. `close()` releases that client's handle. Replace static calls to `TCurlClient::closeCurlHandle()` with `$client->closeCurlHandle()` to release a specific client's handle, or `$client->close()` to clear its buffers as well. + ## 0.25.0 1. `TBinaryProtocol`, `TBinaryProtocolAccelerated` and `TCompactProtocol` now refuse a string or binary field longer than their maximum string size before reading it, with a `TProtocolException` of type `SIZE_LIMIT`. The maximum defaults to `TProtocol::DEFAULT_MAX_STRING_SIZE`, 16384000 bytes, the frame size limit the framed transports apply. It is an optional constructor argument of the three protocols and of their factories; pass `0` to read strings of any length, as before. diff --git a/lib/php/lib/Transport/TCurlClient.php b/lib/php/lib/Transport/TCurlClient.php index 61c5287ef4a..0dc150b41bf 100644 --- a/lib/php/lib/Transport/TCurlClient.php +++ b/lib/php/lib/Transport/TCurlClient.php @@ -40,7 +40,7 @@ class TCurlClient extends TTransport private const DEFAULT_PORTS = ['http' => 80, 'https' => 443]; /** @var \CurlHandle|null */ - private static $curlHandle; + private $curlHandle; /** * The URI to request @@ -112,6 +112,7 @@ public function open(): void public function close(): void { + $this->closeCurlHandle(); $this->request = ''; $this->response = null; $this->responsePos = 0; @@ -165,14 +166,13 @@ public function write(string $buf): void */ public function flush(): void { - if (!self::$curlHandle) { - register_shutdown_function(['Thrift\\Transport\\TCurlClient', 'closeCurlHandle']); - self::$curlHandle = curl_init(); - curl_setopt(self::$curlHandle, CURLOPT_RETURNTRANSFER, true); - curl_setopt(self::$curlHandle, CURLOPT_USERAGENT, 'PHP/TCurlClient'); - curl_setopt(self::$curlHandle, CURLOPT_CUSTOMREQUEST, 'POST'); + if (!$this->curlHandle) { + $this->curlHandle = curl_init(); + curl_setopt($this->curlHandle, CURLOPT_RETURNTRANSFER, true); + curl_setopt($this->curlHandle, CURLOPT_USERAGENT, 'PHP/TCurlClient'); + curl_setopt($this->curlHandle, CURLOPT_CUSTOMREQUEST, 'POST'); // curl follows no redirect; flush() follows one itself, within the origin of the URL. - curl_setopt(self::$curlHandle, CURLOPT_FOLLOWLOCATION, false); + curl_setopt($this->curlHandle, CURLOPT_FOLLOWLOCATION, false); } // God, PHP really has some esoteric ways of doing simple things. $host = $this->host . ($this->port != 80 ? ':' . $this->port : ''); @@ -189,48 +189,48 @@ public function flush(): void $headers[] = "$key: $value"; } - curl_setopt(self::$curlHandle, CURLOPT_HTTPHEADER, $headers); + curl_setopt($this->curlHandle, CURLOPT_HTTPHEADER, $headers); if ($this->timeout > 0) { if ($this->timeout < 1.0) { // Timestamps smaller than 1 second are ignored when CURLOPT_TIMEOUT is used - curl_setopt(self::$curlHandle, CURLOPT_TIMEOUT_MS, 1000 * $this->timeout); + curl_setopt($this->curlHandle, CURLOPT_TIMEOUT_MS, 1000 * $this->timeout); } else { - curl_setopt(self::$curlHandle, CURLOPT_TIMEOUT, $this->timeout); + curl_setopt($this->curlHandle, CURLOPT_TIMEOUT, $this->timeout); } } if ($this->connectionTimeout > 0) { if ($this->connectionTimeout < 1.0) { // Timestamps smaller than 1 second are ignored when CURLOPT_CONNECTTIMEOUT is used - curl_setopt(self::$curlHandle, CURLOPT_CONNECTTIMEOUT_MS, 1000 * $this->connectionTimeout); + curl_setopt($this->curlHandle, CURLOPT_CONNECTTIMEOUT_MS, 1000 * $this->connectionTimeout); } else { - curl_setopt(self::$curlHandle, CURLOPT_CONNECTTIMEOUT, $this->connectionTimeout); + curl_setopt($this->curlHandle, CURLOPT_CONNECTTIMEOUT, $this->connectionTimeout); } } - curl_setopt(self::$curlHandle, CURLOPT_POSTFIELDS, $this->request); + curl_setopt($this->curlHandle, CURLOPT_POSTFIELDS, $this->request); $this->request = ''; - curl_setopt(self::$curlHandle, CURLOPT_URL, $fullUrl); - $this->response = curl_exec(self::$curlHandle); - $code = curl_getinfo(self::$curlHandle, CURLINFO_HTTP_CODE); + curl_setopt($this->curlHandle, CURLOPT_URL, $fullUrl); + $this->response = curl_exec($this->curlHandle); + $code = curl_getinfo($this->curlHandle, CURLINFO_HTTP_CODE); // Follow one redirect, and only within the origin of the URL: the request goes // out again, with its headers and body, to the new path and query. if ($this->response !== false && $code >= 300 && $code < 400) { - $redirectUrl = self::redirectWithinOrigin($origin, curl_getinfo(self::$curlHandle, CURLINFO_REDIRECT_URL)); + $redirectUrl = self::redirectWithinOrigin($origin, curl_getinfo($this->curlHandle, CURLINFO_REDIRECT_URL)); if ($redirectUrl !== null) { $fullUrl = $redirectUrl; - curl_setopt(self::$curlHandle, CURLOPT_URL, $fullUrl); - $this->response = curl_exec(self::$curlHandle); - $code = curl_getinfo(self::$curlHandle, CURLINFO_HTTP_CODE); + curl_setopt($this->curlHandle, CURLOPT_URL, $fullUrl); + $this->response = curl_exec($this->curlHandle); + $code = curl_getinfo($this->curlHandle, CURLINFO_HTTP_CODE); } } $this->responsePos = 0; - $responseError = curl_error(self::$curlHandle); + $responseError = curl_error($this->curlHandle); // Handle non 200 status code / connect failure if ($this->response === false || $code !== 200) { - self::$curlHandle = null; + $this->curlHandle = null; $this->response = null; $error = 'TCurlClient: Could not connect to ' . $fullUrl; if ($responseError) { @@ -243,11 +243,11 @@ public function flush(): void } } - public static function closeCurlHandle(): void + public function closeCurlHandle(): void { // Dropping the reference frees the handle. curl_close() has had no effect // since PHP 8.0, and PHP 8.5 deprecates it. - self::$curlHandle = null; + $this->curlHandle = null; } /** diff --git a/lib/php/test/Unit/Lib/Transport/TCurlClientTest.php b/lib/php/test/Unit/Lib/Transport/TCurlClientTest.php index a55946822c1..6ed3af34272 100644 --- a/lib/php/test/Unit/Lib/Transport/TCurlClientTest.php +++ b/lib/php/test/Unit/Lib/Transport/TCurlClientTest.php @@ -143,6 +143,115 @@ public function testReadAllThrift4656() $transport->readAll(5); } + public function testClientsKeepIndependentTimeoutsAndReuseTheirOwnHandles(): void + { + $this->getFunctionMock('Thrift\\Transport', 'register_shutdown_function'); + $firstHandle = new \stdClass(); + $secondHandle = new \stdClass(); + $this->getFunctionMock('Thrift\\Transport', 'curl_init') + ->expects($this->exactly(2)) + ->willReturnOnConsecutiveCalls($firstHandle, $secondHandle); + + $options = new \SplObjectStorage(); + $this->getFunctionMock('Thrift\\Transport', 'curl_setopt') + ->expects($this->any()) + ->willReturnCallback(function ($handle, $option, $value) use ($options) { + $settings = $options->contains($handle) ? $options[$handle] : []; + $settings[$option] = $value; + $options[$handle] = $settings; + + return true; + }); + $requests = []; + $this->getFunctionMock('Thrift\\Transport', 'curl_exec') + ->expects($this->exactly(3)) + ->willReturnCallback(function ($handle) use ($options, &$requests) { + $settings = $options[$handle]; + $requests[] = [ + $handle, + $settings[CURLOPT_TIMEOUT_MS] ?? null, + $settings[CURLOPT_CONNECTTIMEOUT_MS] ?? null, + ]; + + return 'reply'; + }); + $this->getFunctionMock('Thrift\\Transport', 'curl_getinfo')->expects($this->any())->willReturn(200); + $this->getFunctionMock('Thrift\\Transport', 'curl_error')->expects($this->any())->willReturn(''); + + $first = new TCurlClient('localhost'); + $first->setTimeoutSecs(0.1); + $first->setConnectionTimeoutSecs(0.2); + $first->flush(); + $second = new TCurlClient('localhost'); + $second->flush(); + $first->flush(); + + $this->assertSame([ + [$firstHandle, 100.0, 200.0], + [$secondHandle, null, null], + [$firstHandle, 100.0, 200.0], + ], $requests); + } + + #[DataProvider('releaseHandleDataProvider')] + public function testReleasingOneClientLeavesTheOtherHandleUsable(string $release): void + { + $this->getFunctionMock('Thrift\\Transport', 'register_shutdown_function')->expects($this->never()); + $firstHandle = new \stdClass(); + $secondHandle = new \stdClass(); + $replacementHandle = new \stdClass(); + $this->getFunctionMock('Thrift\\Transport', 'curl_init') + ->expects($this->exactly(3)) + ->willReturnOnConsecutiveCalls($firstHandle, $secondHandle, $replacementHandle); + $this->getFunctionMock('Thrift\\Transport', 'curl_setopt')->expects($this->any())->willReturn(true); + $this->getFunctionMock('Thrift\\Transport', 'curl_getinfo')->expects($this->any())->willReturn(200); + $this->getFunctionMock('Thrift\\Transport', 'curl_error')->expects($this->any())->willReturn(''); + $handles = []; + $this->getFunctionMock('Thrift\\Transport', 'curl_exec') + ->expects($this->exactly($release === 'failure' ? 5 : 4)) + ->willReturnCallback(function ($handle) use (&$handles, $release) { + $handles[] = $handle; + + return $release === 'failure' && count($handles) === 3 ? false : 'reply'; + }); + + $first = new TCurlClient('localhost'); + $second = new TCurlClient('localhost'); + $first->flush(); + $second->flush(); + if ($release === 'failure') { + try { + $first->flush(); + $this->fail('Expected the failed transfer to throw'); + } catch (TTransportException $e) { + $this->assertSame(TTransportException::UNKNOWN, $e->getCode()); + } + } else { + $first->$release(); + } + $second->flush(); + $first->flush(); + + $expected = [$firstHandle, $secondHandle]; + if ($release === 'failure') { + $expected[] = $firstHandle; + } + $expected[] = $secondHandle; + $expected[] = $replacementHandle; + $this->assertSame($expected, $handles); + $this->assertSame('reply', $first->readAll(5)); + $this->assertSame('reply', $second->readAll(5)); + } + + public static function releaseHandleDataProvider(): array + { + return [ + 'close transport' => ['close'], + 'close handle' => ['closeCurlHandle'], + 'failed transfer' => ['failure'], + ]; + } + public function testWrite() { $host = 'localhost'; @@ -187,16 +296,7 @@ public function testFlush( $expectedCode = null ) { $this->getFunctionMock('Thrift\\Transport', 'register_shutdown_function') - ->expects($this->once()) - ->with( - $this->callback( - function ($arg) { - return is_array($arg) - && $arg[0] === 'Thrift\\Transport\\TCurlClient' - && $arg[1] === 'closeCurlHandle'; - } - ) - ); + ->expects($this->never()); $this->getFunctionMock('Thrift\\Transport', 'curl_init') ->expects($this->once()); @@ -558,7 +658,7 @@ public function testCloseCurlHandle() $curlHandle = new ReflectionProperty($transport, 'curlHandle'); $curlHandle->setValue($transport, 'testHandle'); - $transport::closeCurlHandle(); + $transport->closeCurlHandle(); $this->assertNull($curlHandle->getValue($transport)); }