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

CURLOPT_RESOLVE multiple ips and CURLOPT_HAPPY_EYEBALLS_TIMEOUT #2260

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@Andersbakken
Contributor

Andersbakken commented Jan 24, 2018

This pull request enables two things:

  • Override of curl's happy eyeball head start value through a setopt
  • The ability to specify more than one ip address for CURLOPT_RESOLVE

This enables platforms that do their own resolving to still take advantage of happy eyeballs and connecting to the next ip address in the list if the first one doesn't connect within a reasonable timeout.

Show outdated Hide outdated lib/hostip.c

@Andersbakken Andersbakken changed the title from Dns resolve multi to CURLOPT_RESOLVE multiple ips and CURLOPT_HAPPY_EYEBALLS_TIMEOUT Jan 24, 2018

Show outdated Hide outdated lib/hostip.c
Show outdated Hide outdated lib/hostip.c
Show outdated Hide outdated lib/hostip.c
@Andersbakken

This comment has been minimized.

Show comment
Hide comment
@Andersbakken

Andersbakken Jan 31, 2018

Contributor

@jay I've updated the pull request with upstream changes that made the fnmatch stuff work again so I think it's all good to go now. I squashed it into one commit so the tests will take some hours. Let me know if you have other things you want changed. We would love to see this in the next version of Curl :-)

Contributor

Andersbakken commented Jan 31, 2018

@jay I've updated the pull request with upstream changes that made the fnmatch stuff work again so I think it's all good to go now. I squashed it into one commit so the tests will take some hours. Let me know if you have other things you want changed. We would love to see this in the next version of Curl :-)

@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Jan 31, 2018

Member

I've updated the pull request with upstream changes that made the fnmatch stuff work again so I think it's all good to go now. I squashed it into one commit so the tests will take some hours. Let me know if you have other things you want changed.

On refresh just now I only see 1 file in the review, can you check again on that. This PR contains two distinct changes and I'd prefer them as separate commits if possible. Also one thing I saw before I refreshed was camelCase variable names which we don't use and although we technically don't have any guidance against it I suggest changing them.

We would love to see this in the next version of Curl :-)

That's likely if all feedback is addressed.

edit: now working on the 2nd review in https://github.com/curl/curl/compare/master...jay:pr_2260_mod1?expand=1

Member

jay commented Jan 31, 2018

I've updated the pull request with upstream changes that made the fnmatch stuff work again so I think it's all good to go now. I squashed it into one commit so the tests will take some hours. Let me know if you have other things you want changed.

On refresh just now I only see 1 file in the review, can you check again on that. This PR contains two distinct changes and I'd prefer them as separate commits if possible. Also one thing I saw before I refreshed was camelCase variable names which we don't use and although we technically don't have any guidance against it I suggest changing them.

We would love to see this in the next version of Curl :-)

That's likely if all feedback is addressed.

edit: now working on the 2nd review in https://github.com/curl/curl/compare/master...jay:pr_2260_mod1?expand=1

@Andersbakken

This comment has been minimized.

Show comment
Hide comment
@Andersbakken

Andersbakken Jan 31, 2018

Contributor

I've split it up into separate commits and also removed the camel casing.

Contributor

Andersbakken commented Jan 31, 2018

I've split it up into separate commits and also removed the camel casing.

@Andersbakken

This comment has been minimized.

Show comment
Hide comment
@Andersbakken

Andersbakken Feb 1, 2018

Contributor

Don't entirely understand why the last one failed on travis. It seems I had a checksrc issue and I brought the PR up-to-date with master to give it another shot.

Contributor

Andersbakken commented Feb 1, 2018

Don't entirely understand why the last one failed on travis. It seems I had a checksrc issue and I brought the PR up-to-date with master to give it another shot.

Andersbakken added some commits Jan 31, 2018

Add the ability to specify more than one ip address for CURLOPT_RESOLVE
This enables users to preresolve but still take advantage of happy
eyeballs and trying multiple addresses if some are not connecting.

jay added a commit to jay/curl that referenced this pull request Feb 2, 2018

CURLOPT_RESOLVE: Add support for multiple IP addresses per entry
This enables users to preresolve but still take advantage of happy
eyeballs and trying multiple addresses if some are not connecting.

Ref: curl#2260

jay added a commit to jay/curl that referenced this pull request Feb 3, 2018

CURLOPT_RESOLVE: Add support for multiple IP addresses per entry
This enables users to preresolve but still take advantage of happy
eyeballs and trying multiple addresses if some are not connecting.

