Revert "replace native implementation of curl_multi_await with a systemlib one"#8313
Closed
fredemmott wants to merge 1 commit into
Closed
Revert "replace native implementation of curl_multi_await with a systemlib one"#8313fredemmott wants to merge 1 commit into
fredemmott wants to merge 1 commit into
Conversation
…emlib one" fixes facebook#8156 refs facebook#7485 refs facebook#8043 The previous stability issues with the native impl were likely not actually an issue with async curl, but with async more generally with libevent2. facebook@93c31ec#diff-e9e6b8e9c00d40c3ec104ff79c2037a7 appears to have fixed the original stability issues (and the issues with stream_await()); it just looked like `curl_multi_await()` and `stream_await()` were buggy as: - FB rarely uses these functions (in particular, for our use case, async curl is generaly less performant than sync curl - but that depends on what other load you have, and how long requests take) - FB uses libevent1 - `curl_multi_await()` and `stream_await()` are among the most useful external features ... so, an HHVM issue when using libevent2 looks like a curl + stream async issue Test Plan: ran test programs from previous curl issues Crashes or gets stuck (doesn't on master or with this diff): ```Hack <?hh async function get_curl_content(Set<string> $urls): Awaitable<Vector<string>> { $chs = Vector {}; foreach ($urls as $url) { $ch = curl_init($url); curl_setopt($ch, CURLOPT_RETURNTRANSFER, true); $chs[] = $ch; } $mh = curl_multi_init(); foreach ($chs as $ch) { curl_multi_add_handle($mh, $ch); } $active = -1; do { $ret = curl_multi_exec($mh, &$active); } while ($ret == CURLM_CALL_MULTI_PERFORM); while ($active && $ret == CURLM_OK) { $select = await curl_multi_await($mh); if ($select === -1) { // https://bugs.php.net/bug.php?id=61141 await \HH\Asio\usleep(100); } do { $ret = curl_multi_exec($mh, &$active); } while ($ret == CURLM_CALL_MULTI_PERFORM); } $content = Vector {}; foreach ($chs as $ch) { $str = (string) curl_multi_getcontent($ch); $content[] = substr($str, 0, 10); curl_multi_remove_handle($mh, $ch); } curl_multi_close($mh); return $content; } function run(): void { $urls = Set {}; for($i=1;$i<10;$i++) $urls->add('https://hhvm.com/?test='.$i); $content = \HH\Asio\join(get_curl_content($urls)); var_dump($content); } while (true) { run(); } ``` Spins 100% CPU (does on master, doesn't with this diff): ```Hack <?hh $ch = curl_init(); curl_setopt($ch, CURLOPT_URL, "https://httpbin.org/delay/10"); // this line takes 100% cpu for 10 seconds \HH\Asio\join(\HH\Asio\curl_exec($ch)); // this line *does not* takes 100% cpu for 10 seconds // curl_exec($ch); curl_close($ch); ```
hhvm-bot
reviewed
Sep 4, 2018
hhvm-bot
left a comment
Contributor
There was a problem hiding this comment.
fredemmott has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fixes #8156
refs #7485
refs #8043
The previous stability issues with the native impl were likely not
actually an issue with async curl, but with async more generally with
libevent2.
93c31ec appears to have fixed the original stability issues (and the issues with
stream_await()); it just looked like
curl_multi_await()andstream_await()were buggy as:curl is generaly less performant than sync curl - but that depends on
what other load you have, and how long requests take)
curl_multi_await()andstream_await()are among the most usefulexternal features
... so, an HHVM issue when using libevent2 looks like a curl + stream
async issue
Test Plan: ran test programs from previous curl issues
Crashes or gets stuck (doesn't on master or with this diff):
Spins 100% CPU (does on master, doesn't with this diff):