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

fix: workaround curl_multi_poll returning an error on EINTR #11649

Merged
merged 2 commits into from
May 19, 2023

Conversation

pitrou
Copy link
Contributor

@pitrou pitrou commented May 18, 2023

Fixes #11647.


This change is Reviewable

@pitrou
Copy link
Contributor Author

pitrou commented May 18, 2023

For now this implements a workaround in CurlImpl::WaitForHandles. Once we agree on that, I'll have to port the workaround to CurlDownloadRequest::WaitForHandles (or factor out the logic into a shared helper routine).

@coryan

@pitrou
Copy link
Contributor Author

pitrou commented May 18, 2023

I'll note that I can't test whether this actually fixes the underlying issue. At least it doesn't seem to break the test suite on my machine.

@pitrou
Copy link
Contributor Author

pitrou commented May 18, 2023

By the way, this fix assumes we want to retry on EINTR, which seems to make sense but is not the case currently.
In any case, it seems PerformWorkUntil loops if WaitForHandles returns with success. Thoughts?

@coryan
Copy link
Contributor

coryan commented May 18, 2023

Thanks very much for the PR.

For now this implements a workaround in CurlImpl::WaitForHandles. Once we agree on that, I'll have to port the workaround to CurlDownloadRequest::WaitForHandles (or factor out the logic into a shared helper routine).

In all likelihood we will remove that code in a couple of months. It is there just as an emergency hatch in case the new code does not work for some people, and so far it looks like it is not needed.

The code looks good to me, though it is a bit sad to have so many #if branches.

Let's run the build so see if the robots have something to say.

@coryan
Copy link
Contributor

coryan commented May 18, 2023

/gcbrun

@codecov
Copy link

codecov bot commented May 18, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -2.45 ⚠️

Comparison is base (f2eaddb) 93.77% compared to head (63b8702) 91.32%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11649      +/-   ##
==========================================
- Coverage   93.77%   91.32%   -2.45%     
==========================================
  Files        1824     1404     -420     
  Lines      164431   108689   -55742     
==========================================
- Hits       154190    99261   -54929     
+ Misses      10241     9428     -813     
Impacted Files Coverage Δ
google/cloud/internal/curl_impl.cc 90.62% <100.00%> (-0.25%) ⬇️

... and 509 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@coryan
Copy link
Contributor

coryan commented May 18, 2023

The failure with checkers-pr is just formatting, you can see what the robots want at the end of:

https://github.com/googleapis/google-cloud-cpp/pull/11649/checks?check_run_id=13593517862

@coryan coryan added the kokoro:run Add this label to force Kokoro to re-run the tests. label May 18, 2023
@google-cloud-cpp-bot google-cloud-cpp-bot removed the kokoro:run Add this label to force Kokoro to re-run the tests. label May 18, 2023
@coryan
Copy link
Contributor

coryan commented May 19, 2023

/gcbrun

@coryan coryan marked this pull request as ready for review May 19, 2023 13:32
@coryan coryan requested a review from a team as a code owner May 19, 2023 13:32
@coryan coryan added the kokoro:run Add this label to force Kokoro to re-run the tests. label May 19, 2023
@google-cloud-cpp-bot google-cloud-cpp-bot removed the kokoro:run Add this label to force Kokoro to re-run the tests. label May 19, 2023
@coryan
Copy link
Contributor

coryan commented May 19, 2023

Sorry about the guessing game for formatting, I should have pointed you to https://github.com/googleapis/google-cloud-cpp/blob/main/doc/contributor/README.md#formatting

It looks good now. I will approve and merge once the builds pass.

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.

curl_multi_wait might erroneously return CURLM_UNRECOVERABLE_POLL
3 participants