progress callback draft #433

ITotalJustice · 2020-08-08T20:20:18Z

Hi! I added an issue here #432 a few minutes ago, but then i realised that there was an already existing issue that's been open for quite a while sadly #111.

This is a quick draft / example of adding a progress callback option, and it works 😄 . This allows us to create progress bars and also cancel any request early by returning a non zero value from the callback!

If there's any chance of adding this and you want me to clean up anything, let me know! Thanks

ITotalJustice · 2020-08-08T22:45:09Z

Made some changes to the initial pr. I simple class that the user passes a function callback and a data that they want to be passed into callback (this was missing from my initial pr). I think this looks much better that the initial pr.

ITotalJustice · 2020-08-08T23:36:53Z

I was thinking, would it be a good idea to create 2 more classes for write and read callbacks? I could rename progresscb.h to callback.h and then have the 3 classes there.

Iirc there's a few more libcurl callbacks also, such as headers. Maybe it might be a good option for me to add those also?

EDIT: Not sure why one of the tests fails. It says it's the callback test. I checked if maybe i named something the same as a func / var in the test, but i haven't. I did change 2 const vars to constexpr, but surely it can't be that 🤔

ITotalJustice · 2020-08-09T00:02:58Z

I noticed that a lot of the time, people call curl_easy_getinfo in the progress callback. A way to still support this would be give curl a callback created cpr, which when called, calls the user callback also passing in a const *curl.

Or I think a better way would to pass a const struct / class to the user callback. This would simplify the callback that the user has to create as it will only have one pram in the function, then access only what they want from the struct / class.

One immediate problem is that let's say the user returns != 0 from the user callback (meaning that they want to exit), this value would actually be returned back to the cpr function. I mean, the cpr class can just save that value and then return that value on the next progress call, but it wouldn't be immediate. However it should be fast enough, and i think it is 100% worth that trade-off of a max 1 second delay (usually only like 0.3 second) to instead allow the user full access to curl_easy_getinfo.

EDIT: After reading the easy curl header, i found this in regards to the curl_easy_getinfo This function is intended to get used AFTER a performed transfer, all results from this function are undefined until the transfer is completed.. I am not sure why a lot of liburls examples go completely against this statement...
If the results are undefined then this makes this whole idea (in this message) pretty irrelevant.

include/cpr/progresscb.h

COM8 · 2020-08-09T06:27:51Z

Hi @ITotalJustice, thank you for taking the time to add support for this this feature to CPR!

COM8 · 2020-08-09T06:32:05Z

I was thinking, would it be a good idea to create 2 more classes for write and read callbacks? I could rename progresscb.h to callback.h and then have the 3 classes there.

Iirc there's a few more libcurl callbacks also, such as headers. Maybe it might be a good option for me to add those also?

I agree. This would be a great idea. I'd say go for it.

EDIT: Not sure why one of the tests fails. It says it's the callback test. I checked if maybe i named something the same as a func / var in the test, but i haven't. I did change 2 const vars to constexpr, but surely it can't be that thinking

Don't worry about macOS. It fails for every second commit and we currently do not know why.

COM8 · 2020-08-09T06:40:22Z

[...]
EDIT: After reading the easy curl header, i found this in regards to the curl_easy_getinfo This function is intended to get used AFTER a performed transfer, all results from this function are undefined until the transfer is completed.. I am not sure why a lot of liburls examples go completely against this statement...
If the results are undefined then this makes this whole idea (in this message) pretty irrelevant.

I also do not think, we should use curl_easy_getinfo for progress callbacks.
To me it looks like a good idea to give the user access to the curl_easy_getinfo results, but those results should then be made available after the request in the Response class of the request.
I will create a PR for this.

COM8 · 2020-08-09T06:43:04Z

Thinks that have to be done before a merge:

Please create a PR against the gh-pages branche to add documentation for this great feature.
Update the README.md features list.
Add unit tests

ITotalJustice · 2020-08-09T09:57:32Z

I've rebased against master, however it's now showing the master commit history in this PR. Is this correct? It's my first time rebasing haha, I hope that I didn't mess anything up.

I'll work on the documentation now anyway.

COM8 · 2020-08-09T10:09:17Z

I've rebased against master, however it's now showing the master commit history in this PR. Is this correct? It's my first time rebasing haha, I hope that I didn't mess anything up.

I'll work on the documentation now anyway.

Interesting, but should be fine :)

ITotalJustice · 2020-08-09T12:37:54Z

So i've changed how the progress callback works. no longer is the user function directly passed to libcurl, instead, we wrap the function and pass everything in a nice tidy struct like this:

int ProgressCallbackFunction(ProgressCallbackData* data, cpr_off_t dlnow, cpr_off_t dltotal, cpr_off_t ulnow, cpr_off_t ultotal) {
    data->data.downloadNow = dlnow;
    data->data.downloadTotal = dltotal;
    data->data.uploadNow = ulnow;
    data->data.uploadTotal = ultotal;
    return data->cb(&data->data);
  }

