Skip to content

feat(transport): use share handle #1996

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

Open
Litarnus wants to merge 3 commits into
base: master
Choose a base branch
from
Open

feat(transport): use share handle #1996

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
eeb7dde
feat(transport): use share handle
Litarnus Jan 13, 2026
c563cef
use proper option
Litarnus Jan 13, 2026
224bd15
add guard for constant
Litarnus Jan 13, 2026
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
40 changes: 40 additions & 0 deletions src/HttpClient/HttpClient.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,47 @@ class HttpClient implements HttpClientInterface
*/
protected $sdkVersion;

/**
* Either a persistent share handle or a regular share handle, or null if no share handle can be obtained.
*
* @var object|resource|null
*/
private $shareHandle;

public function __construct(string $sdkIdentifier, string $sdkVersion)
{
$this->sdkIdentifier = $sdkIdentifier;
$this->sdkVersion = $sdkVersion;
if (\function_exists('curl_share_init_persistent')) {
$shareOptions = [\CURL_LOCK_DATA_DNS];
if (\defined('CURL_LOCK_DATA_CONNECT')) {
$shareOptions[] = \CURL_LOCK_DATA_CONNECT;
}
if (\defined('CURL_LOCK_DATA_SSL_SESSION')) {
$shareOptions[] = \CURL_LOCK_DATA_SSL_SESSION;
}
try {
$this->shareHandle = curl_share_init_persistent($shareOptions);
} catch (\Throwable $throwable) {
// don't crash if the share handle cannot be created
}
}

// If the persistent share handle cannot be created or doesn't exist
if ($this->shareHandle === null) {
try {
$this->shareHandle = curl_share_init();
Copy link

@cursor cursor bot Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing check for false return from curl_share_init

Medium Severity

When curl_share_init() returns false (possible in PHP 7.x on failure), $this->shareHandle is set to false. The check if ($this->shareHandle !== null) passes since false !== null is true, causing curl_setopt() to be called with false as the share handle. The PR discussion explicitly noted this concern ("an extra check doesn't hurt") but the check wasn't added. This could cause warnings and prevent proper fallback behavior when share handle initialization fails.

Additional Locations (1)

Fix in Cursor Fix in Web

curl_share_setopt($this->shareHandle, \CURLSHOPT_SHARE, \CURL_LOCK_DATA_DNS);
if (\defined('CURL_LOCK_DATA_CONNECT')) {
curl_share_setopt($this->shareHandle, \CURLSHOPT_SHARE, \CURL_LOCK_DATA_CONNECT);
}
if (\defined('CURL_LOCK_DATA_SSL_SESSION')) {
curl_share_setopt($this->shareHandle, \CURLSHOPT_SHARE, \CURL_LOCK_DATA_SSL_SESSION);
}
} catch (\Throwable $throwable) {
// don't crash if the share handle cannot be created
}
Comment on lines +58 to +64
Copy link

@sentry sentry bot Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The return values of curl_share_setopt() are not checked. If a call fails, the share handle will be misconfigured, silently disabling the intended caching feature.
Severity: MEDIUM

🔍 Detailed Analysis

The try-catch block around curl_share_init() is intended to gracefully handle setup failures. However, the subsequent curl_share_setopt() calls do not throw exceptions on failure; they return false. The code does not check these return values. If any curl_share_setopt() call fails, the catch block is skipped, and $this->shareHandle remains set to a partially configured handle. This misconfigured handle will be used later, causing the intended DNS, connect, and SSL session sharing to fail silently without any error or warning. The feature will not work as expected.

💡 Suggested Fix

Check the boolean return value of each curl_share_setopt() call. If any call returns false, set $this->shareHandle to null to prevent the misconfigured handle from being used, aligning with the intended graceful degradation.

🤖 Prompt for AI Agent 
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: src/HttpClient/HttpClient.php#L58-L64

Potential issue: The `try-catch` block around `curl_share_init()` is intended to
gracefully handle setup failures. However, the subsequent `curl_share_setopt()` calls do
not throw exceptions on failure; they return `false`. The code does not check these
return values. If any `curl_share_setopt()` call fails, the `catch` block is skipped,
and `$this->shareHandle` remains set to a partially configured handle. This
misconfigured handle will be used later, causing the intended DNS, connect, and SSL
session sharing to fail silently without any error or warning. The feature will not work
as expected.

Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 8543567

}
}

public function sendRequest(Request $request, Options $options): Response
Expand Down Expand Up @@ -72,6 +109,9 @@ public function sendRequest(Request $request, Options $options): Response
curl_setopt($curlHandle, \CURLOPT_RETURNTRANSFER, true);
curl_setopt($curlHandle, \CURLOPT_HEADERFUNCTION, $responseHeaderCallback);
curl_setopt($curlHandle, \CURLOPT_HTTP_VERSION, \CURL_HTTP_VERSION_1_1);
if ($this->shareHandle !== null) {
curl_setopt($curlHandle, \CURLOPT_SHARE, $this->shareHandle);
}
Comment on lines +112 to +114

This comment was marked as outdated.

Sign in to view

Copy link
Contributor Author

@Litarnus Litarnus Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation doesn't mention that curl_share_init can return false, in contrast to curl_init.


$httpSslVerifyPeer = $options->getHttpSslVerifyPeer();
if (!$httpSslVerifyPeer) {
Expand Down