refactor grpc: get rid of useless FutureImpl

commit_hash:f6d0bb7531c532aeb5030efc06fd9605d26c4aea

Author: kpavlov00

grpc/include/userver/ugrpc/client/impl/async_methods.hpp

@@ -163,23 +163,6 @@ class RpcData final {
     grpc::Status status_;
 };
 
-class FutureImpl final {
-public:
-    explicit FutureImpl(RpcData& data) noexcept;
-
-    ~FutureImpl() noexcept = default;
-
-    FutureImpl(FutureImpl&&) noexcept;
-    FutureImpl& operator=(FutureImpl&&) noexcept;
-
-    RpcData* GetData() const noexcept;
-
-    void ClearData() noexcept;
-
-private:
-    RpcData* data_;
-};
-
 void CheckOk(RpcData& data, AsyncMethodInvocation::WaitStatus status, std::string_view stage);
 
 template <typename GrpcStream>

grpc/include/userver/ugrpc/client/rpc.hpp

@@ -52,12 +52,12 @@ class [[nodiscard]] UnaryFuture {
     ) noexcept;
     /// @endcond
 
-    UnaryFuture(UnaryFuture&&) noexcept = default;
+    UnaryFuture(UnaryFuture&&) noexcept;
     UnaryFuture& operator=(UnaryFuture&&) noexcept;
     UnaryFuture(const UnaryFuture&) = delete;
     UnaryFuture& operator=(const UnaryFuture&) = delete;
 
-    ~UnaryFuture() noexcept;
+    ~UnaryFuture();
 
     /// @brief Await response
     ///
@@ -92,7 +92,7 @@ class [[nodiscard]] UnaryFuture {
     /// @endcond
 
 private:
-    impl::FutureImpl impl_;
+    impl::RpcData* data_{};
     std::function<void(impl::RpcData& data, const grpc::Status& status)> post_finish_;
 };
 
@@ -109,10 +109,12 @@ class [[nodiscard]] StreamReadFuture {
     ) noexcept;
     /// @endcond
 
-    StreamReadFuture(StreamReadFuture&& other) noexcept = default;
+    StreamReadFuture(StreamReadFuture&& other) noexcept;
     StreamReadFuture& operator=(StreamReadFuture&& other) noexcept;
+    StreamReadFuture(const StreamReadFuture&) = delete;
+    StreamReadFuture& operator=(const StreamReadFuture&) = delete;
 
-    ~StreamReadFuture() noexcept;
+    ~StreamReadFuture();
 
     /// @brief Await response
     ///
@@ -131,8 +133,8 @@ class [[nodiscard]] StreamReadFuture {
     [[nodiscard]] bool IsReady() const noexcept;
 
 private:
-    impl::FutureImpl impl_;
-    typename RPC::RawStream* stream_;
+    impl::RpcData* data_{};
+    typename RPC::RawStream* stream_{};
     std::function<void(impl::RpcData& data)> post_recv_message_;
     std::function<void(impl::RpcData& data, const grpc::Status& status)> post_finish_;
 };
@@ -424,53 +426,57 @@ class [[nodiscard]] BidirectionalStream final : public CallAnyBase {
     impl::RawReaderWriter<Request, Response> stream_;
 };
 
-// ========================== Implementation follows ==========================
-
 template <typename RPC>
 StreamReadFuture<RPC>::StreamReadFuture(
     impl::RpcData& data,
     typename RPC::RawStream& stream,
     std::function<void(impl::RpcData& data)> post_recv_message,
     std::function<void(impl::RpcData& data, const grpc::Status& status)> post_finish
 ) noexcept
-    : impl_(data),
+    : data_(&data),
       stream_(&stream),
       post_recv_message_(std::move(post_recv_message)),
       post_finish_(std::move(post_finish)) {}
 
 template <typename RPC>
-StreamReadFuture<RPC>::~StreamReadFuture() noexcept {
-    if (auto* const data = impl_.GetData()) {
-        impl::RpcData::AsyncMethodInvocationGuard guard(*data);
-        const auto wait_status = impl::Wait(data->GetAsyncMethodInvocation(), data->GetContext());
-        if (wait_status != impl::AsyncMethodInvocation::WaitStatus::kOk) {
-            if (wait_status == impl::AsyncMethodInvocation::WaitStatus::kCancelled) {
-                data->GetStatsScope().OnCancelled();
-            }
-            impl::Finish(*stream_, *data, post_finish_, false);
-        } else {
-            post_recv_message_(*data);
-        }
-    }
-}
+StreamReadFuture<RPC>::StreamReadFuture(StreamReadFuture&& other) noexcept
+    : data_{std::exchange(other.data_, nullptr)},
+      stream_{other.stream_},
+      post_recv_message_{std::move(other.post_recv_message_)},
+      post_finish_{std::move(other.post_finish_)} {}
 
 template <typename RPC>
 StreamReadFuture<RPC>& StreamReadFuture<RPC>::operator=(StreamReadFuture<RPC>&& other) noexcept {
     if (this == &other) return *this;
     [[maybe_unused]] auto for_destruction = std::move(*this);
-    impl_ = std::move(other.impl_);
+    data_ = std::exchange(other.data_, nullptr);
     stream_ = other.stream_;
     post_recv_message_ = std::move(other.post_recv_message_);
     post_finish_ = std::move(other.post_finish_);
     return *this;
 }
 