Ref: curl#2260
@Andersbakken

This comment has been minimized.

Show comment
Hide comment
@Andersbakken

Andersbakken Feb 7, 2018

Contributor

@jay Hi Jay. Just wanted to check in. The fuzzer test that failed seems kinda unrelated to me. Do you think the PR is acceptable in its current form or are there other things that should be changed. Not trying to nag :-)

Contributor

Andersbakken commented Feb 7, 2018

@jay Hi Jay. Just wanted to check in. The fuzzer test that failed seems kinda unrelated to me. Do you think the PR is acceptable in its current form or are there other things that should be changed. Not trying to nag :-)

@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Feb 7, 2018

Member

@jay Hi Jay. Just wanted to check in. The fuzzer test that failed seems kinda unrelated to me. Do you think the PR is acceptable in its current form or are there other things that should be changed. Not trying to nag :-)

No it's fine. I started a final review of the curlopt_resolve changes several days ago, I edited my post to reflect that and also since I ref tagged it it shows each time I update the commit but I guess since it's not the branch associated with the pr it doesn't send out a ping. I made some minor changes to the loop, of note is some consolidation* and to continue on IPv6 when that is not built in. For example imagine you have something like test.com:80:[::1],127.0.0.1 then it should use just 127.0.0.1 in a build without IPv6 rather than error, however test.com:80:[::1] causes an error. In other words it will now succeed when it recognizes at least one address (edit: malformatted addresses will still fail the whole thing). The test change I removed because it turns out the test you used is marked disabled so instead I added a unit test to test some combinations of commas and addresses, but I haven't actually tested it yet. The unit test is on a different computer and I haven't yet pushed it. https://github.com/curl/curl/compare/master...jay:pr_2260_mod1?expand=1

* (operators should not start continuation lines)
Since before discovering the disabled test I anticipated landing it after the loop change so I just changed that as well rather than + it on the review and ask you to change it.

Concerning the happy eyeballs timeout change I looked at it again just now and I still don't like that a timeout of 0 will reset it to the default. Also maybe it's not a good idea to specify that 200 is the default since we may want to change it in the future. Perhaps a new macro like CURLHETOPT_DEFAULT_TIMEOUT that is set (long)-1 that way we know what is set. I am thinking about anyone who reads through the code and they see

curl_easy_setopt(curl, CURLOPT_HAPPY_EYEBALLS_TIMEOUT, 0);
curl_easy_setopt(curl, CURLOPT_HAPPY_EYEBALLS_TIMEOUT, 200);
curl_easy_setopt(curl, CURLOPT_HAPPY_EYEBALLS_TIMEOUT, CURLHETOPT_DEFAULT);

which one of those best signals to the reader that they caller wants the default value

Member

jay commented Feb 7, 2018

@jay Hi Jay. Just wanted to check in. The fuzzer test that failed seems kinda unrelated to me. Do you think the PR is acceptable in its current form or are there other things that should be changed. Not trying to nag :-)

No it's fine. I started a final review of the curlopt_resolve changes several days ago, I edited my post to reflect that and also since I ref tagged it it shows each time I update the commit but I guess since it's not the branch associated with the pr it doesn't send out a ping. I made some minor changes to the loop, of note is some consolidation* and to continue on IPv6 when that is not built in. For example imagine you have something like test.com:80:[::1],127.0.0.1 then it should use just 127.0.0.1 in a build without IPv6 rather than error, however test.com:80:[::1] causes an error. In other words it will now succeed when it recognizes at least one address (edit: malformatted addresses will still fail the whole thing). The test change I removed because it turns out the test you used is marked disabled so instead I added a unit test to test some combinations of commas and addresses, but I haven't actually tested it yet. The unit test is on a different computer and I haven't yet pushed it. https://github.com/curl/curl/compare/master...jay:pr_2260_mod1?expand=1

* (operators should not start continuation lines)
Since before discovering the disabled test I anticipated landing it after the loop change so I just changed that as well rather than + it on the review and ask you to change it.

Concerning the happy eyeballs timeout change I looked at it again just now and I still don't like that a timeout of 0 will reset it to the default. Also maybe it's not a good idea to specify that 200 is the default since we may want to change it in the future. Perhaps a new macro like CURLHETOPT_DEFAULT_TIMEOUT that is set (long)-1 that way we know what is set. I am thinking about anyone who reads through the code and they see

