Skip to content

Fix 76480: Use curl_multi_wait() so that timeouts are respected #3509

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

Closed
wants to merge 3 commits into from

Conversation

adoy
Copy link
Member

@adoy adoy commented Sep 9, 2018

Bug #76480

libcurl 7.28 introduced curl_multi_wait(), which is a select() wrapper that respects configured timeouts.

This patch will change the implementation of php curl_multi_select to use curl_multi_wait instead of curl_multi_fdset and select. It will fix #76480 but also unliked behaviour like #63411.

Unless anyone have any objection, I plan to commit it on PHP-7.1 since there should not be any BC.

@adoy
Copy link
Member Author

adoy commented Sep 9, 2018

FYI, HPHP is already using curl_multi_wait when available as a replacement for curl_multi_fdset + select

@staabm
Copy link
Contributor

staabm commented Sep 11, 2018

I guess this should be covered by a unit test

@php-pulls
Copy link

Comment on behalf of carusogabriel at php.net:

Labelling

@php-pulls php-pulls added the Bug label Sep 16, 2018
@adoy adoy closed this Sep 18, 2018
@adoy adoy deleted the bug-76480 branch September 18, 2018 01:53
@adoy
Copy link
Member Author

adoy commented Sep 18, 2018

Merged

@xPaw
Copy link
Contributor

xPaw commented Oct 17, 2018

Looks like this PR changed return values of curl_multi_select so upgrading to 7.2.11 makes my code run into infinite loops sometimes.

Code I'm running is here: https://github.com/xPaw/Crimp.php/blob/master/Crimp.php

However upon further inspection, I'm not entirely sure why it's doing that.

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

Successfully merging this pull request may close these issues.

4 participants