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 support for CURLOPT_RESOLVER_START callback #2311

Closed
wants to merge 14 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@fsedano
Contributor

fsedano commented Feb 14, 2018

This patch adds a callback which will be called when libcurl tries to start a new resolve, passing the resolver data.

If using ARES, this allows the user to access the ARES channel and configure callbacks or other options.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Feb 15, 2018

Member

The files CURLOPT_RESOLVER_START_DATA.3 and CURLOPT_RESOLVER_START_FUNCTION.3 are missing from the dist tarball - which causes the CI distcheck test to fail. Add them in docs/libcurl/opts/Makfile.inc.

Member

bagder commented Feb 15, 2018

The files CURLOPT_RESOLVER_START_DATA.3 and CURLOPT_RESOLVER_START_FUNCTION.3 are missing from the dist tarball - which causes the CI distcheck test to fail. Add them in docs/libcurl/opts/Makfile.inc.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Feb 15, 2018

Member

After I merged b46cfbc just now, I would also like to have the callback work like this:

  Curl_set_in_callback(conn->data, true);
  rc = invoke_callback ( args ...) ;
  Curl_set_in_callback(conn->data, false);

to make us detect attempts of recursive libcurl calls.

Member

bagder commented Feb 15, 2018

After I merged b46cfbc just now, I would also like to have the callback work like this:

  Curl_set_in_callback(conn->data, true);
  rc = invoke_callback ( args ...) ;
  Curl_set_in_callback(conn->data, false);

to make us detect attempts of recursive libcurl calls.

@fsedano

This comment has been minimized.

Show comment
Hide comment
@fsedano

fsedano Feb 16, 2018

Contributor

Is the breakage with travis known? It does not look related to the patch.

Contributor

fsedano commented Feb 16, 2018

Is the breakage with travis known? It does not look related to the patch.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Feb 16, 2018

Member

The travis jobs sometimes time out/fail in ways that aren't our fault. I restarted that single failed job now.

Member

bagder commented Feb 16, 2018

The travis jobs sometimes time out/fail in ways that aren't our fault. I restarted that single failed job now.

@fsedano

This comment has been minimized.

Show comment
Hide comment
@fsedano

fsedano Feb 20, 2018

Contributor

Seems to fail due to a parent issue:
c:\projects\curl\lib\hostip.c(957): warning C4701: potentially uninitialized local variable 'addresses' used [C:\projects\curl\build.msvc2012\lib\libcurl.vcxproj]
c:\projects\curl\lib\hostip.c(957): warning C4703: potentially uninitialized local pointer variable 'addresses' used [C:\projects\curl\build.msvc2012\lib\libcurl.vcxproj]

Contributor

fsedano commented Feb 20, 2018

Seems to fail due to a parent issue:
c:\projects\curl\lib\hostip.c(957): warning C4701: potentially uninitialized local variable 'addresses' used [C:\projects\curl\build.msvc2012\lib\libcurl.vcxproj]
c:\projects\curl\lib\hostip.c(957): warning C4703: potentially uninitialized local pointer variable 'addresses' used [C:\projects\curl\build.msvc2012\lib\libcurl.vcxproj]

@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Feb 20, 2018

Member

Seems to fail due to a parent issue:
c:\projects\curl\lib\hostip.c(957): warning C4701: potentially uninitialized local variable 'addresses' used

My fault. I made those changes in Visual Studio 2010 which is not that sensitive on potentially uninitialized variables and didn't warn. The variable is never used uninitialized. Fixed in 73050fb.

Member

jay commented Feb 20, 2018

Seems to fail due to a parent issue:
c:\projects\curl\lib\hostip.c(957): warning C4701: potentially uninitialized local variable 'addresses' used

My fault. I made those changes in Visual Studio 2010 which is not that sensitive on potentially uninitialized variables and didn't warn. The variable is never used uninitialized. Fixed in 73050fb.

@fsedano

This comment has been minimized.

Show comment
Hide comment
@fsedano

fsedano Feb 21, 2018

Contributor

It seems travis failed again for something unrelated to us. Can we have this restarted please? We need this patch to be part of next release

Contributor

fsedano commented Feb 21, 2018

It seems travis failed again for something unrelated to us. Can we have this restarted please? We need this patch to be part of next release

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Feb 21, 2018

Member

"This branch has conflicts that must be resolved" due to conflicting changes that were merged recently. The unrelated errors were also fixed in the mean time I believe.

Member

bagder commented Feb 21, 2018

"This branch has conflicts that must be resolved" due to conflicting changes that were merged recently. The unrelated errors were also fixed in the mean time I believe.

@fsedano

This comment has been minimized.

Show comment
Hide comment
@fsedano

fsedano Feb 21, 2018

Contributor

Working on the conflicts

Contributor

fsedano commented Feb 21, 2018

Working on the conflicts

fsedano and others added some commits Feb 14, 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 #2260
@fsedano

This comment has been minimized.

Show comment
Hide comment
@fsedano

fsedano Feb 21, 2018

Contributor

Everything looks fine - Could we have this merged for 7.59.0 please? It's important for us.

Contributor

fsedano commented Feb 21, 2018

Everything looks fine - Could we have this merged for 7.59.0 please? It's important for us.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Feb 21, 2018

Member

