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

Disable signals in libcurl. #2226

Closed
wants to merge 5 commits into from

Conversation

coryan
Copy link
Member

@coryan coryan commented Mar 13, 2019

TIL: libcurl uses signals to perform timeouts by default. This change
disables the signals for all the handles we create, per the
documentation on:

https://curl.haxx.se/libcurl/c/CURLOPT_NOSIGNAL.html

I will try to write a regression test later, though it might be hard
because it requires "waiting for a couple of minutes" to get the
signal.

In any case, given the documentation, and the fact that there is a bug
report (#2225) associated with this, I believe we should incorporate
these fixes.


This change is Reviewable

TIL: libcurl uses signals to perform timeouts by default. This change
disables the signals for all the handles we create, per the
documentation on:

https://curl.haxx.se/libcurl/c/CURLOPT_NOSIGNAL.html

I will try to write a regression test later, though it might be hard
because it requires "waiting for a couple of minutes" to get the
signal.

In any case, given the documentation, and the fact that there is a bug
report (googleapis#2225) associated with this, I believe we should incorporate
these fixes.
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 13, 2019
@coryan
Copy link
Member Author

coryan commented Mar 14, 2019

@nickolasrossi if you have a few minutes to spare: I added an integration test to this PR that I hoped would reproduce the problem. Alas! It does not. Any suggestions on what to change would be great.

@codecov
Copy link

codecov bot commented Mar 14, 2019

Codecov Report

Merging #2226 into master will increase coverage by 0.49%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2226      +/-   ##
==========================================
+ Coverage   91.99%   92.49%   +0.49%     
==========================================
  Files         307      307              
  Lines       18402    18430      +28     
==========================================
+ Hits        16929    17046     +117     
+ Misses       1473     1384      -89
Impacted Files Coverage Δ
...le/cloud/storage/internal/curl_download_request.cc 75.18% <100%> (+0.37%) ⬆️
...ogle/cloud/storage/internal/curl_upload_request.cc 82.6% <100%> (+0.25%) ⬆️
google/cloud/storage/internal/curl_request.cc 96.77% <100%> (+0.22%) ⬆️
...le/cloud/bigtable/internal/completion_queue_impl.h 96.07% <0%> (-1.57%) ⬇️
google/cloud/bigtable/client_options.h 100% <0%> (ø) ⬆️
google/cloud/bigtable/completion_queue.h 100% <0%> (ø) ⬆️
google/cloud/examples/gcs2cbt.cc 89.6% <0%> (+72.8%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dd9444b...4fbb751. Read the comment docs.

@nickolasrossi
Copy link

Yeah, it’s tough to repro an intermittent problem... since it comes from a DNS timeout, I think you want to download from a host name that causes DNS to take a while, and set a small CURLOPT_TIMEOUT to make it time out faster.

@ghost
Copy link

ghost commented Mar 14, 2019

Yeah, it’s tough to repro an intermittent problem... since it comes from a DNS timeout, I think you want to download from a host name that causes DNS to take a while, and set a small CURLOPT_TIMEOUT to make it time out faster.

That's a good suggestion. What do you think about something similar to this?

https://github.com/google/google-api-cpp-client/blob/d0bbe169d81a50936ec5fcea4e6dbcfb97303f13/src/googleapis/client/transport/curl_http_transport.cc#L241-L254

@coryan
Copy link
Member Author

coryan commented Mar 18, 2019

I created a clean PR with just the fixes, closing this for now. If/When I can create a regression test I will submit a separate PR.

@coryan coryan closed this Mar 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants