prereq: a new callback triggered before a request is made #7477

maxdymond · 2021-07-22T14:35:52Z

…after a connection is set up

This is an implementation of the code and tests described in #7471 to add a new callback at the start of MSTATE_DO.

Still needs docs writing, with an example
Still needs a redirect test.

lib/multi.c

bagder · 2021-07-26T20:16:28Z

lib/multi.c

+                          data->info.conn_scheme,
+                          data->info.conn_protocol,
+                          data->info.conn_primary_port,
+                          data->info.conn_local_port);


Should we consider allowing curl_easy_getinfo() calls from within this callback to retrieve all that information instead of passing these six specific arguments?

Interesting. Maybe? That's a departure from all existing callbacks AFAIK and I don't know if accessing curl_easy_getinfo() is supported during callbacks during a transfer - I thought it was only a post-transfer thing. This is going off vague memories from IRC though.

@bagder is there anything else in the code that uses curl_easy_getinfo in flight? I'd have thought there was a danger of some variables having not been populated.

tests/data/test2082

bagder · 2021-07-26T20:18:43Z

tests/libtest/Makefile.inc

@@ -36,7 +36,7 @@ SUPPORTFILES = first.c test.h

 # These are all libcurl test programs
 noinst_PROGRAMS = chkhostname libauthretry libntlmconnect                \
- chkdecimalpoint                                                         \
+ chkdecimalpoint libprereq                                               \


Why doesn't this follow the pattern of being named lib[num] ?

Because it's generic over a number of different tests. AFAICT this is the pattern for that (see libntlmconnect).

maxdymond · 2021-08-02T12:18:32Z

Hitting a weird problem in some tests where CR/LFs are being introduced. The protocol especially doesn't make sense because it's just a number...

 Connected to: 127.0.0.1[LF]
 Connected from: 127.0.0.1[LF]
-Scheme: HTTP[CR][LF]
-Protocol: 1[CR][LF]
+Scheme: HTTP[LF]
+Protocol: 1[LF]

The function is literally only doing

  printf("Protocol: %d\n", conn_protocol);

and the test file isn't in Windows mode. @bagder have you ever seen this before?

maxdymond · 2021-08-02T14:36:49Z

and the test file isn't in Windows mode. @bagder have you ever seen this before?

Looks like the subNewlines function in Hyper test-mode is a little trigger happy when it sees lines of the format

Something: somevalue

and because the scheme lines and protocol lines aren't being modified by later functions, they're the only two to look weird.

maxdymond · 2021-08-03T09:54:45Z

AFAICT the remaining build failures are not my fault.

bagder · 2021-08-09T13:41:34Z

Looks like the subNewlines function in Hyper test-mode is a little trigger happy

Yes, but as long as you can just make other output to not look like HTTP headers it is very easily avoided. The "hyper mode" thing is a bit annoying but I haven't been able to come up with a better take on it.

maxdymond · 2021-08-10T10:08:42Z

Rebasing as recommended on IRC 😄

maxdymond · 2021-08-11T07:55:20Z

@bagder looks like all but one test succeeds now (yay) but I don't understand the AppVeyor error (1502 failing), given that it succeeds on all the other AppVeyor builds.

bagder

It looks fine, I'm mostly having some final thoughts the specific arguments passed to the callback...

docs/libcurl/opts/CURLOPT_PREREQFUNCTION.3

sterchelen · 2021-08-18T19:53:25Z

lib/multi.c

+        if(prereq_rc != CURL_PREREQFUNC_OK) {
+          failf(data, "operation aborted by pre-request callback");
+          callback_failure = TRUE;
+        }
+      }
+
+      if(callback_failure) {
+        /* failure in pre-request callback - don't do any other processing  */
+        result = CURLE_ABORTED_BY_CALLBACK;
+        Curl_posttransfer(data);
+        multi_done(data, result, FALSE);
+        stream_error = TRUE;
+      }
+      else if(data->set.connect_only) {


As you want to stop the execution if the pre-request callback fails we could break early and avoid modifying the control flow? (maybe a bit nitpicking :))

case MSTATE_DO: if(data->set.fprereq) { ... if (failure){ break; } } if(data->set.connect_only) { ... } else { ... }

Suggested change

if(prereq_rc != CURL_PREREQFUNC_OK) {

failf(data, "operation aborted by pre-request callback");

callback_failure = TRUE;

}

}

if(callback_failure) {

/* failure in pre-request callback - don't do any other processing */

result = CURLE_ABORTED_BY_CALLBACK;

Curl_posttransfer(data);

multi_done(data, result, FALSE);

stream_error = TRUE;

}