I'm really sorry, but I rather err on the safe side rather than hurry to get something in before the curtain falls. I have not had time yet to check this out closer and properly consider the API and function arguments etc - and nobody else has stepped up with feedback (neither positive nor negative). I need more time, so it will not get merged in this feature window. I apologize for this, but I've had a few very busy weeks at work lately and it has made me have less time to curl.

Member

bagder commented Feb 21, 2018

I'm really sorry, but I rather err on the safe side rather than hurry to get something in before the curtain falls. I have not had time yet to check this out closer and properly consider the API and function arguments etc - and nobody else has stepped up with feedback (neither positive nor negative). I need more time, so it will not get merged in this feature window. I apologize for this, but I've had a few very busy weeks at work lately and it has made me have less time to curl.

@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Feb 21, 2018

Member

@bagder I started reviewing it this afternoon. I'm changing some of the doc and internal naming to bring it in line. I can have it in before the deadline, let me know if that is a problem.

Member

jay commented Feb 21, 2018

@bagder I started reviewing it this afternoon. I'm changing some of the doc and internal naming to bring it in line. I can have it in before the deadline, let me know if that is a problem.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Feb 21, 2018

Member

@jay, I fully trust your judgement so if you're happy with it and think its fine to merge, I'm just happy. Go ahead!

Member

bagder commented Feb 21, 2018

@jay, I fully trust your judgement so if you're happy with it and think its fine to merge, I'm just happy. Go ahead!

@fsedano

This comment has been minimized.

Show comment
Hide comment
@fsedano

fsedano Feb 21, 2018

Contributor

@jay, @bagder really helpful. As mentioned, this feature is crucial for our project and we need to use an official curl version, so it's important for us. Please let me know if I can be of any help.

Thanks,

Contributor

fsedano commented Feb 21, 2018

@jay, @bagder really helpful. As mentioned, this feature is crucial for our project and we need to use an official curl version, so it's important for us. Please let me know if I can be of any help.

Thanks,

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

url: Add option CURLOPT_RESOLVER_START_FUNCTION
- Add new option CURLOPT_RESOLVER_START_FUNCTION

Closes curl#2311

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

url: Add option CURLOPT_RESOLVER_START_FUNCTION
- Add new option CURLOPT_RESOLVER_START_FUNCTION

Closes curl#2311

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

url: Add option CURLOPT_RESOLVER_START_FUNCTION
- Add new option CURLOPT_RESOLVER_START_FUNCTION to set a callback that
  will back called before resolver start (ie before a host is resolved)
  with a pointer to backend-specific resolver data.

- Add new option CURLOPT_RESOLVER_START_DATA to set a user pointer to
  pass to the resolver start callback.

Closes curl#2311

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

url: Add option CURLOPT_RESOLVER_START_FUNCTION
- Add new option CURLOPT_RESOLVER_START_FUNCTION to set a callback that
  will back called before resolver start (ie before a host is resolved)
  with a pointer to backend-specific resolver data.

- Add new option CURLOPT_RESOLVER_START_DATA to set a user pointer to
  pass to the resolver start callback.

Closes curl#2311

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

url: Add option CURLOPT_RESOLVER_START_FUNCTION
- Add new option CURLOPT_RESOLVER_START_FUNCTION to set a callback that
  will back called before resolver start (ie before a host is resolved)
  with a pointer to backend-specific resolver data.

- Add new option CURLOPT_RESOLVER_START_DATA to set a user pointer to
  pass to the resolver start callback.

Closes curl#2311

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

url: Add option CURLOPT_RESOLVER_START_FUNCTION
- Add new option CURLOPT_RESOLVER_START_FUNCTION to set a callback that
  will be called every time before a new resolve request is started
  (ie before a host is resolved) with a pointer to backend-specific
  resolver data. Currently this is only useful for ares.

- Add new option CURLOPT_RESOLVER_START_DATA to set a user pointer to
  pass to the resolver start callback.

Closes curl#2311

@jay jay closed this in 2371364 Feb 22, 2018

@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Feb 22, 2018

Member

I moved the callback into Curl_resolv right before Curl_getaddrinfo is called. This way it is called for all backends, even though currently it appears only to have a benefit for the ares backend. I also added a 'reserved' pointer to the callback in case we want to expand on it in the future. (For example allowing the user to receive the hostname/port and do their own resolve maybe?)

int resolver_start_cb(void *resolver_state, void *reserved, void *userdata);

The EXAMPLE in the doc I don't think is sufficient since it just shows resolver_state pointer address. It could really use an example that shows how the resolver_state should be used with ares, like if multiple hostnames are resolved do you set the socket multiple times? etc, or how in practice it is used.

Member

jay commented Feb 22, 2018

I moved the callback into Curl_resolv right before Curl_getaddrinfo is called. This way it is called for all backends, even though currently it appears only to have a benefit for the ares backend. I also added a 'reserved' pointer to the callback in case we want to expand on it in the future. (For example allowing the user to receive the hostname/port and do their own resolve maybe?)

int resolver_start_cb(void *resolver_state, void *reserved, void *userdata);

The EXAMPLE in the doc I don't think is sufficient since it just shows resolver_state pointer address. It could really use an example that shows how the resolver_state should be used with ares, like if multiple hostnames are resolved do you set the socket multiple times? etc, or how in practice it is used.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 14, 2018

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