Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
nicolas-grekas merged 1 commit intosymfony:4.4fromNyholm:4.4-http-multi-curl
Dec 11, 2021

Conversation

@Nyholm
Copy link
Member

@NyholmNyholm commentedDec 6, 2021
edited
Loading

QA
Branch?4.4
Bug fix?yes
New feature?no
Deprecations?no
Tickets
LicenseMIT
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 formsg, then our code will break without this PR. The only value formsg with current latest version of curl isCURLMSG_DONE which means that this is not a bugfix yet.. but it make sure that we respect the libcurl API.

divinity76 reacted with thumbs up emoji
@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);thrownew \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_PERFORMCURLM_OKCURLM_BAD_HANDLECURLM_BAD_EASY_HANDLECURLM_OUT_OF_MEMORYCURLM_INTERNAL_ERROR

@Nyholm
Copy link
MemberAuthor

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 aE_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-grekasnicolas-grekas merged commit513fc4c intosymfony:4.4Dec 11, 2021
This was referencedDec 29, 2021
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Projects

None yet

Milestone

4.4

Development

Successfully merging this pull request may close these issues.

4 participants

@Nyholm@divinity76@nicolas-grekas@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp