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

Use select() syscall to avoid busy waiting #110

Closed
wants to merge 1 commit into from
Closed

Use select() syscall to avoid busy waiting #110

wants to merge 1 commit into from

Conversation

diegode
Copy link

@diegode diegode commented Apr 2, 2014

Currently it consumes a lot of CPU, it's a no-brainer i think.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.09%) when pulling 8783764 on diegode:curl_multi_select into 81a917f on rmccue:master.

@rmccue
Copy link
Collaborator

rmccue commented Apr 20, 2014

Hmm, interesting. Can you explain exactly what this does please? :)

@diegode
Copy link
Author

diegode commented Apr 20, 2014

Well, busy-waiting is a technique in which a process repeatedly checks to see if a condition is true. In this case, the condition is "has any of the requests been answered?". This is in general a wasteful practice, so the OSes provide an array of system calls to avoid it.

For example, to wait 10 seconds you won't do:

$before = time();
while ($before+10 > time());

But instead:

sleep(10);

For this case, select() is a traditional solution, and cURL provides a wrapper through curl_multi_select(). It just blocks until one or more of the requests has I/O available, and you can set a timeout (1 second by default).

@rmccue
Copy link
Collaborator

rmccue commented Apr 21, 2014

For this case, select() is a traditional solution, and cURL provides a wrapper through curl_multi_select(). It just blocks until one or more of the requests has I/O available, and you can set a timeout (1 second by default).

Right, I understand how select works, but I thought curl_multi_exec automatically selected. Reading the documentation again, looks like I was wrong. :)

Thanks for this! I'll check it out and merge.

@staabm
Copy link
Contributor

staabm commented May 13, 2014

@rmccue any progress on this topic?

@rmccue
Copy link
Collaborator

rmccue commented May 14, 2014

Sorry, I've been pretty busy and didn't get around to trying this out yet. Will do.

@diegode
Copy link
Author

diegode commented Oct 21, 2014

ping @rmccue

@diegode
Copy link
Author

diegode commented Oct 21, 2014

Maybe we should add some checks as in http://stackoverflow.com/a/15173929

TysonAndre added a commit to TysonAndre/Requests that referenced this pull request Jun 14, 2017
This may be the same as WordPress#110

This will require more testing across various libcurl versions.
I doubt that the timeout is necessary for curl_multi_select
(calls select() if it can),
but leaving in one just in case of bugs, so that it will end.
- Haven't thoroughly checked for relevant libcurl bugs.

Asynchronously wait for events with a short timeout if CURLM_CALL_MULTI_PERFORM
fails.
@TysonAndre
Copy link
Contributor

I have a branch which independently has the exact same approach (to the exact same problem) in a slightly different location. See #284

https://secure.php.net/manual/en/function.curl-multi-select.php#refsect1-function.curl-multi-select-returnvalues

On failure, this function will return -1 on a select failure (from the underlying select system call).

We probably want to usleep() in the rare case (OS/PHP/curl setup?) that curl_multi_select returns -1, but this is something I'm in favor of.

@schlessera schlessera closed this Jun 18, 2021
@schlessera schlessera deleted the branch WordPress:master June 18, 2021 06:19
@schlessera
Copy link
Member

This PR was closed inadvertently when we changed the stable branch from master to stable. GitHub was supposed to smoothly adapt the PRs to point to the new branch, but this seems to have failed for the cases where the author's branch/fork of the PR is not available anymore.

@jrfnl
Copy link
Member

jrfnl commented Jun 18, 2021

PR recreated as #493

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.

None yet

7 participants