curl_easy_setopt(curl, CURLOPT_HAPPY_EYEBALLS_TIMEOUT, 0);
curl_easy_setopt(curl, CURLOPT_HAPPY_EYEBALLS_TIMEOUT, 200);
curl_easy_setopt(curl, CURLOPT_HAPPY_EYEBALLS_TIMEOUT, CURLHETOPT_DEFAULT);

which one of those best signals to the reader that they caller wants the default value

@Andersbakken

This comment has been minimized.

Show comment
Hide comment
@Andersbakken

Andersbakken Feb 8, 2018

Contributor

Hi @jay

Thanks. That sounds awesome. I agree with both the ipv6 change you made and your sentiments on the default. Do you want me to add code for that or will you do it?

regards

Anders

Contributor

Andersbakken commented Feb 8, 2018

Hi @jay

Thanks. That sounds awesome. I agree with both the ipv6 change you made and your sentiments on the default. Do you want me to add code for that or will you do it?

regards

Anders

@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Feb 9, 2018

Member

Do you want me to add code for that or will you do it?

go for it. CURLHETOPT_DEFAULT_TIMEOUT seems like a good name unless you can think of something better. the first part as CURLHETOPT because I know there is CURLSSLOPT for CURLOPT_SSL_OPTIONS option symbols so I was trying to mimic that. looking at the symbols-in-versions I don't see a clear winner for abbreviation format maybe CURL_HET_DEFAULT_TIMEOUT? CURL_HAPPY_EYEBALLS_DEFAULT_TIMEOUT seems like a mouthful. @bagder any ideas

Member

jay commented Feb 9, 2018

Do you want me to add code for that or will you do it?

go for it. CURLHETOPT_DEFAULT_TIMEOUT seems like a good name unless you can think of something better. the first part as CURLHETOPT because I know there is CURLSSLOPT for CURLOPT_SSL_OPTIONS option symbols so I was trying to mimic that. looking at the symbols-in-versions I don't see a clear winner for abbreviation format maybe CURL_HET_DEFAULT_TIMEOUT? CURL_HAPPY_EYEBALLS_DEFAULT_TIMEOUT seems like a mouthful. @bagder any ideas

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Feb 13, 2018

Member

Since HET contains the timeout already, maybe just CURL_HET_DEFAULT ?

Member

bagder commented Feb 13, 2018

Since HET contains the timeout already, maybe just CURL_HET_DEFAULT ?

@Andersbakken

This comment has been minimized.

Show comment
Hide comment
@Andersbakken

Andersbakken Feb 16, 2018

Contributor

@jay Sorry about the delay. I added a commit you can squash into your branch. I decided to allow 0 as a value and just initialize the timeout to the default in Curl_open.

Contributor

Andersbakken commented Feb 16, 2018

@jay Sorry about the delay. I added a commit you can squash into your branch. I decided to allow 0 as a value and just initialize the timeout to the default in Curl_open.

jay added a commit that referenced this pull request Feb 20, 2018

CURLOPT_RESOLVE: Add support for multiple IP addresses per entry
This enables users to preresolve but still take advantage of happy
eyeballs and trying multiple addresses if some are not connecting.

Ref: #2260
@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Feb 20, 2018

Member

I landed the CURLOPT_RESOLVE changes and I'll land CURLOPT_HAPPY_EYEBALLS_TIMEOUT before the feature window closes. I am going to expose it in the tool --happy-eyeballs-timeout-ms.

Member

jay commented Feb 20, 2018

I landed the CURLOPT_RESOLVE changes and I'll land CURLOPT_HAPPY_EYEBALLS_TIMEOUT before the feature window closes. I am going to expose it in the tool --happy-eyeballs-timeout-ms.

fsedano added a commit to fsedano/curl that referenced this pull request Feb 20, 2018

CURLOPT_RESOLVE: Add support for multiple IP addresses per entry
This enables users to preresolve but still take advantage of happy
eyeballs and trying multiple addresses if some are not connecting.

Ref: curl#2260

@jay jay closed this in 2427d94 Feb 20, 2018

fsedano added a commit to fsedano/curl that referenced this pull request Feb 21, 2018

url: Add option CURLOPT_HAPPY_EYEBALLS_TIMEOUT
- Add new option CURLOPT_HAPPY_EYEBALLS_TIMEOUT to set libcurl's happy
  eyeball timeout value.

