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

curl_multi_get_handles - get all easy handles in a multi handle #11750

Closed
wants to merge 1 commit into from

Conversation

bagder
Copy link
Member

@bagder bagder commented Aug 28, 2023

include/curl/multi.h Outdated Show resolved Hide resolved
Copy link
Contributor

@MonkeybreadSoftware MonkeybreadSoftware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@bagder
Copy link
Member Author

bagder commented Aug 28, 2023

@jeroen you think this kind of API works for a binding such as yours, or would it be better served by an iterator-using one?

@jeroen
Copy link
Contributor

jeroen commented Aug 28, 2023

Yes this works for me, but my consideration is that this api is not extensible, if you would want to expose additional status information about each handle in the multi. For example, if the request is in queued, in progress, or done, and which connection it uses, incl http version, etc.

There is a possibility to make this a more generic API to list the status of all the requests in the multi, which is something that can be useful e.g. to debug multiplex issues, for example what we discussed here: https://curl.se/mail/lib-2023-02/0024.html

A fun example to toy with might be to improve the crawler to show detailed progress about the pending transfers, and be a bit smarter how to limit the number of requests based on what is already in the queue.

@bagder
Copy link
Member Author

bagder commented Aug 28, 2023

if you would want to expose additional status information about each handle in the multi. For example, if the request is in queued, in progress, or done, and which connection it uses, incl http version, etc.

If we want to provide that info - per easy handle - I think the *get_handles() API is not the right spot anyway. Then we should provide that via curl_easy_getinfo() so you'd iterate over the handles and query each handle for that info.

There is a possibility to make this a more generic API

You mean by instead creating a curl_multi_getinfo() function and making this all-easy-handles be a first "option" that this function can return?

@bagder
Copy link
Member Author

bagder commented Aug 29, 2023

  • add a CURLHND parameter to curl_multi_get_handles so that the user can request only their (user) easy handles, or internal handles, or both, in the multi.

Can we think of a use case where users actually want the internally created ones?

If they do, don't they then also want to know which kind each of them is? I mean, it seems like we should rather provide either non-internal or only-internal ones so that the caller knows what kind of handles they are. update: which is what this new option allows.

include/curl/multi.h Outdated Show resolved Hide resolved
@jay
Copy link
Member

jay commented Aug 29, 2023

Can we think of a use case where users actually want the internally created ones?

I can't. Maybe I overdid it I'll take away the filter parameter.

@jay
Copy link
Member

jay commented Aug 29, 2023

I should mention my hackish ListEasyHandles example from several years ago that I just updated :)

@rwmjones
Copy link

This would work for our use case.

@bagder bagder added the feature-window A merge of this requires an open feature window label Sep 7, 2023
@bagder bagder changed the title curl_multi_get_handles: test shot curl_multi_get_handles - get all easy handles in a multi handle Sep 11, 2023
@bagder bagder marked this pull request as ready for review September 11, 2023 07:20
bagder added a commit that referenced this pull request Sep 11, 2023
bagder added a commit that referenced this pull request Sep 21, 2023
@bagder
Copy link
Member Author

bagder commented Sep 25, 2023

Any further comments before we merge this first version?

Should we mark it experimental in a first round to make it possible to tweak the API ?

@rwmjones
Copy link

I read the updated patch series that you posted a few days ago and it still looks fine and would work for us. I have no view on whether it should be experimental or not.

@rwmjones
Copy link

I have no view on whether it should be experimental or not.

We would use it right away, but conditionalize the code behind a ./configure-time test, so for us it wouldn't matter.

@bagder
Copy link
Member Author

bagder commented Sep 25, 2023

I intend to merge this as-is and give everyone a chance to try it out for a few weeks. If we have any issues or problems showing up, we can mark it experimental before we do the next release. Otherwise it can ship as-is.

This function is pretty simple and straight-forward. I don't think it should cause many problems.

@bagder bagder closed this in 9ffd411 Sep 25, 2023
@rwmjones
Copy link

@bagder bagder deleted the bagder/multi-get-handles branch October 30, 2023 09:04
mmuman pushed a commit to mmuman/nbdkit that referenced this pull request Mar 4, 2024
Curl 8.3.1 will add a new experimental curl_multi_get_handles API
which can be used to read out the easy handles in a multi.  Currently
we have to store this separately.

If the new API is available, use it to remove our shadow list.

Thanks: Daniel Stenberg
Link: curl/curl#11750
Link: curl/curl@9ffd411
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-window A merge of this requires an open feature window libcurl API tests
Development

Successfully merging this pull request may close these issues.

6 participants