Skip to content

THRIFT-6370: PHP: Give each TCurlClient its own curl handle #3947

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
sveneld wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

THRIFT-6370: PHP: Give each TCurlClient its own curl handle #3947

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions lib/php/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
50 changes: 25 additions & 25 deletions lib/php/lib/Transport/TCurlClient.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -112,6 +112,7 @@ public function open(): void

public function close(): void
{
$this->closeCurlHandle();
$this->request = '';
$this->response = null;
$this->responsePos = 0;
Expand Down Expand Up @@ -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 : '');
Expand All @@ -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) {
Expand All @@ -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;
}

/**
Expand Down
122 changes: 111 additions & 11 deletions lib/php/test/Unit/Lib/Transport/TCurlClientTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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());

Expand Down Expand Up @@ -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));
}
Expand Down
Loading