- Add new optval macro CURL_HET_DEFAULT to represent the default happy
  eyeballs timeout value (currently 200 ms).

- Add new tool option --happy-eyeballs-timeout-ms to expose
  CURLOPT_HAPPY_EYEBALLS_TIMEOUT. The -ms suffix is used because the
  other -timeout options in the tool expect seconds not milliseconds.

Closes curl#2260

fsedano added a commit to fsedano/curl that referenced this pull request Feb 21, 2018

url: Add option CURLOPT_HAPPY_EYEBALLS_TIMEOUT
- Add new option CURLOPT_HAPPY_EYEBALLS_TIMEOUT to set libcurl's happy
  eyeball timeout value.

- Add new optval macro CURL_HET_DEFAULT to represent the default happy
  eyeballs timeout value (currently 200 ms).

- Add new tool option --happy-eyeballs-timeout-ms to expose
  CURLOPT_HAPPY_EYEBALLS_TIMEOUT. The -ms suffix is used because the
  other -timeout options in the tool expect seconds not milliseconds.

Closes curl#2260

fsedano added a commit to fsedano/curl that referenced this pull request Feb 21, 2018

url: Add option CURLOPT_HAPPY_EYEBALLS_TIMEOUT
- Add new option CURLOPT_HAPPY_EYEBALLS_TIMEOUT to set libcurl's happy
  eyeball timeout value.

- Add new optval macro CURL_HET_DEFAULT to represent the default happy
  eyeballs timeout value (currently 200 ms).

- Add new tool option --happy-eyeballs-timeout-ms to expose
  CURLOPT_HAPPY_EYEBALLS_TIMEOUT. The -ms suffix is used because the
  other -timeout options in the tool expect seconds not milliseconds.

Closes curl#2260

jay added a commit to jay/curl that referenced this pull request Feb 21, 2018

lib: CURLOPT_HAPPY_EYEBALLS_TIMEOUT => CURLOPT_HAPPY_EYEBALLS_TIMEOUT_MS
- In keeping with the naming of our other connect timeout options rename
  CURLOPT_HAPPY_EYEBALLS_TIMEOUT to CURLOPT_HAPPY_EYEBALLS_TIMEOUT_MS.

This change adds the _MS suffix since the option expects milliseconds.
This is more intuitive for our users since other connect timeout options
that expect milliseconds use _MS such as CURLOPT_TIMEOUT_MS,
CURLOPT_CONNECTTIMEOUT_MS, CURLOPT_ACCEPTTIMEOUT_MS.

The tool option already uses an -ms suffix, --happy-eyeballs-timeout-ms.

Follow-up to 2427d94 which added the option yesterday.

Ref: curl#2260

jay added a commit to jay/curl that referenced this pull request Feb 21, 2018

lib: CURLOPT_HAPPY_EYEBALLS_TIMEOUT => CURLOPT_HAPPY_EYEBALLS_TIMEOUT_MS
- In keeping with the naming of our other connect timeout options rename
  CURLOPT_HAPPY_EYEBALLS_TIMEOUT to CURLOPT_HAPPY_EYEBALLS_TIMEOUT_MS.

This change adds the _MS suffix since the option expects milliseconds.
This is more intuitive for our users since other connect timeout options
that expect milliseconds use _MS such as CURLOPT_TIMEOUT_MS,
CURLOPT_CONNECTTIMEOUT_MS, CURLOPT_ACCEPTTIMEOUT_MS.

The tool option already uses an -ms suffix, --happy-eyeballs-timeout-ms.

Follow-up to 2427d94 which added the lib and tool option yesterday.

Ref: curl#2260
@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Feb 21, 2018

Member

This option has been renamed CURLOPT_HAPPY_EYEBALLS_TIMEOUT_MS in dd027c8 for consistency with other connect timeout options that expect milliseconds and use _MS suffix.

Member

jay commented Feb 21, 2018

This option has been renamed CURLOPT_HAPPY_EYEBALLS_TIMEOUT_MS in dd027c8 for consistency with other connect timeout options that expect milliseconds and use _MS suffix.

@Andersbakken

This comment has been minimized.

Show comment
Hide comment
@Andersbakken

Andersbakken Feb 23, 2018

Contributor

Sounds good to me. Thanks for your help on this @jay

Contributor

Andersbakken commented Feb 23, 2018

Sounds good to me. Thanks for your help on this @jay

@lock lock bot locked as resolved and limited conversation to collaborators May 24, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.