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

Fix cURL deprecated warning #171

Merged
merged 1 commit into from
Mar 21, 2024
Merged

Fix cURL deprecated warning #171

merged 1 commit into from
Mar 21, 2024

Conversation

ThePedroo
Copy link
Contributor

Notice

  • I understand the code that I have edited, and have the means
    to test it before making changes to Concord.

What?

This PR "replaces" (deprecated) curl_multi_socket_all with curl_multi_socket_action.

Why?

As mentioned before, _all is deprecated since 7.19.5. In other words, versions above or equal 7.19.5 should not use it, and should use _action instead. Concord requires libcurl 7.56.1 minimum, which makes its usage not recommended.

How?

This PR introduces the usage of curl_multi_socket_action (as per "recommended" by libcurl documentation) together with curl_multi_perform, replacing curl_multi_socket_all.

Testing?

To test the minimum "workability" of this PR, a bot was compiled with this PR.

Anything Else?

Those changes are slightly experimental, as no explicit replacement is made in libcurl documentation, only for curl_multi_socket.

I may also point out that even if this PR is not introduced, curl_multi_socket_all usage is not recommended.

@ThePedroo ThePedroo requested a review from lcsmuller February 1, 2024 07:21
@lcsmuller
Copy link
Collaborator

Looks good, have you tested it?

core/io_poller.c Outdated Show resolved Hide resolved
@@ -360,7 +360,7 @@ discord_requestor_info_read(struct discord_requestor *rqtor)
{
int alive = 0;

if (CURLM_OK != curl_multi_socket_all(rqtor->mhandle, &alive))
if (CURLM_OK != curl_multi_socket_action(rqtor->mhandle, 0, 0, &alive))
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to check what != CURLM_OK means for curl_multi_socket_action(). Does it actually translates to a CCORD_CURLM_INTERNAL error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot_20240201_210120_Chrome

No documented change.

@Anotra
Copy link
Contributor

Anotra commented Feb 2, 2024

according to https://curl.se/libcurl/c/curl_multi_socket_action.html
you should call
6. Call [curl_multi_socket_action](https://curl.se/libcurl/c/curl_multi_socket_action.html)(..., CURL_SOCKET_TIMEOUT, 0, ...) to kickstart everything. To get one or more callbacks called.
instead of 0, which is a legitimate file descriptor. I haven't tested this, and am too lazy to right now.

This commit fixes a deprecated warning due to the usage of "curl_multi_socket_all" in libcurl versions.
@ThePedroo ThePedroo force-pushed the fix/curl-deprecated-warning branch from f5fd637 to 1f2a958 Compare February 2, 2024 23:41
@ThePedroo
Copy link
Contributor Author

Works perfectly with my tests. Thank you both

@lcsmuller lcsmuller merged commit 59edc64 into dev Mar 21, 2024
2 checks passed
@lcsmuller lcsmuller deleted the fix/curl-deprecated-warning branch March 21, 2024 12:55
@ThePedroo ThePedroo restored the fix/curl-deprecated-warning branch March 23, 2024 20:56
@ThePedroo
Copy link
Contributor Author

I'll have to open another PR as even though this works, it weirdly consumes gigantic amounts of CPU

@ThePedroo
Copy link
Contributor Author

Just an update: As this is weird, and seemingly there's no useful information out in the web, I created a discussion in curl repository

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

Successfully merging this pull request may close these issues.

3 participants