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): returnable argument to .then() #13316

Merged

Conversation

coryan
Copy link
Contributor

@coryan coryan commented Dec 15, 2023

The main motivation for these changes is to support:

future<T> F(future<T> f) {
  return f.then([](auto g) { /* stuff */; return g; });
}

This is common enough, and was broken for fairly obscure reasons. I completely changed the implementation of .then(). It was too hacky to do something else, and I think the new implementation is simpler too.

Fixes #13314 and fixes #13258


This change is Reviewable

Copy link

codecov bot commented Dec 15, 2023

Codecov Report

Attention: 21 lines in your changes are missing coverage. Please review.

Comparison is base (66db303) 93.05% compared to head (eb4fa2e) 93.06%.
Report is 4 commits behind head on main.

Files Patch % Lines
google/cloud/internal/future_then_impl_test.cc 95.02% 10 Missing ⚠️
google/cloud/internal/future_then_impl.h 93.61% 6 Missing ⚠️
google/cloud/internal/future_impl.h 82.75% 5 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main   #13316    +/-   ##
========================================
  Coverage   93.05%   93.06%            
========================================
  Files        2140     2142     +2     
  Lines      186610   186838   +228     
========================================
+ Hits       173657   173873   +216     
- Misses      12953    12965    +12     

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

@coryan
Copy link
Contributor Author

coryan commented Dec 15, 2023

The check-api results are false positives.

  1. Evidently we no longer create an instantiation of future<future<future<void>>>::get(), we do not need to, but if anybody creates an instance it will work.
Step #3: Removed Symbols  1 
Step #3: 
Step #3: ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Step #3: future_generic.h
Step #3: namespace google::cloud::v2_20
Step #3: future<future<future<void> > >::get ( )
Step #3: _ZN6google5cloud5v2_206futureINS2_INS2_IvEEEEE3getEv
  1. This is an absl::typedef that went away:
Step #3: Problems with Data Types, Low Severity  1 
Step #3: 
Step #3: ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Step #3: type_traits.h
Step #3: namespace absl
Step #3: [+] typedef remove_reference_t  1 

@coryan
Copy link
Contributor Author

coryan commented Dec 15, 2023

The warnings on Debian:bookworm (GCC 12 FWIW) look like false positives:

/usr/include/c++/12/bits/exception_ptr.h:117:33: error: '*(std::__exception_ptr::exception_ptr*)((char*)&<unnamed> + offsetof(absl::debian3::variant<absl::debian3::monostate, std::__exception_ptr::exception_ptr, google::cloud::v2_20::internal::FutureVoid, google::cloud::v2_20::internal::FutureValueRetrieved>,absl::debian3::variant<absl::debian3::monostate, std::__exception_ptr::exception_ptr, google::cloud::v2_20::internal::FutureVoid, google::cloud::v2_20::internal::FutureValueRetrieved>::<unnamed>.absl::debian3::variant_internal::VariantCopyAssignBaseNontrivial<absl::debian3::monostate, std::__exception_ptr::exception_ptr, google::cloud::v2_20::internal::FutureVoid, google::cloud::v2_20::internal::FutureValueRetrieved>::<unnamed>.absl::debian3::variant_internal::VariantMoveAssignBaseNontrivial<absl::debian3::monostate, std::__exception_ptr::exception_ptr, google::cloud::v2_20::internal::FutureVoid, google::cloud::v2_20::internal::FutureValueRetrieved>::<unnamed>.absl::debian3::variant_internal::VariantCopyBaseNontrivial<absl::debian3::monostate, std::__exception_ptr::exception_ptr, google::cloud::v2_20::internal::FutureVoid, google::cloud::v2_20::internal::FutureValueRetrieved>::<unnamed>.absl::debian3::variant_internal::VariantMoveBaseNontrivial<absl::debian3::monostate, std::__exception_ptr::exception_ptr, google::cloud::v2_20::internal::FutureVoid, google::cloud::v2_20::internal::FutureValueRetrieved>::<unnamed>.absl::debian3::variant_internal::VariantStateBaseDestructorNontrivial<absl::debian3::monostate, std::__exception_ptr::exception_ptr, google::cloud::v2_20::internal::FutureVoid, google::cloud::v2_20::internal::FutureValueRetrieved>::<unnamed>.absl::debian3::variant_internal::VariantStateBase<absl::debian3::monostate, std::__exception_ptr::exception_ptr, google::cloud::v2_20::internal::FutureVoid, google::cloud::v2_20::internal::FutureValueRetrieved>::state_)).std::__exception_ptr::exception_ptr::_M_exception_object' may be used uninitialized [-Werror=maybe-uninitialized]
  117 |       : _M_exception_object(__o._M_exception_object)
      |                             ~~~~^~~~~~~~~~~~~~~~~~~
/workspace/google/cloud/internal/future_impl.h: In member function 'void google::cloud::v2_20::internal::AsyncConnectionReadyFuture::RunIteration()':
/workspace/google/cloud/internal/future_impl.h:176:65: note: '<anonymous>' declared here
  176 |   future_shared_state() : future_shared_state([] {}, ValueType{}) {}
      |                                                                 ^
cc1plus: all warnings being treated as errors

First, it is a maybe-unitialized. Second, it is in absl::variant<> (or std::exception_ptr) we have no control over either and are supposed to initialize properly. Third, we use the version of Abseil included in this distro, which is fairly old.

I am going to suppress the warning using #pragma.

The main motivation for these changes is to support:

```cc
future<T> F(future<T> f) {
  return f.then([](auto g) { /* stuff */; return g; });
}
```

This is common enough, and was broken for fairly obscure reasons.  I
completely changed the implementation of `.then()`. It was too hacky to
do something else, and I think the new implementation is simpler too.
@coryan coryan force-pushed the fix-common-future-continuation-returns-argument branch from b4833cf to bad023c Compare December 15, 2023 20:17
@coryan coryan marked this pull request as ready for review December 15, 2023 20:31
@coryan coryan requested a review from a team as a code owner December 15, 2023 20:31
google/cloud/internal/future_impl.h Outdated Show resolved Hide resolved
google/cloud/internal/future_impl.h Outdated Show resolved Hide resolved
google/cloud/internal/future_impl.h Outdated Show resolved Hide resolved
@coryan coryan enabled auto-merge (squash) December 18, 2023 18:18
@coryan coryan merged commit 11fd85c into googleapis:main Dec 18, 2023
59 checks passed
@coryan coryan deleted the fix-common-future-continuation-returns-argument branch December 18, 2023 20:12
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.

Propagate abandoned future errors via future<T>::then() future<T>::then() cannot return the provided future
2 participants