else if(data->set.connect_only) {

if(prereq_rc != CURL_PREREQFUNC_OK) {

failf(data, "operation aborted by pre-request callback");

/* failure in pre-request callback - don't do any other processing */

result = CURLE_ABORTED_BY_CALLBACK;

Curl_posttransfer(data);

multi_done(data, result, FALSE);

stream_error = TRUE;

break;

}

}

if(data->set.connect_only) {

Seems like a reasonable thing to do. I was trying hard not to mess up any of the existing call flow but yeah, an early break makes sense.

maxdymond · 2021-08-19T10:49:42Z

@bagder if this isn't getting into the next release due to the feature freeze can you please let me know so I can update the versions in the documentation.

bagder · 2021-08-20T06:08:11Z

if this isn't getting into the next release due to the feature freeze can you please let me know so I can update the versions in the documentation.

Sorry about that but yes we (I) didn't manage to clear out everything for merge before the freeze so now we have a few more weeks to iron out any remaining wrinkles...

maxdymond · 2021-08-20T08:33:12Z

if this isn't getting into the next release due to the feature freeze can you please let me know so I can update the versions in the documentation.

Sorry about that but yes we (I) didn't manage to clear out everything for merge before the freeze so now we have a few more weeks to iron out any remaining wrinkles...

Ugh, frustrating. Is it even worth me rebasing now, or should I wait until the next release is cut and rebase onto there?

maxdymond · 2021-09-02T10:16:35Z

Having done some more testing on this in my end product, it looks like Curl_persistconninfo can get called for some connections with local_ip = "" and local_port = -1 before going into MULTI_DO - probably as a result of reusing the connection, but not definitely.

Curl_persistconninfo only gets called for url.c in reuse_conn and (create_conn if PROTOPT_NONETWORK, so file:///), and in connect.c in Curl_updateconninfo. In both cases it can be called with ""/-1 if some logic fails.

Will try and get to the bottom of this.

(EDIT: This was a bug in curl! Has been fixed)

maxdymond · 2021-09-15T08:48:23Z

Now rebased onto 7.79.0.

bagder · 2021-09-27T06:38:24Z

It seems this now conflicts with other merged new options, can you please fix that and force-push and I think we'll be ready to land this baby!

…after a connection is set up Commits: - callback: Update docs and callback for pre-request callback - Add some documentation for CURLOPT_PREREQDATA and CURLOPT_PREREQFUNCTION, as well as adding the option to stop a transfer from happening by returning a specific callback code. - Add redirect test and callback failure test - Note that the function may be called multiple times on a redirection - Disable new 2086 test due to Windows weirdness

maxdymond · 2021-09-27T08:47:03Z

@bagder should be sorted now 🤞

bagder · 2021-09-27T15:18:27Z

Thanks!

maxdymond force-pushed the md/connectionestablishedcallback branch from 04f6eca to e7b26eb Compare July 22, 2021 14:52

bagder reviewed Jul 26, 2021

View reviewed changes

maxdymond mentioned this pull request Aug 2, 2021

progress: Trigger the progress function at the beginning of MSTATE_DO #7471

Closed

maxdymond force-pushed the md/connectionestablishedcallback branch from 4100f7c to 411b7e3 Compare August 3, 2021 08:20

maxdymond force-pushed the md/connectionestablishedcallback branch from 411b7e3 to 045959c Compare August 10, 2021 10:08

jay added the libcurl API label Aug 10, 2021

bagder reviewed Aug 13, 2021

View reviewed changes

bagder reviewed Aug 16, 2021

View reviewed changes

docs/libcurl/opts/CURLOPT_PREREQFUNCTION.3 Show resolved Hide resolved

maxdymond force-pushed the md/connectionestablishedcallback branch from 6686acb to 569f1d0 Compare August 16, 2021 11:32

bagder changed the title ~~callback: add a new callback, triggered before a request is made but …~~ prereq: a new callback triggered before a request is made Aug 16, 2021

sterchelen reviewed Aug 18, 2021

View reviewed changes

bagder self-assigned this Aug 20, 2021

bagder added the feature-window A merge of this requires an open feature window label Aug 20, 2021

maxdymond force-pushed the md/connectionestablishedcallback branch from e7ebf95 to c20d7aa Compare September 15, 2021 08:47

maxdymond force-pushed the md/connectionestablishedcallback branch from c20d7aa to 29d5c91 Compare September 22, 2021 14:51

maxdymond force-pushed the md/connectionestablishedcallback branch from 29d5c91 to 6b95987 Compare September 27, 2021 08:43

bagder closed this in a517378 Sep 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

prereq: a new callback triggered before a request is made #7477

prereq: a new callback triggered before a request is made #7477

prereq: a new callback triggered before a request is made #7477

prereq: a new callback triggered before a request is made #7477

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment