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(common): better defaults for curl initialization #9798

Conversation

coryan
Copy link
Member

@coryan coryan commented Sep 8, 2022

By default, initialize libcurl with locking and signal handlers that work for most applications. In particular, leaving the default handler for SIGPIPE usually results in the application crashes when a socket is closed.


This change is Reviewable

By default, initialize libcurl with locking and signal handlers that
work for most applications. In particular, leaving the default handler
for `SIGPIPE` usually results in the application crashes when a socket
is closed.
@google-cloud-cpp-bot
Copy link
Collaborator

Google Cloud Build Logs
For commit: a87ae78b033da6873e6182285254989e0a8dfb3f

ℹ️ NOTE: Kokoro logs are linked from "Details" below.

Copy link
Member

@dbolduc dbolduc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have an Options CurlDefaultOptions(Options), where we can default these values to true (and test that the defaults are correct)?

Maybe we call it here:

options_(std::move(options)) {

@codecov
Copy link

codecov bot commented Sep 8, 2022

Codecov Report

Merging #9798 (678e839) into main (171f936) will increase coverage by 0.00%.
The diff coverage is 94.11%.

@@           Coverage Diff           @@
##             main    #9798   +/-   ##
=======================================
  Coverage   94.22%   94.22%           
=======================================
  Files        1495     1495           
  Lines      139801   139816   +15     
=======================================
+ Hits       131725   131744   +19     
+ Misses       8076     8072    -4     
Impacted Files Coverage Δ
google/cloud/internal/curl_wrappers.cc 77.90% <83.33%> (-0.15%) ⬇️
google/cloud/internal/curl_wrappers_test.cc 100.00% <100.00%> (ø)
...e/cloud/pubsublite/internal/alarm_registry_impl.cc 97.05% <0.00%> (-2.95%) ⬇️
...le/cloud/internal/default_completion_queue_impl.cc 96.59% <0.00%> (-0.57%) ⬇️
...cloud/pubsub/internal/subscription_session_test.cc 97.98% <0.00%> (-0.17%) ⬇️
google/cloud/completion_queue_test.cc 97.32% <0.00%> (+0.19%) ⬆️
...bigtable/examples/bigtable_hello_instance_admin.cc 83.00% <0.00%> (+2.00%) ⬆️
...te/integration_tests/publisher_integration_test.cc 98.68% <0.00%> (+6.57%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@coryan coryan marked this pull request as ready for review September 8, 2022 23:59
@coryan coryan requested a review from a team as a code owner September 8, 2022 23:59
@google-cloud-cpp-bot
Copy link
Collaborator

Google Cloud Build Logs
For commit: 678e8394a5cab519564d8b4f9f9356f1aaffa9a2

ℹ️ NOTE: Kokoro logs are linked from "Details" below.

Copy link
Member

@dbolduc dbolduc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@coryan coryan enabled auto-merge (squash) September 9, 2022 00:43
@coryan coryan merged commit 7c4b8ca into googleapis:main Sep 9, 2022
@coryan coryan deleted the fix-common-better-defaults-for-curl-initialization branch September 9, 2022 12:28
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.

None yet

3 participants