+template <typename RPC>
+StreamReadFuture<RPC>::~StreamReadFuture() {
+    if (data_) {
+        impl::RpcData::AsyncMethodInvocationGuard guard(*data_);
+        const auto wait_status = impl::Wait(data_->GetAsyncMethodInvocation(), data_->GetContext());
+        if (wait_status != impl::AsyncMethodInvocation::WaitStatus::kOk) {
+            if (wait_status == impl::AsyncMethodInvocation::WaitStatus::kCancelled) {
+                data_->GetStatsScope().OnCancelled();
+            }
+            impl::Finish(*stream_, *data_, post_finish_, false);
+        } else {
+            post_recv_message_(*data_);
+        }
+    }
+}
+
 template <typename RPC>
 bool StreamReadFuture<RPC>::Get() {
-    auto* const data = impl_.GetData();
-    UINVARIANT(data, "'Get' must be called only once");
-    impl::RpcData::AsyncMethodInvocationGuard guard(*data);
-    impl_.ClearData();
+    UINVARIANT(data_, "'Get' must be called only once");
+    impl::RpcData::AsyncMethodInvocationGuard guard(*data_);
+    auto* const data = std::exchange(data_, nullptr);
     const auto result = impl::Wait(data->GetAsyncMethodInvocation(), data->GetContext());
     if (result == impl::AsyncMethodInvocation::WaitStatus::kCancelled) {
         data->GetStatsScope().OnCancelled();
@@ -487,9 +493,8 @@ bool StreamReadFuture<RPC>::Get() {
 
 template <typename RPC>
 bool StreamReadFuture<RPC>::IsReady() const noexcept {
-    auto* const data = impl_.GetData();
-    UINVARIANT(data, "IsReady should be called only before 'Get'");
-    auto& method = data->GetAsyncMethodInvocation();
+    UINVARIANT(data_, "IsReady should be called only before 'Get'");
+    auto& method = data_->GetAsyncMethodInvocation();
     return method.IsReady();
 }
 

grpc/src/ugrpc/client/impl/async_methods.cpp

@@ -75,20 +75,6 @@ void SetErrorForSpan(RpcData& data, std::string&& message) {
 RpcConfigValues::RpcConfigValues(const dynamic_config::Snapshot& config)
     : enforce_task_deadline(config[kEnforceClientTaskDeadline]) {}
 
-FutureImpl::FutureImpl(RpcData& data) noexcept : data_(&data) {}
-
-FutureImpl::FutureImpl(FutureImpl&& other) noexcept : data_(std::exchange(other.data_, nullptr)) {}
-
-FutureImpl& FutureImpl::operator=(FutureImpl&& other) noexcept {
-    if (this == &other) return *this;
-    data_ = std::exchange(other.data_, nullptr);
-    return *this;
-}
-
-RpcData* FutureImpl::GetData() const noexcept { return data_; }
-
-void FutureImpl::ClearData() noexcept { data_ = nullptr; }
-
 RpcData::RpcData(impl::CallParams&& params, CallKind call_kind)
     : context_(std::move(params.context)),
       client_name_(params.client_name),

grpc/src/ugrpc/client/rpc.cpp

@@ -45,28 +45,39 @@ UnaryFuture::UnaryFuture(
     impl::RpcData& data,
     std::function<void(impl::RpcData& data, const grpc::Status& status)> post_finish
 ) noexcept
-    : impl_(data), post_finish_(std::move(post_finish)) {
+    : data_(&data), post_finish_(std::move(post_finish)) {
     // We expect that FinishAsyncMethodInvocation was already emplaced
     // For unary future it is done in UnaryCall::FinishAsync
-    UASSERT(data.HoldsFinishAsyncMethodInvocationDebug());
+    UASSERT(data_->HoldsFinishAsyncMethodInvocationDebug());
 }
 
-UnaryFuture::~UnaryFuture() noexcept {
-    if (auto* const data = impl_.GetData()) {
-        impl::RpcData::AsyncMethodInvocationGuard guard(*data);
+UnaryFuture::UnaryFuture(UnaryFuture&& other) noexcept
+    : data_{std::exchange(other.data_, nullptr)}, post_finish_{std::move(other.post_finish_)} {}
 
-        auto& finish = data->GetFinishAsyncMethodInvocation();
+UnaryFuture& UnaryFuture::operator=(UnaryFuture&& other) noexcept {
+    if (this == &other) return *this;
+    [[maybe_unused]] auto for_destruction = std::move(*this);
+    data_ = std::exchange(other.data_, nullptr);
+    post_finish_ = std::move(other.post_finish_);
+    return *this;
+}
+
+UnaryFuture::~UnaryFuture() {
+    if (data_) {
+        impl::RpcData::AsyncMethodInvocationGuard guard(*data_);
 
-        data->GetContext().TryCancel();
+        auto& finish = data_->GetFinishAsyncMethodInvocation();
 
-        const auto wait_status = impl::Wait(finish, data->GetContext());
+        data_->GetContext().TryCancel();
+
+        const auto wait_status = impl::Wait(finish, data_->GetContext());
 
         switch (wait_status) {
             case impl::AsyncMethodInvocation::WaitStatus::kOk:
                 [[fallthrough]];
             case impl::AsyncMethodInvocation::WaitStatus::kError:
                 impl::ProcessFinishResult(
-                    *data,
+                    *data_,
                     wait_status,
                     std::move(finish.GetStatus()),
                     std::move(finish.GetParsedGStatus()),
@@ -75,7 +86,7 @@ UnaryFuture::~UnaryFuture() noexcept {
                 );
                 break;
             case impl::AsyncMethodInvocation::WaitStatus::kCancelled:
-                data->GetStatsScope().OnCancelled();
+                data_->GetStatsScope().OnCancelled();
                 break;
             case impl::AsyncMethodInvocation::WaitStatus::kDeadline:
                 UASSERT_MSG(false, "Unexpected status 'kDeadline' at UnaryFuture destruction");
@@ -84,31 +95,23 @@ UnaryFuture::~UnaryFuture() noexcept {
     }
 }
 
-UnaryFuture& UnaryFuture::operator=(UnaryFuture&& other) noexcept {
-    if (this == &other) return *this;
-    [[maybe_unused]] auto for_destruction = std::move(*this);
-    impl_ = std::move(other.impl_);
-    post_finish_ = std::move(other.post_finish_);
-    return *this;
-}
-
 void UnaryFuture::Get() {
-    auto* const data = impl_.GetData();
-    UINVARIANT(data, "'Get' should not be called after readiness");
+    UINVARIANT(data_, "'Get' should not be called after readiness");
+    auto* const data = data_;
 
     const auto result = Get(engine::Deadline{});
     UASSERT_MSG(result != engine::FutureStatus::kTimeout, "kTimeout has happened for infinite timeout");
 
     if (result == engine::FutureStatus::kCancelled) {
-        UASSERT_MSG(!impl_.GetData(), "Data should be cleaned up before RpcCancelledError generation");
+        UASSERT_MSG(!data_, "Data should be cleaned up before RpcCancelledError generation");
         throw RpcCancelledError(data->GetCallName(), "Get()");
     }
 }
 
 engine::FutureStatus UnaryFuture::Get(engine::Deadline deadline) {
-    auto* const data = impl_.GetData();
-    UINVARIANT(data, "'Get' should not be called after readiness");
-    impl::RpcData::AsyncMethodInvocationGuard guard(*data);
+    UINVARIANT(data_, "'Get' should not be called after readiness");
+    impl::RpcData::AsyncMethodInvocationGuard guard(*data_);
+    auto* const data = data_;
 
     auto& finish = data->GetFinishAsyncMethodInvocation();
 
@@ -132,7 +135,7 @@ engine::FutureStatus UnaryFuture::Get(engine::Deadline deadline) {
         // All used data could be cleared as it is not required anymore.
         // AsyncMethodInvocation object also should be cleared and in result
         // destructor will not wait any finalization from it.
-        impl_.ClearData();
+        data_ = nullptr;
     }
 
     switch (wait_status) {
@@ -163,20 +166,18 @@ engine::impl::ContextAccessor* UnaryFuture::TryGetContextAccessor() noexcept {
     // Unfortunately, we can't require that TryGetContextAccessor is not called
     // after future is finished - it doesn't match pattern usage of WaitAny
     // Instead we should return nullptr
-    auto* const data = impl_.GetData();
-    if (!data) {
+    if (!data_) {
         return nullptr;
     }
 
     // if data exists, then FinishAsyncMethodInvocation also exists
-    auto& finish = data->GetFinishAsyncMethodInvocation();
+    auto& finish = data_->GetFinishAsyncMethodInvocation();
     return finish.TryGetContextAccessor();
 }
 
 bool UnaryFuture::IsReady() const noexcept {
-    auto* const data = impl_.GetData();
-    UINVARIANT(data, "IsReady should be called only before 'Get'");
-    auto& finish = data->GetFinishAsyncMethodInvocation();
+    UINVARIANT(data_, "IsReady should be called only before 'Get'");
+    auto& finish = data_->GetFinishAsyncMethodInvocation();
     return finish.IsReady();
 }