Skip to content

feat: share Connection & DNS Cache to CURLRequest #9557

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.

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
wants to merge 5 commits into
base: 4.7
Choose a base branch
from

Conversation

ddevsr
Copy link
Collaborator

@ddevsr ddevsr commented May 14, 2025

Description
I need this for reuse connection. As soon PHP 8.5 will have new function for persistent, Benefit choosing persistent_id by PHP (read more on RFC).

php/php-src@d20880c#diff-6db2da0f77fa4c56808260fdf566a723bbb52d6159077953097d05d995335558R135-R245

https://wiki.php.net/rfc/curl_share_persistence_improvement

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@michalsn
Copy link
Member

This is a nice improvement, but I think it would benefit from being more configurable.

How about introducing a $share array in the config with default values like connect and dns, and optionally supporting others like cookie, session, and psl? This way, we can control what gets shared based on the use case. If the array is empty, we can skip calling curl_share_init() entirely, which avoids unnecessary memory allocation.

We should also note in the user guide that the $share option makes the most sense when making requests to the same domain.

It's a bit unfortunate that we already have $shareOptions property in the config - hopefully it won't be confusing.

@ddevsr
Copy link
Collaborator Author

ddevsr commented May 14, 2025

In new function persistent that avoid cookie, we avoid too?

@michalsn
Copy link
Member

Do you mean curl_share_init_persistent()?
I think we should introduce a new configuration option, such as sharePersistent, which would default to false. If persistence is enabled, I’d still accept all share options - PHP will handle any invalid ones, like the cookie option, by throwing an error.

@ddevsr ddevsr added enhancement PRs that improve existing functionalities 4.7 labels May 15, 2025
@ddevsr ddevsr force-pushed the share-connection-curl branch from 8d5fe45 to 3c20b47 Compare May 16, 2025 14:46
@ddevsr
Copy link
Collaborator Author

ddevsr commented May 16, 2025

<?php

$optShareConnection = [
            CURL_LOCK_DATA_CONNECT,
            CURL_LOCK_DATA_DNS,
];
$this->shareConnection = curl_share_init();

foreach (array_unique($optShareConnection) as $opt) {
      curl_share_setopt($this->shareConnection, CURLSHOPT_SHARE, $opt);
}

same as persistent implementation in next PR (when ready):

<?php

$optShareConnection = [
            CURL_LOCK_DATA_CONNECT,
            CURL_LOCK_DATA_DNS,
];
$this->shareConnection = curl_share_init_persistent($optShareConnection);

@ddevsr ddevsr marked this pull request as ready for review May 16, 2025 15:04
@ddevsr ddevsr requested review from michalsn and paulbalandan May 16, 2025 15:06
@ddevsr ddevsr marked this pull request as draft May 16, 2025 15:20
@ddevsr ddevsr marked this pull request as ready for review May 16, 2025 16:09
* CURLRequest Share Connection
* --------------------------------------------------------------------------
*
* Whether share connection between requests.
Copy link
Member

Choose a reason for hiding this comment

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

The sentence suggests it is boolean rather than as array. Please revise. I think this property should read as "shareConnectionOptions".


.. versionadded:: 4.7.0

If you want to share connection between requests, set ``$shareConnection`` with array constant `CURL_LOCK_DATA_* <https://www.php.net/manual/en/curl.constants.php#constant.curl-lock-data-connect>`_ in **app/Config/CURLRequest.php**:
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to mention that these are the default values.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.7 enhancement PRs that improve existing functionalities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants