Skip to content

[HttpClient] Double check if handle is complete #44479

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

Merged
merged 1 commit into from
Dec 11, 2021

Conversation

Nyholm
Copy link
Member

@Nyholm Nyholm commented Dec 6, 2021

Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets
License MIT
Doc PR

This is a forward compatibility fix. We did the same in Guzzle after a comment from bagder.

guzzle/guzzle#2892 (comment)

Basically, if libcurl decides to add an other value for msg, then our code will break without this PR. The only value for msg with current latest version of curl is CURLMSG_DONE which means that this is not a bugfix yet.. but it make sure that we respect the libcurl API.

@divinity76
Copy link
Contributor

+1 from me, btw wonder if a E_USER_WARNING about an unknown-ignored message would be appropriate (probably means you're running an old version of symfony)

another thing, the code above seems faulty, the loop above should be changed from

while (\CURLM_CALL_MULTI_PERFORM === curl_multi_exec($multi->handle, $active));

to something like

do{
    $err = curl_multi_exec($multi->handle, $active);
} while($err === \CURLM_CALL_MULTI_PERFORM);
if($err !== CURLM_OK){
 // handle error somehow
}

while exactly how to handle the error may be up for debate, just ignoring the error (as the current code does) is not the right approach. i've done it like

            do {
                $err = curl_multi_exec($mh, $still_running);
            } while ($err === CURLM_CALL_MULTI_PERFORM);
            if ($err !== CURLM_OK) {
                $errinfo = [
                    "multi_exec_return" => $err,
                    "curl_multi_errno" => curl_multi_errno($mh),
                    "curl_multi_strerror" => curl_multi_strerror($err)
                ];
                $errstr = "curl_multi_exec error: " . var_export($errinfo, true);
                throw new \RuntimeException($errstr);
            }

in the past, but didn't really give it much thought as to how it should really be handled. possible return values include

CURLM_CALL_MULTI_PERFORM
CURLM_OK
CURLM_BAD_HANDLE
CURLM_BAD_EASY_HANDLE
CURLM_OUT_OF_MEMORY
CURLM_INTERNAL_ERROR

@Nyholm
Copy link
Member Author

Nyholm commented Dec 7, 2021

btw wonder if a E_USER_WARNING about an unknown-ignored message would be appropriate (probably means you're running an old version of symfony)

Im not sure what you mean. Are you suggesting we should trigger a E_USER_WARNING if libcurl adds another value for $info['msg']?


Your other comments seams interesting. Feel free to open an new PR or issue about those. They are a bit unrelated to this PR.

@divinity76
Copy link
Contributor

Are you suggesting we should trigger a E_USER_WARNING if libcurl adds another value for $info['msg']?

maybe, after all, it means that this version Symfony received a message from libcurl that this version of Symfony doesn't understand, and because the message wasn't understood, Symfony opted to completely ignore the message. does that warrant a warning? i don't know tbh. i don't have strong feelings on it either way, just ignoring unknown messages is probably fine.

Your other comments seams interesting. Feel free to open an new PR or issue about those. They are a bit unrelated to this PR.

yeah maybe i'll do that ^^

@nicolas-grekas
Copy link
Member

Thank you @Nyholm.

@nicolas-grekas nicolas-grekas merged commit 513fc4c into symfony:4.4 Dec 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants