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

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

Closed

Conversation

@maxdymond
Copy link
Contributor

@maxdymond maxdymond commented Jul 22, 2021

…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.
@maxdymond maxdymond force-pushed the md/connectionestablishedcallback branch from 04f6eca to e7b26eb Jul 22, 2021
lib/multi.c Outdated Show resolved Hide resolved
lib/multi.c Outdated
data->info.conn_scheme,
data->info.conn_protocol,
data->info.conn_primary_port,
data->info.conn_local_port);
Copy link
Member

@bagder bagder Jul 26, 2021

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

Copy link
Contributor Author

@maxdymond maxdymond Aug 2, 2021

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.

Copy link
Contributor Author

@maxdymond maxdymond Aug 10, 2021

@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 Outdated Show resolved Hide resolved
@@ -36,7 +36,7 @@ SUPPORTFILES = first.c test.h

# These are all libcurl test programs
noinst_PROGRAMS = chkhostname libauthretry libntlmconnect \
chkdecimalpoint \
chkdecimalpoint libprereq \
Copy link
Member

@bagder bagder Jul 26, 2021

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

Copy link
Contributor Author

@maxdymond maxdymond Aug 2, 2021

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

@maxdymond
Copy link
Contributor Author

@maxdymond maxdymond commented Aug 2, 2021

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

@maxdymond maxdymond commented Aug 2, 2021

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 maxdymond force-pushed the md/connectionestablishedcallback branch from 4100f7c to 411b7e3 Aug 3, 2021
@maxdymond
Copy link
Contributor Author

@maxdymond maxdymond commented Aug 3, 2021

AFAICT the remaining build failures are not my fault.

@bagder
Copy link
Member

@bagder bagder commented Aug 9, 2021

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 maxdymond force-pushed the md/connectionestablishedcallback branch from 411b7e3 to 045959c Aug 10, 2021
@maxdymond
Copy link
Contributor Author

@maxdymond maxdymond commented Aug 10, 2021

Rebasing as recommended on IRC 😄

@maxdymond
Copy link
Contributor Author

@maxdymond maxdymond commented Aug 11, 2021

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

Copy link
Member

@bagder bagder left a comment

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

docs/libcurl/opts/CURLOPT_PREREQFUNCTION.3 Outdated Show resolved Hide resolved
docs/libcurl/opts/CURLOPT_PREREQFUNCTION.3 Outdated Show resolved Hide resolved
docs/libcurl/opts/CURLOPT_PREREQFUNCTION.3 Outdated Show resolved Hide resolved
docs/libcurl/opts/CURLOPT_PREREQFUNCTION.3 Show resolved Hide resolved
@maxdymond maxdymond force-pushed the md/connectionestablishedcallback branch from 6686acb to 569f1d0 Aug 16, 2021
@bagder 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
lib/multi.c Outdated
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) {
Copy link
Contributor

@sterchelen sterchelen Aug 18, 2021

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) {

Copy link
Contributor Author

@maxdymond maxdymond Aug 19, 2021

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

@maxdymond maxdymond commented Aug 19, 2021

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

@bagder bagder commented Aug 20, 2021

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...

@bagder bagder self-assigned this Aug 20, 2021
@maxdymond
Copy link
Contributor Author

@maxdymond maxdymond commented Aug 20, 2021

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

@maxdymond maxdymond commented Sep 2, 2021

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 maxdymond force-pushed the md/connectionestablishedcallback branch from e7ebf95 to c20d7aa Sep 15, 2021
@maxdymond
Copy link
Contributor Author

@maxdymond maxdymond commented Sep 15, 2021

Now rebased onto 7.79.0.

@maxdymond maxdymond force-pushed the md/connectionestablishedcallback branch from c20d7aa to 29d5c91 Sep 22, 2021
@bagder
Copy link
Member

@bagder bagder commented Sep 27, 2021

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 maxdymond force-pushed the md/connectionestablishedcallback branch from 29d5c91 to 6b95987 Sep 27, 2021
@maxdymond
Copy link
Contributor Author

@maxdymond maxdymond commented Sep 27, 2021

@bagder should be sorted now 🤞

@bagder bagder closed this in a517378 Sep 27, 2021
@bagder
Copy link
Member

@bagder bagder commented Sep 27, 2021

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants