Skip to content

Conversation

@dscho
Copy link
Member

@dscho dscho commented Sep 29, 2025

In cURL v8.16.0, the behavior changed subtly: Timeouts were added even in circumstances where cURL failed to connect and should have errored out with the corresponding CURLE_COULDNT_CONNECT instead.

This has been identified in microsoft/git via test failures in t5799:

 ++ test_when_finished per_test_cleanup
  ++ test 0 = 0
  ++ test_cleanup='{ per_test_cleanup
  		} && (exit "$eval_ret"); eval_ret=$?; :'
  ++ test_must_fail git -C 'D:/a/git/git/t/trash directory.t5799-gvfs-helper/repo_t1' gvfs-helper --cache-server=disable --remote=origin get --max-retries=2
  ++ case "$1" in
  ++ _test_ok=
  ++ test_must_fail_acceptable git -C 'D:/a/git/git/t/trash directory.t5799-gvfs-helper/repo_t1' gvfs-helper --cache-server=disable --remote=origin get --max-retries=2
  ++ test git = env
  ++ test git = nongit
  ++ case "$1" in
  ++ return 0
  ++ git -C 'D:/a/git/git/t/trash directory.t5799-gvfs-helper/repo_t1' gvfs-helper --cache-server=disable --remote=origin get --max-retries=2
  ++ exit_code=2
  ++ test 2 -eq 0
  ++ test_match_signal 13 2
  ++ test 2 = 141
  ++ test 2 = 269
  ++ return 1
  ++ test 2 -gt 129
  ++ test 2 -eq 127
  ++ test 2 -eq 126
  ++ return 0
  ++ grep -q 'error: get: (curl:7)' OUT.stderr
  error: last command exited with $?=1
  ++ per_test_cleanup
  ++ stop_gvfs_protocol_server
  ++ test -f 'D:/a/git/git/t/trash directory.t5799-gvfs-helper/pid-file.pid'
  ++ return 0
  ++ rm -rf 'D:/a/git/git/t/trash directory.t5799-gvfs-helper/shared_cache_t1/[0-9a-f][0-9a-f]/'
  ++ rm -rf 'D:/a/git/git/t/trash directory.t5799-gvfs-helper/shared_cache_t1/info/*'
  ++ rm -rf 'D:/a/git/git/t/trash directory.t5799-gvfs-helper/shared_cache_t1/pack/tempPacks'
  ++ rm -rf OUT.output OUT.stderr
  ++ return 0
  ++ exit 1
  ++ eval_ret=1
  ++ :
  not ok 13 - curl-error: no server

It is a bit difficult to diagnose the problem from Git's output, as the interesting information is suppressed: Instead of curl:7 (CURLE_COULDNT_CONNECT), it reports curl:28 (CURLE_OPERATION_TIMEDOUT). I reported this to the cURL project and they gracefully jumped on fixing it. This here PR back-ports the fix from curl/curl#18768.

@dscho
Copy link
Member Author

dscho commented Sep 29, 2025

/deploy curl

The x86_64 and the i686 workflow runs were started.

In cURL v8.16.0, the behavior changed subtly: Timeouts were added even
in circumstances where cURL failed to connect and should have errored
out with the corresponding `CURLE_COULDNT_CONNECT` instead.

This commit back-ports the fix from
curl/curl#18768.

Signed-off-by: Johannes Schindelin <[email protected]>
@dscho dscho force-pushed the happy-eyeballs-fix-for-curl branch from a63bbee to 4d8f15b Compare September 29, 2025 16:02
@dscho
Copy link
Member Author

dscho commented Sep 29, 2025

/deploy curl

The x86_64 and the i686 workflow runs were started.

@dscho dscho merged commit 00c84af into main Sep 29, 2025
5 checks passed
@dscho dscho deleted the happy-eyeballs-fix-for-curl branch September 29, 2025 21:35
dscho added a commit to microsoft/git that referenced this pull request Sep 30, 2025
I investigated a couple of new test failures over in #799; Essentially,
t5799.13(curl-connect: no server) no longer received the expected
`CURLE_COULDNT_CONNECT` immediately, but instead received
`CURLE_OPERATION_TIMEDOUT` after 5 minutes (!), and due to
internal details that problem was repeated 4 times (!!!).

Turns out that the regression was _not_ introduced by the patches in
that PR, but by the independent Git for Windows SDK update of cURL
to v8.16.0.

I integrated a fix into the Git for Windows SDK in
git-for-windows/MINGW-packages#163, but
since Microsoft Git also targets Linux and macOS (where we do not
control the cURL version), here is a work-around.

This PR also adds some clean-ups for the `t5799-gvfs-helper.sh` test
script in general, e.g. to improve future debug'ability.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants