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

impl(rest): set ReadFunctionAbort via RAII #14617

Merged
merged 3 commits into from
Aug 7, 2024

Conversation

scotthart
Copy link
Member

@scotthart scotthart commented Aug 6, 2024

Leverage RAII to ensure the CURLOPT_READFUNCTION is set to ReadFunctionAbort regardless if MakeRequestImpl returns normally or if an exception is thrown.


This change is Reviewable

Copy link

codecov bot commented Aug 6, 2024

Codecov Report

Attention: Patch coverage is 91.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 93.59%. Comparing base (9ffcb83) to head (dfb996a).
Report is 1 commits behind head on main.

Files Patch % Lines
google/cloud/internal/curl_impl.cc 91.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #14617      +/-   ##
==========================================
- Coverage   93.59%   93.59%   -0.01%     
==========================================
  Files        2316     2316              
  Lines      207118   207122       +4     
==========================================
- Hits       193853   193851       -2     
- Misses      13265    13271       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

~ReadFunctionAbortGuard() {
// If curl_closed_ is true, then the handle has already been recycled and
// attempting to set an option on it will error.
if (impl_.curl_closed_) {
Copy link
Contributor

Choose a reason for hiding this comment

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

!impl_.curl_closed_?

Copy link
Member Author

@scotthart scotthart left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @devbww)


google/cloud/internal/curl_impl.cc line 496 at r1 (raw file):

Previously, devbww (Bradley White) wrote…

!impl_.curl_closed_?

Yes, that would be much better.

// an error, typically a timeout, has occurred. Use ReadFunctionAbortGuard to
// instruct curl to not attempt to send anymore data on this handle regardless
// if an error or exception is encountered.
{
Copy link
Member

Choose a reason for hiding this comment

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

nit: the scope isn't doing anything. we can remove it.

Copy link
Member Author

@scotthart scotthart left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @dbolduc and @devbww)


google/cloud/internal/curl_impl.cc line 532 at r2 (raw file):

Previously, dbolduc (Darren Bolduc) wrote…

nit: the scope isn't doing anything. we can remove it.

It's there for future readers of the code. If it was a std::lock_guard which most folks are familiar with, I would not introduce the extra scope block. ReadFunctionAbortGuard is unfamiliar and I want to make sure that future me takes into account it's purpose when potentially modifying this function.

@dbolduc
Copy link
Member

dbolduc commented Aug 7, 2024

It's there for future readers of the code.

I think future readers of the code will wonder why it is there, like I just did.

@scotthart scotthart enabled auto-merge (squash) August 7, 2024 20:43
@scotthart scotthart merged commit 7eff3dd into googleapis:main Aug 7, 2024
69 of 70 checks passed
cuiy0006 pushed a commit to cuiy0006/google-cloud-cpp that referenced this pull request Aug 16, 2024
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