Skip to content
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

share: don't reinitialize conncache #14696

Conversation

ericnorris
Copy link
Contributor

@ericnorris ericnorris commented Aug 26, 2024

Recently I was messing around with the idea of enabling persistent Curl_share structs in PHP, i.e. to be able to reuse the same instance of the struct across multiple requests in a single mod_php worker.

This worked something like the following (note that this is PHP, not C):

$sh = curl_share_init("some-persistent-id");

curl_share_setopt($sh, CURLSHOPT_SHARE, CURL_LOCK_DATA_DNS);
curl_share_setopt($sh, CURLSHOPT_SHARE, CURL_LOCK_DATA_CONNECT);
// ...etc

I noticed, however, that CURL_LOCK_DATA_CONNECT wasn't working across multiple requests in a mod_php worker. It did work if I made sure to only ever call curl_share_setopt on the very first request, which led me to look into the definition of curl_share_setopt.

I noticed that unlike other options - CURL_LOCK_DATA_SSL_SESSION, for example - CURL_LOCK_DATA_CONNECT wasn't checking to see if the connection cache had already been initialized.

Regardless of whether I continue to work on the PHP stuff, I figured it would be a simple change to check for this. It wasn't clear to me what the best way was to check if the connection cache was already initialized, so let me know if you have a preference for a better way.

@ericnorris
Copy link
Contributor Author

I ran make test locally, but it seems these tests are failing on the line length, I'll update that.

Before this change, calling curl_share_setopt w/ CURL_LOCK_DATA_CONNECT
a second time would re-initialize the connection cache, rather than use
the existing one.

After this change, calling curl_share_setopt w/ CURL_LOCK_DATA_CONNECT
multiple times will have no effect after the first call.
@ericnorris ericnorris force-pushed the share-preserve-existing-connections-in-setopt branch from 4200370 to 727dc25 Compare August 26, 2024 22:13
@bagder bagder requested a review from icing August 27, 2024 05:52
Copy link
Contributor

@icing icing left a comment

Choose a reason for hiding this comment

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

Thanks, that is a good catch. If we do this for sessions, we should do this idempotent behaviour for the connection cache as well.

@bagder bagder closed this in 8dd0cb7 Aug 27, 2024
@bagder
Copy link
Member

bagder commented Aug 27, 2024

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants