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

Add IPv4 fallback on connection timeout, and adds COMPOSER_IPRESOLVE env var #11791

Merged
merged 3 commits into from Jan 11, 2024

Conversation

Seldaek
Copy link
Member

@Seldaek Seldaek commented Jan 10, 2024

@Seldaek Seldaek added this to the 2.7 milestone Jan 10, 2024
@Seldaek Seldaek requested a review from naderman January 10, 2024 15:43
doc/articles/troubleshooting.md Outdated Show resolved Hide resolved
src/Composer/Util/Http/CurlDownloader.php Show resolved Hide resolved
src/Composer/Util/Http/CurlDownloader.php Show resolved Hide resolved
src/Composer/Util/Http/CurlDownloader.php Outdated Show resolved Hide resolved
@Seldaek
Copy link
Member Author

Seldaek commented Jan 10, 2024

@stof thanks, all feedback should be addressed.

@@ -143,7 +143,7 @@ public function download(callable $resolve, callable $reject, string $origin, st
/**
* @param mixed[] $options
*
* @param array{retryAuthFailure?: bool, redirects?: int<0, max>, retries?: int<0, max>, storeAuth?: 'prompt'|bool} $attributes
* @param array{retryAuthFailure?: bool, redirects?: int<0, max>, retries?: int<0, max>, storeAuth?: 'prompt'|bool, ipResolve?: 4|6|null} $attributes
Copy link
Member

Choose a reason for hiding this comment

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

can't make up your mind whether to use |null or ? and here you use both? ;-)

Copy link
Member Author

Choose a reason for hiding this comment

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

It's optional to pass it but it defaults to null so yeah this looks weird but works well enough

Copy link
Contributor

Choose a reason for hiding this comment

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

The ? after a key does not indicate nullability but the fact that this key is optional in the array.

Copy link
Member

@naderman naderman left a comment

Choose a reason for hiding this comment

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

Looks good!

@@ -199,6 +206,12 @@ private function initDownload(callable $resolve, callable $reject, string $origi
curl_setopt($curlHandle, CURLOPT_ENCODING, ""); // let cURL set the Accept-Encoding header to what it supports
curl_setopt($curlHandle, CURLOPT_PROTOCOLS, CURLPROTO_HTTP | CURLPROTO_HTTPS);

if ($attributes['ipResolve'] === 4) {
curl_setopt($curlHandle, CURLOPT_IPRESOLVE, CURL_IPRESOLVE_V4);

Choose a reason for hiding this comment

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

curl_setopt($curlHandle, CURLOPT_HAPPY_EYEBALLS_TIMEOUT_MS, 0);

When using the IPv4 protocol, is it possible to change the default 200ms wait for IPv6 to 0ms?

Copy link
Contributor

Choose a reason for hiding this comment

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

if resolution is forced to be ipv4, there won't be any happy eyeballs happening, so this timeout won't ever be used.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah that would be my expectation as well, so I don't think this is necessary.

@Seldaek Seldaek merged commit 3491986 into composer:main Jan 11, 2024
20 checks passed
@Seldaek Seldaek deleted the curl_ipvx branch January 11, 2024 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Connection timed out
4 participants