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
Conversation
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
docs/libcurl/opts/CURLOPT_RESOLVE.3
Outdated
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 |
There was a problem hiding this comment.
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]...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. Fixed now.
docs/libcurl/symbols-in-versions
Outdated
@@ -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 |
There was a problem hiding this comment.
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)) { |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
lib/hostip.c
Outdated
if(address[0] == '[') { | ||
if(address[alen-1] != ']') | ||
/* it needs to also end with ] to be valid */ | ||
continue; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries :-)
There was a problem hiding this comment.
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.
d6a672a
to
11b5d3d
Compare
@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 :-) |
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.
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 |
3a03c32
to
3f3c1e4
Compare
I've split it up into separate commits and also removed the camel casing. |
3f3c1e4
to
8b1ab82
Compare
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.
8b1ab82
to
1360591
Compare
This enables users to preresolve but still take advantage of happy eyeballs and trying multiple addresses if some are not connecting. Ref: curl#2260
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 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
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); which one of those best signals to the reader that they caller wants the default value |
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 |
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 |
Since HET contains the timeout already, maybe just |
@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. |
This enables users to preresolve but still take advantage of happy eyeballs and trying multiple addresses if some are not connecting. Ref: #2260
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. |
This enables users to preresolve but still take advantage of happy eyeballs and trying multiple addresses if some are not connecting. Ref: curl#2260
- 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
- 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
- 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
- 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
- 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
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. |
Sounds good to me. Thanks for your help on this @jay |
This pull request enables two things:
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.