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

CURLOPT_RESOLVE multiple ips and CURLOPT_HAPPY_EYEBALLS_TIMEOUT #2260

Closed
wants to merge 3 commits into from

Conversation

Andersbakken
Copy link
Contributor

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.

Pass a long. It should contain the time in milliseconds that you want
to provide as a head start for ipv6 for the happy eyeballs algorithm.
Set to zero to switch to the default built-in connection timeout 200 ms.
.SH AVAILABILITY
Copy link
Member

Choose a reason for hiding this comment

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

this should have a .SH DEFAULT and wouldn't it make more sense that the default is 200

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds better. Fixed now.

the port number of the service where libcurl wants to connect to the HOST and
ADDRESS is the numerical IP address. If libcurl is built to support IPv6,
ADDRESS can of course be either IPv4 or IPv6 style addressing.
HOST:PORT:ADDRESS... where HOST is the name libcurl will try to
Copy link
Member

Choose a reason for hiding this comment

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

i think this would be easier read as HOST:PORT:ADDRESS[,ADDRESS]...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. Fixed now.

@@ -400,6 +400,7 @@ CURLOPT_FTP_USE_EPRT 7.10.5
CURLOPT_FTP_USE_EPSV 7.9.2
CURLOPT_FTP_USE_PRET 7.20.0
CURLOPT_GSSAPI_DELEGATION 7.22.0
CURLOPT_HAPPY_EYEBALLS_TIMEOUT 7.55.3
Copy link
Member

Choose a reason for hiding this comment

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

this needs to be fixed 7.58.1

lib/hostip.c Outdated
address[alen-1] = 0; /* zero terminate there */
address++; /* pass the open bracket */
tmpPort = strtoul(tmp + 1, &tmp, 10);
if(!tmpPort || tmpPort > USHRT_MAX || *tmp != ':' || !(tmp + 1)) {
Copy link
Member

Choose a reason for hiding this comment

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

I think you meant !*(tmp+1)

Copy link
Member

Choose a reason for hiding this comment

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

also curl supports port 0 or so I've read

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did indeed :-) I also removed the !tmpPort check and rather added a check for the no valid characters case.

@Andersbakken Andersbakken changed the title Dns resolve multi CURLOPT_RESOLVE multiple ips and CURLOPT_HAPPY_EYEBALLS_TIMEOUT Jan 24, 2018
lib/hostip.c Outdated
if(address[0] == '[') {
if(address[alen-1] != ']')
/* it needs to also end with ] to be valid */
continue;
Copy link
Member

Choose a reason for hiding this comment

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

though i'm aware this is the way it ws before should we just error here. what is the advantage to continuing if [ only, is that ever valid

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The continue is to go on to the next hostp in the the linked list of hostpairs. It didn't seem beneficial to change this behavior but I'm not attached to that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah. I see what you mean. Yes, it should be goto err. Fixing that now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed that now. You're completely right. The continue would be for the wrong loop here.

lib/hostip.c Outdated
if(tmp)
alen = tmp - last;
else
alen = strlen(hostp->data) - (last - hostp->data);
Copy link
Member

Choose a reason for hiding this comment

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

i don't follow this

foo.bar:80:1.1.1.1,2.2.2.2,3.3.3.3
           ^      ^
           |      |
           -last  -tmp

assume hostp->data is 0 just for pseudo math ehre
34 - (11 - 0) = 23 address len?

also personally i find last and tmp confusing, they could be begin and end.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just needed to get the pointer converted into an offset but I now realize it's better to just do strlen addrBegin

I also renamed all the variables to more readable ones.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just needed to do the pointer arithmetic to get the offset but I changed it to strlen(addrEnd) which reads better and is faster anyway.

lib/hostip.c Outdated
if(!addr) {
infof(data, "Address in '%s' found illegal!\n", hostp->data);
if(tmp)
last = tmp + 1 + alen;
Copy link
Member

Choose a reason for hiding this comment

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

this again I don't follow wouldn't it be out of bounds. if you checked for null it would be better like
if(*end)
begin = end;
else
break;

also it occurs to me i should have done this review in the aggregate. sorry for the slew of notifications!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The + alen was incorrect. You're right. This would read out of bounds. I've fixed that now.

@Andersbakken
Copy link
Contributor Author

@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
Copy link
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 Andersbakken force-pushed the dns_resolve_multi branch 2 times, most recently from 3a03c32 to 3f3c1e4 Compare January 31, 2018 19:28
@Andersbakken
Copy link
Contributor Author

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

@Andersbakken
Copy link
Contributor Author

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.

This enables users to preresolve but still take advantage of happy
eyeballs and trying multiple addresses if some are not connecting.
jay pushed a commit to jay/curl that referenced this pull request Feb 2, 2018
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 pushed a commit to jay/curl that referenced this pull request Feb 3, 2018
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
Copy link
Contributor Author

@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
Copy link
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
Copy link
Contributor Author

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
Copy link
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
Copy link
Member

bagder commented Feb 13, 2018

Since HET contains the timeout already, maybe just CURL_HET_DEFAULT ?

@Andersbakken
Copy link
Contributor Author

@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 pushed a commit that referenced this pull request Feb 20, 2018
This enables users to preresolve but still take advantage of happy
eyeballs and trying multiple addresses if some are not connecting.

Ref: #2260
@jay
Copy link
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 pushed a commit to fsedano/curl that referenced this pull request Feb 20, 2018
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 pushed a commit to fsedano/curl that referenced this pull request Feb 21, 2018
- 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 pushed a commit to fsedano/curl that referenced this pull request Feb 21, 2018
- 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 pushed a commit to fsedano/curl that referenced this pull request Feb 21, 2018
- 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
- 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
- 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
Copy link
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
Copy link
Contributor Author

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.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants