-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[HttpClient] Fix Undefined array key connection
#59046
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
[HttpClient] Fix Undefined array key connection
#59046
Conversation
&& -1.0 === curl_getinfo($ch, \CURLINFO_CONTENT_LENGTH_DOWNLOAD) | ||
&& \array_key_exists('connection', $responses[$id]->headers) | ||
&& \in_array('close', array_map('strtolower', $responses[$id]->headers['connection']), true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so, the question to me is: do we need these lines at all? maybe ignoring the SSL error is all we should do ("errno 0" is a hint this is not an issue I guess)
ideally, one of you would give it a try and report back the results from live usage.
Up to it?
Sorry for the delay, I had placed some debug code into production before this block of code, that would log when SSL_ERROR_SYSCALL was seen again. Below is the traced request debug - redacted - for a failed request. note that the request goes through our internal proxy, and example.com has a valid SSL installed. The reason for the SSL_ERROR_SYSCALL is not determined, and happens intermittently for different sites, I suspect the underlying issue here might be i/o locally, but thats only a guess, its not important (for Symfony) what is causing it, only that Symfony httpclient handles it. As you can see there are no response headers at all, as soon as Dumping of
I think @nicolas-grekas is right, by the time you have got a |
Surely if curl gets a |
I'm not sure what you mean by silencing as the response is there, at least for me, I get a proper HTTP response status and headers. My issue that I wanted to solve was, that I have NO way to determine that there was a problem in my App code due to the fact that it simply threw a plain See stack trace of reverted handling:
The log journal simply implements This then leads to throwing an SSL Error from reading the body of an response, that has proper 4xx response code and headers. So maybe for you the result is all the same for either because your proxy throws away all the response - for me however, the response is already there but has not been read from the stream yet.
|
I agree that either solution (removing the check for the header or checking if the key exists) will solve the issue my PR brought in - I'm in no way disputing that the array key issue must get adressed, I just don't want to reintroduce the bug. I want to add that the default of the connection header (meaning: the value to assume when none has been given) has changed from HTTP 1.0 to HTTP 1.1 - Maybe we should rather check against a proper default instead of removing the connection check? && \in_array('close', array_map('strtolower', $responses[$id]->headers['connection'] ?? ['connection' => 'close']), true) In the end having a connection |
connection
Just to say I leave the country on vacation this week and not back until christmas eve, so if someone wants to take this over feel free :) |
Signed-off-by: Phil E. Taylor <phil@phil-taylor.com>
9454b7a
to
51c32ef
Compare
$multi->handlesActivity[$id][] = \in_array($result, [\CURLE_OK, \CURLE_TOO_MANY_REDIRECTS], true) | ||
|| '_0' === $waitFor | ||
|| curl_getinfo($ch, \CURLINFO_SIZE_DOWNLOAD) === curl_getinfo($ch, \CURLINFO_CONTENT_LENGTH_DOWNLOAD) | ||
|| ('C' === $waitFor[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this condition which means we need headers to be fully received before ignoring this SSL error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works fine in my originally affected app.
Thank you @PhilETaylor. |
Check for array key before attempting to access it
Code style for human reading