Add support for CURLOPT_RESOLVER_START callback #2311

fsedano · 2018-02-14T17:30:15Z

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 · 2018-02-15T07:58:34Z

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 · 2018-02-15T08:02:02Z

docs/libcurl/opts/CURLOPT_RESOLVER_START_FUNCTION.3

+.nf
+#include <curl/curl.h>
+
+void resolver_alloc_cb (void *resolver_data, void *user_data);


I think you need to allow this function to return at least an error for whatever went wrong in the callback that makes the code want to stop the transfer. So maybe just an int and let 0 mean OK, 1 means "error, stop, get out" and the rest left reserved for the future?

bagder · 2018-02-15T08:04:14Z

docs/libcurl/opts/CURLOPT_RESOLVER_START_FUNCTION.3

+This will return CURLE_OK.
+.SH EXAMPLE
+A common use case is to use this callback to setup any options on the ares
+channel, for example socket callbacks.


I think the EXAMPLE section should show actual code using the option.

bagder · 2018-02-15T08:06:35Z

docs/libcurl/opts/CURLOPT_RESOLVER_START_DATA.3

+.\" *                            | (__| |_| |  _ <| |___
+.\" *                             \___|\___/|_| \_\_____|
+.\" *
+.\" * Copyright (C) 1998 - 2015, Daniel Stenberg, <daniel@haxx.se>, et al.


I think using 2018 as the end year in the copyright year range from start seems appropriate! =)

bagder · 2018-02-15T08:06:47Z

docs/libcurl/opts/CURLOPT_RESOLVER_START_FUNCTION.3

+.\" *                            | (__| |_| |  _ <| |___
+.\" *                             \___|\___/|_| \_\_____|
+.\" *
+.\" * Copyright (C) 1998 - 2014, Daniel Stenberg, <daniel@haxx.se>, et al.


bagder · 2018-02-15T08:07:24Z

include/curl/curl.h

+  /* User data to pass to the resolver start callback */
+  CINIT(RESOLVER_START_DATA, OBJECTPOINT, 272),
+
+


extra newline, let's stick to only one between each option

bagder · 2018-02-15T08:07:54Z

lib/hostasyn.c

+
+  /* Call the resolver start callback, if defined */
+  if(data->set.resolver_callbacks.resolver_start_cb) {
+    data->set.resolver_callbacks.


so here I would argue there should be a return code and handling of that, as mentioned above

Sure, thanks for the suggestion - I've fixed the failing TCs and added the example, will add the two other issues you mention (recursive CB attempt + return code) on a subsequent commit. Thanks for the help.

Just submitted code with the requested changes, updated TCs and doc.
Thanks,

bagder · 2018-02-15T08:51:41Z

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 · 2018-02-16T07:51:51Z

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

bagder · 2018-02-16T07:54:12Z

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

fsedano · 2018-02-20T16:50:49Z

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 · 2018-02-20T19:38:25Z

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 · 2018-02-21T09:31:41Z

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 · 2018-02-21T09:34:31Z

"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 · 2018-02-21T09:48:10Z

Working on the conflicts

- 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 · 2018-02-21T12:05:08Z

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

bagder · 2018-02-21T22:06:01Z

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 · 2018-02-21T22:26:14Z

@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 · 2018-02-21T22:31:56Z

@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 · 2018-02-21T22:34:27Z

@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,

- Add new option CURLOPT_RESOLVER_START_FUNCTION Closes curl#2311

- 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

- 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 · 2018-02-22T02:38:59Z

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.

bagder reviewed Feb 15, 2018

View reviewed changes

bagder added the connecting & proxies label Feb 15, 2018

fsedano force-pushed the master branch from b018142 to 8ce51a0 Compare February 15, 2018 23:38

fsedano and others added 14 commits February 21, 2018 10:18

Add support for CURLOPT_RESOLVER_START callback

d6bd462

Add testcase for CURLOPT_RESOLVER_START

6a9f41a

Fix broken testcase compilation

b99fccd

Address review comments

d94449f

Add return code to callback. Add recursive callback detection

bbdeffb

Add support for CURLOPT_RESOLVER_START callback

5cfe830

Address review comments

34377a7

Add support for CURLOPT_RESOLVER_START callback

8e744a8

Address review comments

be9b5dc

Fix merge conflict

041b6f5

Add support for CURLOPT_RESOLVER_START callback

bcaf6b3

Address review comments

f4c2420

Fix merge conflicts

0710174

fsedano force-pushed the master branch from 8d64e4b to 0710174 Compare February 21, 2018 10:22

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

url: Add option CURLOPT_RESOLVER_START_FUNCTION

b4696c2

- Add new option CURLOPT_RESOLVER_START_FUNCTION Closes curl#2311

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

url: Add option CURLOPT_RESOLVER_START_FUNCTION

658acf9

- Add new option CURLOPT_RESOLVER_START_FUNCTION Closes curl#2311

jay closed this in 2371364 Feb 22, 2018

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for CURLOPT_RESOLVER_START callback #2311

Add support for CURLOPT_RESOLVER_START callback #2311

fsedano commented Feb 14, 2018

bagder commented Feb 15, 2018

bagder Feb 15, 2018 •

edited

bagder Feb 15, 2018

bagder Feb 15, 2018

bagder Feb 15, 2018

bagder Feb 15, 2018

bagder Feb 15, 2018

fsedano Feb 15, 2018

fsedano Feb 20, 2018

bagder commented Feb 15, 2018

fsedano commented Feb 16, 2018

bagder commented Feb 16, 2018

fsedano commented Feb 20, 2018

jay commented Feb 20, 2018

fsedano commented Feb 21, 2018

bagder commented Feb 21, 2018

fsedano commented Feb 21, 2018

fsedano commented Feb 21, 2018

bagder commented Feb 21, 2018 •

edited

jay commented Feb 21, 2018

bagder commented Feb 21, 2018

fsedano commented Feb 21, 2018

jay commented Feb 22, 2018

		/* User data to pass to the resolver start callback */
		CINIT(RESOLVER_START_DATA, OBJECTPOINT, 272),

Add support for CURLOPT_RESOLVER_START callback #2311

Add support for CURLOPT_RESOLVER_START callback #2311

Conversation

fsedano commented Feb 14, 2018

bagder commented Feb 15, 2018

bagder Feb 15, 2018 • edited

Choose a reason for hiding this comment

bagder Feb 15, 2018

Choose a reason for hiding this comment

bagder Feb 15, 2018

Choose a reason for hiding this comment

bagder Feb 15, 2018

Choose a reason for hiding this comment

bagder Feb 15, 2018

Choose a reason for hiding this comment

bagder Feb 15, 2018

Choose a reason for hiding this comment

fsedano Feb 15, 2018

Choose a reason for hiding this comment

fsedano Feb 20, 2018

Choose a reason for hiding this comment

bagder commented Feb 15, 2018

fsedano commented Feb 16, 2018

bagder commented Feb 16, 2018

fsedano commented Feb 20, 2018

jay commented Feb 20, 2018

fsedano commented Feb 21, 2018

bagder commented Feb 21, 2018

fsedano commented Feb 21, 2018

fsedano commented Feb 21, 2018

bagder commented Feb 21, 2018 • edited

jay commented Feb 21, 2018

bagder commented Feb 21, 2018

fsedano commented Feb 21, 2018

jay commented Feb 22, 2018

bagder Feb 15, 2018 •

edited

bagder commented Feb 21, 2018 •

edited