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

libcurl 7.88.0 - Connection failed maybe due to the refactor of detecting if a connection is dead. #10646

Closed
SendSonS opened this issue Mar 1, 2023 · 9 comments
Assignees

Comments

@SendSonS
Copy link

SendSonS commented Mar 1, 2023

With libcurl 7.88.0/7.88.1, we sometimes encountered this issue, that the sending request was 'Expire cleared', it tried to reuse connections but ' Connection died, tried 5 times before giving up'.

Refer to the attached logs:
logs_libcurl_7_87_0.txt
logs_libcurl_7_88_0.txt

There're 6 connections in the cache but they are all dead.
With 7.87.0, when handling a new request, it fount that the connection is already dead before sending data (Connection 2 seems to be dead), then it remove all dead connections.
With 7.88.0, when handling a new request, it did not remove any dead connection before sending data, but failed to send data (we are done reading and this is set to close, stop send), then it retry the next died connection (Connection died, retrying a fresh connect (retry count: 1), finally it failed after retried with 5 times.

We checked the history and found that, before create_conn it will prune_dead_connections, the detecting method was changed at here:
71b7e01#diff-a8a54563608f8155973318f4ddb61d7328dab512b8ff2b5cc48cc76979d4204c

-      dead = SocketIsDead(conn->sock[FIRSTSOCKET]);
+      dead = !Curl_conn_is_alive(data, conn);

Looks like it cannot find the connection that is already dead before sending data.

@icing icing self-assigned this Mar 1, 2023
@icing
Copy link
Contributor

icing commented Mar 1, 2023

@SendSonS what platform are you running on?

icing added a commit to icing/curl that referenced this issue Mar 1, 2023
- refs curl#10646 where reuse was attempted on closed connections in the
  cache, leading to an exhaustion of retries on a transfer
- the mistake was that poll events like POLLHUP, POLLERR, etc
  were regarded as "not dead".
- change cf-socket filter check to regard such events as inidication
  of corpsiness.
- vtls filter checks: fixed interpretation of backend check result
  when inconclusive to interrogate status further down the filter
  chain.
@icing
Copy link
Contributor

icing commented Mar 1, 2023

Thanks for the report and the logs.

Could your check that #10652 fixes the problem for you as well?

@bagder bagder closed this as completed in 9fd2d5a Mar 2, 2023
@SendSonS
Copy link
Author

SendSonS commented Mar 6, 2023

Thanks for the report and the logs.

Could your check that #10652 fixes the problem for you as well?

@icing We still see this issue with your patch.
The log is still as same as before, it re-tried to re-use 5 dead connections.

@icing icing reopened this Mar 6, 2023
@icing
Copy link
Contributor

icing commented Mar 6, 2023

I assume the requests involved use https:?

@icing
Copy link
Contributor

icing commented Mar 6, 2023

Please find in #10690 my latest fix for putting a lid not this issue.

Background: the refactoring from 7.87 to 7.88 failed to properly preserve the handling of pending input on connection reuse. For http connections, having input arriving from a server is considered bad, since the server should not send anything unless there is some issue.

For https: connections the most common case of pending input is a TLS notify close message, e.g. the server was done with it.

The PR restores the previous way of checking for this.

@icing
Copy link
Contributor

icing commented Mar 8, 2023

@SendSonS could you try the current master?

@SendSonS
Copy link
Author

@icing Thanks, we just try against #10690 , and we didn't see this issue again.
We think this issue is fixed now.

@icing icing closed this as completed Mar 10, 2023
@stanhu
Copy link
Contributor

stanhu commented Mar 14, 2023

Thanks! We're keen on updating curl to v7.88, but this issue makes us a little hesitant since we have a number of users who use proxy servers.

Will there be a curl 7.88.2 with this fix?

@bagder
Copy link
Member

bagder commented Mar 14, 2023

The next release is 8.0.0, due in 6 days.

bch pushed a commit to bch/curl that referenced this issue Jul 19, 2023
- refs curl#10646 where reuse was attempted on closed connections in the
  cache, leading to an exhaustion of retries on a transfer
- the mistake was that poll events like POLLHUP, POLLERR, etc
  were regarded as "not dead".
- change cf-socket filter check to regard such events as inidication
  of corpsiness.
- vtls filter checks: fixed interpretation of backend check result
  when inconclusive to interrogate status further down the filter
  chain.

Reported-by: SendSonS on github
Fixes curl#10646
Closes curl#10652
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

4 participants