which then for the user, all they have to do is this:

int myCallback(cpr::ProgressCallbackUser *data) {
    MyData *myData = (MyData*)data->userData;
    std::cout << myData->message << std::endl;
    return 0;
}

I think this is ideal because it makes the callback struct for the user much simpler (mirrors how the response class works) and also because we can add more info to the struct if libcurl api ever changes without the user having to change anything.

The names i've given everything are just temporary, so let me know what you would like to rename them as. Also let me know where I should put the structs / function, or are you happy with where they are?

EDIT: I tried to see if there's a way to use a template for the userdata, that way the user wouldn't have to cast the void*, but I couldn't get it to work. Do you think it's possible / worth looking into?

ITotalJustice · 2020-08-09T14:48:40Z

Sorry for making another big change 😄

I've added read and write callback support. This will allow for users to write data to disk in chunks, rather than storing it all in memory. This closes a few more issues such as #255 #208 #111 maybe #25 #271

include/cpr/callback.h

COM8 · 2020-08-09T16:22:59Z

I think something went wrong during the rebase earlier.
I'd suggest you try it again, since there are a lot of conflicts, that should not be conflicts.
For reference: https://gist.github.com/ravibhure/a7e0918ff4937c9ea1c456698dcd58aa

include/cpr/callback.h

this will be needed for unit tests to make sure that cancelling the request works. Also useful for the user to know if curl finished the request *before* the request was cancelled, as maybe then the user might still cache some data (like images or auth login etc...)

the docs specify `char*` but actually use `void*` in every example... So I guess it doesn't matter, but probably better to use the generic void*.

also added space at the end of callback.h file

ITotalJustice · 2020-08-09T16:46:39Z

I rebased following the guide that you sent (thank you!). Did this fix the issues?

xloem · 2020-09-17T17:43:14Z

Hey, I found this PR and see it's been sitting for about a month. Do you guys need help with the tests and documentation situation, or do you just need some time?

COM8 · 2020-09-18T07:48:26Z

Only the documentation for the return values is still pending.
Besides that, this PR is ready to be merged.

xloem · 2020-09-18T11:49:10Z

cpr/session.cpp

+void Session::Impl::SetReadCallback(const ReadCallback& read) {
+    CURL* curl = curl_->handle;
+    if (curl) {
+        curl_easy_setopt(curl, CURLOPT_READFUNCTION , read.callback.cb);


I'm looking at this patch a little and noticed that CURLOPT_READFUNCTION is assigned a value of a type of int(*read_cb_t)(ReadCallbackUser*); (types in callback.h), whereas https://curl.haxx.se/libcurl/c/CURLOPT_READFUNCTION.html specifies a type of size_t (*read_callback)(char *buffer, size_t size, size_t nitems, void *userdata);. It seems like that would cause a crash if used, due to the change: am I mistaken?

xloem · 2020-09-18T11:54:36Z

cpr/session.cpp

+    if (curl) {
+        curl_easy_setopt(curl, CURLOPT_READFUNCTION , read.callback.cb);
+        curl_easy_setopt(curl, CURLOPT_READDATA, &read.callback.data);
+        this->userRead_ = true;


this member variable appears unused

xloem · 2020-09-18T11:56:47Z

I'm not sure this PR is complete; it kind of looks like a proof of concept waiting for confirmation around design choices, maybe even for somebody else to finish when needed. It looks like most of the work is in the progress callback. I might try to contribute a little.

ITotalJustice · 2020-09-18T12:02:59Z

Just got an email notification for this, very sorry again for late to replying (can't believe it's been a month already!)

@xloem yeah it was mainly a quick draft. i put it together in like an hours because i needed to early cancel api requests. There's a few(?) unused variables as i changed the design of the pr a few times over. It would be great if you can contribute to the pr (or a new pr). I don't really have much free time sadly

xloem · 2020-09-19T11:00:14Z

I'm spending some time bringing this together locally. I expect to finish the tests, likely the docs.

xloem · 2020-09-19T12:49:44Z

Completed work for this PR at ITotalJustice#1

COM8 · 2020-09-22T08:53:04Z

Closing this one in favour of #446.

* add documentation for ProgressCallback For #433 * fix my example code * start adding info about read / write callbacks * Update advanced-usage.md requested changes #1 * Updated docs for my changes in other branch, including mentioning return values. * forgot to mention DebugCallback in documentation; added it * Update advanced-usage.md * Update advanced-usage.md * Update advanced-usage.md * Update advanced-usage.md * Update advanced-usage.md Co-authored-by: ITotalJustice <47043333+ITotalJustice@users.noreply.github.com> Co-authored-by: Fabian Sauter <sauter.fabian@mailbox.org>

This was linked to issues Aug 9, 2020

Add an option or callback to cancel a request #432

Closed

Feature Request: Progress callback #111

Closed

COM8 added the Feature ✨ label Aug 9, 2020

COM8 added this to the CPR 1.6.0 milestone Aug 9, 2020

COM8 requested changes Aug 9, 2020

View reviewed changes

include/cpr/progresscb.h Outdated Show resolved Hide resolved

include/cpr/progresscb.h Outdated Show resolved Hide resolved

include/cpr/progresscb.h Outdated Show resolved Hide resolved

COM8 added Documentation Needed 📗 Tests Needed 🚨 labels Aug 9, 2020

COM8 added a commit that referenced this pull request Aug 9, 2020

Added wrapper around curl_off_t for #433

943194f

ITotalJustice added a commit to ITotalJustice/cpr that referenced this pull request Aug 9, 2020

add documentation for ProgressCallback For libcpr#433

0383e98

This was linked to issues Aug 9, 2020

Request feature - Callbacks support #255

Closed

It's possible add asynchronous callback support on session ? #208

Closed

Streamed requests #25

Closed

Streaming Download #271

Closed

COM8 requested changes Aug 9, 2020

View reviewed changes

include/cpr/callback.h Outdated Show resolved Hide resolved

COM8 reviewed Aug 9, 2020

View reviewed changes

include/cpr/callback.h Outdated Show resolved Hide resolved

ITotalJustice added 3 commits August 9, 2020 17:32

add callback draft

833711e

improve callback imp (allows for userdata)

1a96a9e

option for user just to set the callback and *not* and userdata

0e2d401

ITotalJustice added 13 commits August 9, 2020 17:32

add support for older versions of libcurl

be9c249

add methods to manually add callback and userdata ptr

6f548fe

use wrapper cpr_off_t. fix * placement

748ac53

add callback draft

c07b521

improve callback imp (allows for userdata)

3853853

add new feature to readme

ad92bf6

simplify the user callback *a lot*

c252ee3

update curlopt for callback changes

80c2679

added callback support for read and write

f9561fa

update download api request for user callbacks

b6cd6e0

change r/w callback buffers to void*

05549f6

the docs specify `char*` but actually use `void*` in every example... So I guess it doesn't matter, but probably better to use the generic void*.

fix download api call (forgot to update var)

9cea9c7

also added space at the end of callback.h file

ITotalJustice force-pushed the add-progress-callback branch from 36938a6 to 9cea9c7 Compare August 9, 2020 16:45

rename callback params

f4c8383

COM8 approved these changes Aug 9, 2020

View reviewed changes

xloem reviewed Sep 18, 2020

View reviewed changes

COM8 closed this Sep 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

progress callback draft #433

progress callback draft #433

ITotalJustice commented Aug 8, 2020

ITotalJustice commented Aug 8, 2020

ITotalJustice commented Aug 8, 2020 •

edited

Loading

ITotalJustice commented Aug 9, 2020 •

edited

Loading

COM8 commented Aug 9, 2020

COM8 commented Aug 9, 2020

COM8 commented Aug 9, 2020

COM8 commented Aug 9, 2020

ITotalJustice commented Aug 9, 2020

COM8 commented Aug 9, 2020

ITotalJustice commented Aug 9, 2020 •

edited

Loading

ITotalJustice commented Aug 9, 2020

COM8 commented Aug 9, 2020

ITotalJustice commented Aug 9, 2020

xloem commented Sep 17, 2020

COM8 commented Sep 18, 2020

xloem Sep 18, 2020

xloem Sep 18, 2020

xloem commented Sep 18, 2020

ITotalJustice commented Sep 18, 2020

xloem commented Sep 19, 2020

xloem commented Sep 19, 2020

COM8 commented Sep 22, 2020

progress callback draft #433

progress callback draft #433

Conversation

ITotalJustice commented Aug 8, 2020

ITotalJustice commented Aug 8, 2020

ITotalJustice commented Aug 8, 2020 • edited Loading

ITotalJustice commented Aug 9, 2020 • edited Loading

COM8 commented Aug 9, 2020

COM8 commented Aug 9, 2020

COM8 commented Aug 9, 2020

COM8 commented Aug 9, 2020

ITotalJustice commented Aug 9, 2020

COM8 commented Aug 9, 2020

ITotalJustice commented Aug 9, 2020 • edited Loading

ITotalJustice commented Aug 9, 2020

COM8 commented Aug 9, 2020

ITotalJustice commented Aug 9, 2020

xloem commented Sep 17, 2020

COM8 commented Sep 18, 2020

xloem Sep 18, 2020

Choose a reason for hiding this comment

xloem Sep 18, 2020

Choose a reason for hiding this comment

xloem commented Sep 18, 2020

ITotalJustice commented Sep 18, 2020

xloem commented Sep 19, 2020

xloem commented Sep 19, 2020

COM8 commented Sep 22, 2020

ITotalJustice commented Aug 8, 2020 •

edited

Loading

ITotalJustice commented Aug 9, 2020 •

edited

Loading

ITotalJustice commented Aug 9, 2020 •

edited

Loading