From 41e42946e63e7e0e0e61a595aae30192bfafeb6d Mon Sep 17 00:00:00 2001 From: Anna Levenberg Date: Wed, 10 Jan 2024 17:40:06 -0500 Subject: [PATCH 01/12] feat(pubsub): add lease management for unary pull --- .../internal/default_pull_ack_handler.cc | 17 +++++++++- .../internal/default_pull_ack_handler.h | 8 +++++ .../internal/default_pull_ack_handler_test.cc | 31 +++++++++++++++---- 3 files changed, 49 insertions(+), 7 deletions(-) diff --git a/google/cloud/pubsub/internal/default_pull_ack_handler.cc b/google/cloud/pubsub/internal/default_pull_ack_handler.cc index eb36a786de3e3..ffb29232af569 100644 --- a/google/cloud/pubsub/internal/default_pull_ack_handler.cc +++ b/google/cloud/pubsub/internal/default_pull_ack_handler.cc @@ -36,7 +36,22 @@ DefaultPullAckHandler::DefaultPullAckHandler(CompletionQueue cq, ack_id_(std::move(ack_id)), delivery_attempt_(delivery_attempt), lease_manager_( - MakePullLeaseManager(cq_, stub_, subscription_, ack_id_, options)) {} + MakePullLeaseManager(cq_, stub_, subscription_, ack_id_, options)) { + initialize(); +} + +DefaultPullAckHandler::DefaultPullAckHandler( + CompletionQueue cq, std::weak_ptr w, Options const& options, + pubsub::Subscription subscription, std::string ack_id, + std::int32_t delivery_attempt, std::shared_ptr manager) + : cq_(std::move(cq)), + stub_(std::move(w)), + subscription_(std::move(subscription)), + ack_id_(std::move(ack_id)), + delivery_attempt_(delivery_attempt), + lease_manager_(std::move(manager)) { + initialize(); +} DefaultPullAckHandler::~DefaultPullAckHandler() = default; diff --git a/google/cloud/pubsub/internal/default_pull_ack_handler.h b/google/cloud/pubsub/internal/default_pull_ack_handler.h index f64100431fe0a..1849ea5ba49c9 100644 --- a/google/cloud/pubsub/internal/default_pull_ack_handler.h +++ b/google/cloud/pubsub/internal/default_pull_ack_handler.h @@ -46,6 +46,12 @@ class DefaultPullAckHandler : public pubsub::PullAckHandler::Impl { Options const& options, pubsub::Subscription subscription, std::string ack_id, std::int32_t delivery_attempt); + // For testing. + DefaultPullAckHandler(CompletionQueue cq, std::weak_ptr w, + Options const& options, + pubsub::Subscription subscription, std::string ack_id, + std::int32_t delivery_attempt, + std::shared_ptr manager); ~DefaultPullAckHandler() override; future ack() override; @@ -55,6 +61,8 @@ class DefaultPullAckHandler : public pubsub::PullAckHandler::Impl { pubsub::Subscription subscription() const override; private: + void initialize() { lease_manager_->StartLeaseLoop(); } + CompletionQueue cq_; std::weak_ptr stub_; pubsub::Subscription subscription_; diff --git a/google/cloud/pubsub/internal/default_pull_ack_handler_test.cc b/google/cloud/pubsub/internal/default_pull_ack_handler_test.cc index d6eac32bddc92..d65b64463acf4 100644 --- a/google/cloud/pubsub/internal/default_pull_ack_handler_test.cc +++ b/google/cloud/pubsub/internal/default_pull_ack_handler_test.cc @@ -16,6 +16,7 @@ #include "google/cloud/pubsub/internal/default_pull_lease_manager.h" #include "google/cloud/pubsub/options.h" #include "google/cloud/pubsub/retry_policy.h" +#include "google/cloud/pubsub/testing/mock_pull_lease_manager.h" #include "google/cloud/pubsub/testing/mock_subscriber_stub.h" #include "google/cloud/pubsub/testing/test_retry_policies.h" #include "google/cloud/testing_util/async_sequencer.h" @@ -29,6 +30,7 @@ namespace pubsub_internal { GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN namespace { +using ::google::cloud::pubsub_testing::MockPullLeaseManager; using ::google::cloud::pubsub_testing::MockSubscriberStub; using ::google::cloud::testing_util::AsyncSequencer; using ::google::cloud::testing_util::StatusIs; @@ -96,7 +98,8 @@ TEST(PullAckHandlerTest, AckSimple) { AsyncSequencer aseq; auto cq = MakeMockCompletionQueue(aseq); auto handler = std::make_unique( - cq, mock, MakeTestOptions(), subscription, "test-ack-id", 42); + cq, mock, MakeTestOptions(), subscription, "test-ack-id", 42, + std::make_shared()); EXPECT_EQ(handler->delivery_attempt(), 42); auto status = handler->ack(); auto timer = aseq.PopFrontWithName(); @@ -120,7 +123,8 @@ TEST(PullAckHandlerTest, AckPermanentError) { AsyncSequencer aseq; auto cq = MakeMockCompletionQueue(aseq); auto handler = std::make_unique( - cq, mock, MakeTestOptions(), subscription, "test-ack-id", 42); + cq, mock, MakeTestOptions(), subscription, "test-ack-id", 42, + std::make_shared()); EXPECT_EQ(handler->delivery_attempt(), 42); auto status = handler->ack(); EXPECT_THAT(status.get(), @@ -144,7 +148,8 @@ TEST(PullAckHandlerTest, NackSimple) { AsyncSequencer aseq; auto cq = MakeMockCompletionQueue(aseq); auto handler = std::make_unique( - cq, mock, MakeTestOptions(), subscription, "test-ack-id", 42); + cq, mock, MakeTestOptions(), subscription, "test-ack-id", 42, + std::make_shared()); EXPECT_EQ(handler->delivery_attempt(), 42); auto status = handler->nack(); auto timer = aseq.PopFrontWithName(); @@ -170,20 +175,34 @@ TEST(PullAckHandlerTest, NackPermanentError) { AsyncSequencer aseq; auto cq = MakeMockCompletionQueue(aseq); auto handler = std::make_unique( - cq, mock, MakeTestOptions(), subscription, "test-ack-id", 42); + cq, mock, MakeTestOptions(), subscription, "test-ack-id", 42, + std::make_shared()); EXPECT_EQ(handler->delivery_attempt(), 42); auto status = handler->nack(); EXPECT_THAT(status.get(), StatusIs(StatusCode::kPermissionDenied, HasSubstr("uh-oh"))); } +TEST(PullAckHandlerTest, StartsLeaseManager) { + auto subscription = pubsub::Subscription("test-project", "test-subscription"); + + auto mock = std::make_shared(); + AsyncSequencer aseq; + auto cq = MakeMockCompletionQueue(aseq); + auto lm = std::make_shared(); + EXPECT_CALL(*lm, StartLeaseLoop()).Times(1); + auto handler = std::make_unique( + cq, mock, MakeTestOptions(), subscription, "test-ack-id", 42, lm); +} + TEST(AckHandlerTest, Subscription) { auto subscription = pubsub::Subscription("test-project", "test-subscription"); auto mock = std::make_shared(); AsyncSequencer aseq; auto cq = MakeMockCompletionQueue(aseq); auto handler = std::make_unique( - cq, mock, MakeTestOptions(), subscription, "test-ack-id", 42); + cq, mock, MakeTestOptions(), subscription, "test-ack-id", 42, + std::make_shared()); EXPECT_EQ(handler->subscription(), subscription); } @@ -195,7 +214,7 @@ TEST(AckHandlerTest, AckId) { auto handler = std::make_unique( cq, mock, MakeTestOptions(), pubsub::Subscription("test-project", "test-subscription"), "test-ack-id", - 42); + 42, std::make_shared()); EXPECT_EQ(handler->ack_id(), "test-ack-id"); } From f0831b5eb71bf604b5cb89b81f3c6d7c44a51e80 Mon Sep 17 00:00:00 2001 From: Anna Levenberg Date: Fri, 12 Jan 2024 11:08:51 -0500 Subject: [PATCH 02/12] remove unused param --- google/cloud/pubsub/internal/default_pull_ack_handler.cc | 2 +- google/cloud/pubsub/internal/default_pull_ack_handler.h | 5 ++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/google/cloud/pubsub/internal/default_pull_ack_handler.cc b/google/cloud/pubsub/internal/default_pull_ack_handler.cc index ffb29232af569..3337d6e77dae5 100644 --- a/google/cloud/pubsub/internal/default_pull_ack_handler.cc +++ b/google/cloud/pubsub/internal/default_pull_ack_handler.cc @@ -41,7 +41,7 @@ DefaultPullAckHandler::DefaultPullAckHandler(CompletionQueue cq, } DefaultPullAckHandler::DefaultPullAckHandler( - CompletionQueue cq, std::weak_ptr w, Options const& options, + CompletionQueue cq, std::weak_ptr w, Options const&, pubsub::Subscription subscription, std::string ack_id, std::int32_t delivery_attempt, std::shared_ptr manager) : cq_(std::move(cq)), diff --git a/google/cloud/pubsub/internal/default_pull_ack_handler.h b/google/cloud/pubsub/internal/default_pull_ack_handler.h index 1849ea5ba49c9..9f9fc84b213de 100644 --- a/google/cloud/pubsub/internal/default_pull_ack_handler.h +++ b/google/cloud/pubsub/internal/default_pull_ack_handler.h @@ -48,9 +48,8 @@ class DefaultPullAckHandler : public pubsub::PullAckHandler::Impl { std::int32_t delivery_attempt); // For testing. DefaultPullAckHandler(CompletionQueue cq, std::weak_ptr w, - Options const& options, - pubsub::Subscription subscription, std::string ack_id, - std::int32_t delivery_attempt, + Options const&, pubsub::Subscription subscription, + std::string ack_id, std::int32_t delivery_attempt, std::shared_ptr manager); ~DefaultPullAckHandler() override; From c9773e34430a9b53a83f68792a38cc77472c8d57 Mon Sep 17 00:00:00 2001 From: Anna Levenberg Date: Fri, 12 Jan 2024 12:28:44 -0500 Subject: [PATCH 03/12] remove options param --- .../internal/default_pull_ack_handler.cc | 2 +- .../internal/default_pull_ack_handler.h | 4 ++-- .../internal/default_pull_ack_handler_test.cc | 19 +++++++++---------- 3 files changed, 12 insertions(+), 13 deletions(-) diff --git a/google/cloud/pubsub/internal/default_pull_ack_handler.cc b/google/cloud/pubsub/internal/default_pull_ack_handler.cc index 3337d6e77dae5..a4f98fd1091ab 100644 --- a/google/cloud/pubsub/internal/default_pull_ack_handler.cc +++ b/google/cloud/pubsub/internal/default_pull_ack_handler.cc @@ -41,7 +41,7 @@ DefaultPullAckHandler::DefaultPullAckHandler(CompletionQueue cq, } DefaultPullAckHandler::DefaultPullAckHandler( - CompletionQueue cq, std::weak_ptr w, Options const&, + CompletionQueue cq, std::weak_ptr w, pubsub::Subscription subscription, std::string ack_id, std::int32_t delivery_attempt, std::shared_ptr manager) : cq_(std::move(cq)), diff --git a/google/cloud/pubsub/internal/default_pull_ack_handler.h b/google/cloud/pubsub/internal/default_pull_ack_handler.h index 9f9fc84b213de..54d414c45a87b 100644 --- a/google/cloud/pubsub/internal/default_pull_ack_handler.h +++ b/google/cloud/pubsub/internal/default_pull_ack_handler.h @@ -48,8 +48,8 @@ class DefaultPullAckHandler : public pubsub::PullAckHandler::Impl { std::int32_t delivery_attempt); // For testing. DefaultPullAckHandler(CompletionQueue cq, std::weak_ptr w, - Options const&, pubsub::Subscription subscription, - std::string ack_id, std::int32_t delivery_attempt, + pubsub::Subscription subscription, std::string ack_id, + std::int32_t delivery_attempt, std::shared_ptr manager); ~DefaultPullAckHandler() override; diff --git a/google/cloud/pubsub/internal/default_pull_ack_handler_test.cc b/google/cloud/pubsub/internal/default_pull_ack_handler_test.cc index d65b64463acf4..4702c9b238139 100644 --- a/google/cloud/pubsub/internal/default_pull_ack_handler_test.cc +++ b/google/cloud/pubsub/internal/default_pull_ack_handler_test.cc @@ -98,7 +98,7 @@ TEST(PullAckHandlerTest, AckSimple) { AsyncSequencer aseq; auto cq = MakeMockCompletionQueue(aseq); auto handler = std::make_unique( - cq, mock, MakeTestOptions(), subscription, "test-ack-id", 42, + cq, mock, subscription, "test-ack-id", 42, std::make_shared()); EXPECT_EQ(handler->delivery_attempt(), 42); auto status = handler->ack(); @@ -123,7 +123,7 @@ TEST(PullAckHandlerTest, AckPermanentError) { AsyncSequencer aseq; auto cq = MakeMockCompletionQueue(aseq); auto handler = std::make_unique( - cq, mock, MakeTestOptions(), subscription, "test-ack-id", 42, + cq, mock, subscription, "test-ack-id", 42, std::make_shared()); EXPECT_EQ(handler->delivery_attempt(), 42); auto status = handler->ack(); @@ -148,7 +148,7 @@ TEST(PullAckHandlerTest, NackSimple) { AsyncSequencer aseq; auto cq = MakeMockCompletionQueue(aseq); auto handler = std::make_unique( - cq, mock, MakeTestOptions(), subscription, "test-ack-id", 42, + cq, mock, subscription, "test-ack-id", 42, std::make_shared()); EXPECT_EQ(handler->delivery_attempt(), 42); auto status = handler->nack(); @@ -175,7 +175,7 @@ TEST(PullAckHandlerTest, NackPermanentError) { AsyncSequencer aseq; auto cq = MakeMockCompletionQueue(aseq); auto handler = std::make_unique( - cq, mock, MakeTestOptions(), subscription, "test-ack-id", 42, + cq, mock, subscription, "test-ack-id", 42, std::make_shared()); EXPECT_EQ(handler->delivery_attempt(), 42); auto status = handler->nack(); @@ -191,8 +191,8 @@ TEST(PullAckHandlerTest, StartsLeaseManager) { auto cq = MakeMockCompletionQueue(aseq); auto lm = std::make_shared(); EXPECT_CALL(*lm, StartLeaseLoop()).Times(1); - auto handler = std::make_unique( - cq, mock, MakeTestOptions(), subscription, "test-ack-id", 42, lm); + auto handler = std::make_unique(cq, mock, subscription, + "test-ack-id", 42, lm); } TEST(AckHandlerTest, Subscription) { @@ -201,7 +201,7 @@ TEST(AckHandlerTest, Subscription) { AsyncSequencer aseq; auto cq = MakeMockCompletionQueue(aseq); auto handler = std::make_unique( - cq, mock, MakeTestOptions(), subscription, "test-ack-id", 42, + cq, mock, subscription, "test-ack-id", 42, std::make_shared()); EXPECT_EQ(handler->subscription(), subscription); @@ -212,9 +212,8 @@ TEST(AckHandlerTest, AckId) { AsyncSequencer aseq; auto cq = MakeMockCompletionQueue(aseq); auto handler = std::make_unique( - cq, mock, MakeTestOptions(), - pubsub::Subscription("test-project", "test-subscription"), "test-ack-id", - 42, std::make_shared()); + cq, mock, pubsub::Subscription("test-project", "test-subscription"), + "test-ack-id", 42, std::make_shared()); EXPECT_EQ(handler->ack_id(), "test-ack-id"); } From 090aba55157c95b7efb89b1411794de824a7a278 Mon Sep 17 00:00:00 2001 From: Anna Levenberg Date: Fri, 12 Jan 2024 12:42:59 -0500 Subject: [PATCH 04/12] remove helper --- .../pubsub/internal/default_pull_ack_handler_test.cc | 8 -------- 1 file changed, 8 deletions(-) diff --git a/google/cloud/pubsub/internal/default_pull_ack_handler_test.cc b/google/cloud/pubsub/internal/default_pull_ack_handler_test.cc index 4702c9b238139..93ed0763fbfd7 100644 --- a/google/cloud/pubsub/internal/default_pull_ack_handler_test.cc +++ b/google/cloud/pubsub/internal/default_pull_ack_handler_test.cc @@ -14,7 +14,6 @@ #include "google/cloud/pubsub/internal/default_pull_ack_handler.h" #include "google/cloud/pubsub/internal/default_pull_lease_manager.h" -#include "google/cloud/pubsub/options.h" #include "google/cloud/pubsub/retry_policy.h" #include "google/cloud/pubsub/testing/mock_pull_lease_manager.h" #include "google/cloud/pubsub/testing/mock_subscriber_stub.h" @@ -47,13 +46,6 @@ using ::testing::Return; using MockClock = ::testing::MockFunction; -Options MakeTestOptions() { - return google::cloud::pubsub_testing::MakeTestOptions( - Options{} - .set(std::chrono::seconds(300)) - .set(std::chrono::seconds(10))); -} - CompletionQueue MakeMockCompletionQueue(AsyncSequencer& aseq) { auto mock = std::make_shared(); EXPECT_CALL(*mock, MakeRelativeTimer).WillRepeatedly([&](auto) { From 25f6a4a5d855bf1c95497fa13bba3a60d61f565c Mon Sep 17 00:00:00 2001 From: Anna Levenberg Date: Fri, 12 Jan 2024 14:02:48 -0500 Subject: [PATCH 05/12] fix factory test --- .../pubsub/internal/pull_ack_handler_factory_test.cc | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/google/cloud/pubsub/internal/pull_ack_handler_factory_test.cc b/google/cloud/pubsub/internal/pull_ack_handler_factory_test.cc index 468f43c9f8bc4..d11598c6f73eb 100644 --- a/google/cloud/pubsub/internal/pull_ack_handler_factory_test.cc +++ b/google/cloud/pubsub/internal/pull_ack_handler_factory_test.cc @@ -70,6 +70,10 @@ TEST(PullAckHandlerTest, AckSimple) { Property(&AcknowledgeRequest::subscription, subscription.FullName())); EXPECT_CALL(*mock, AsyncAcknowledge(_, _, request_matcher)) .WillOnce(Return(ByMove(make_ready_future(Status{})))); + // Since the lease manager is started in the constructor o the ack handler, we + // need to match the lease manager calls. + EXPECT_CALL(*mock, AsyncModifyAckDeadline(_, _, _)) + .WillOnce(Return(ByMove(make_ready_future(Status{})))); AsyncSequencer aseq; auto cq = MakeMockCompletionQueue(aseq); auto handler = @@ -100,6 +104,10 @@ TEST(PullAckHandlerTest, TracingEnabled) { Property(&AcknowledgeRequest::subscription, subscription.FullName())); EXPECT_CALL(*mock, AsyncAcknowledge(_, _, request_matcher)) .WillOnce(Return(ByMove(make_ready_future(Status{})))); + // Since the lease manager is started in the constructor o the ack handler, we + // need to match the lease manager calls. + EXPECT_CALL(*mock, AsyncModifyAckDeadline(_, _, _)) + .WillOnce(Return(ByMove(make_ready_future(Status{})))); AsyncSequencer aseq; auto cq = MakeMockCompletionQueue(aseq); auto handler = @@ -124,6 +132,10 @@ TEST(PullAckHandlerTest, TracingDisabled) { Property(&AcknowledgeRequest::subscription, subscription.FullName())); EXPECT_CALL(*mock, AsyncAcknowledge(_, _, request_matcher)) .WillOnce(Return(ByMove(make_ready_future(Status{})))); + // Since the lease manager is started in the constructor o the ack handler, we + // need to match the lease manager calls. + EXPECT_CALL(*mock, AsyncModifyAckDeadline(_, _, _)) + .WillOnce(Return(ByMove(make_ready_future(Status{})))); AsyncSequencer aseq; auto cq = MakeMockCompletionQueue(aseq); auto handler = From ec68760239e9b529a4b86e9c5807bbea5acf8dd9 Mon Sep 17 00:00:00 2001 From: Anna Levenberg Date: Tue, 16 Jan 2024 11:24:28 -0500 Subject: [PATCH 06/12] fix --- .../internal/pull_ack_handler_factory_test.cc | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/google/cloud/pubsub/internal/pull_ack_handler_factory_test.cc b/google/cloud/pubsub/internal/pull_ack_handler_factory_test.cc index d11598c6f73eb..a00fc8797d12b 100644 --- a/google/cloud/pubsub/internal/pull_ack_handler_factory_test.cc +++ b/google/cloud/pubsub/internal/pull_ack_handler_factory_test.cc @@ -70,10 +70,10 @@ TEST(PullAckHandlerTest, AckSimple) { Property(&AcknowledgeRequest::subscription, subscription.FullName())); EXPECT_CALL(*mock, AsyncAcknowledge(_, _, request_matcher)) .WillOnce(Return(ByMove(make_ready_future(Status{})))); - // Since the lease manager is started in the constructor o the ack handler, we - // need to match the lease manager calls. + // Since the lease manager is started in the constructor of the ack handler, we + // need to match the lease manager calls. EXPECT_CALL(*mock, AsyncModifyAckDeadline(_, _, _)) - .WillOnce(Return(ByMove(make_ready_future(Status{})))); + .WillRepeatedly(Return(ByMove(make_ready_future(Status{})))); AsyncSequencer aseq; auto cq = MakeMockCompletionQueue(aseq); auto handler = @@ -104,10 +104,10 @@ TEST(PullAckHandlerTest, TracingEnabled) { Property(&AcknowledgeRequest::subscription, subscription.FullName())); EXPECT_CALL(*mock, AsyncAcknowledge(_, _, request_matcher)) .WillOnce(Return(ByMove(make_ready_future(Status{})))); - // Since the lease manager is started in the constructor o the ack handler, we + // Since the lease manager is started in the constructor of the ack handler, we // need to match the lease manager calls. EXPECT_CALL(*mock, AsyncModifyAckDeadline(_, _, _)) - .WillOnce(Return(ByMove(make_ready_future(Status{})))); + .WillRepeatedly(Return(ByMove(make_ready_future(Status{})))); AsyncSequencer aseq; auto cq = MakeMockCompletionQueue(aseq); auto handler = @@ -132,10 +132,10 @@ TEST(PullAckHandlerTest, TracingDisabled) { Property(&AcknowledgeRequest::subscription, subscription.FullName())); EXPECT_CALL(*mock, AsyncAcknowledge(_, _, request_matcher)) .WillOnce(Return(ByMove(make_ready_future(Status{})))); - // Since the lease manager is started in the constructor o the ack handler, we + // Since the lease manager is started in the constructor of the ack handler, we // need to match the lease manager calls. EXPECT_CALL(*mock, AsyncModifyAckDeadline(_, _, _)) - .WillOnce(Return(ByMove(make_ready_future(Status{})))); + .WillRepeatedly(Return(ByMove(make_ready_future(Status{})))); AsyncSequencer aseq; auto cq = MakeMockCompletionQueue(aseq); auto handler = From 21445072e0fce1c005f6ae1eec5360e05a205c4b Mon Sep 17 00:00:00 2001 From: Anna Levenberg Date: Tue, 16 Jan 2024 11:25:27 -0500 Subject: [PATCH 07/12] checkers --- .../internal/pull_ack_handler_factory_test.cc | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/google/cloud/pubsub/internal/pull_ack_handler_factory_test.cc b/google/cloud/pubsub/internal/pull_ack_handler_factory_test.cc index a00fc8797d12b..0eb11238f7535 100644 --- a/google/cloud/pubsub/internal/pull_ack_handler_factory_test.cc +++ b/google/cloud/pubsub/internal/pull_ack_handler_factory_test.cc @@ -70,8 +70,9 @@ TEST(PullAckHandlerTest, AckSimple) { Property(&AcknowledgeRequest::subscription, subscription.FullName())); EXPECT_CALL(*mock, AsyncAcknowledge(_, _, request_matcher)) .WillOnce(Return(ByMove(make_ready_future(Status{})))); - // Since the lease manager is started in the constructor of the ack handler, we - // need to match the lease manager calls. + // Since the lease manager is started in the constructor of the ack handler, + // we + // need to match the lease manager calls. EXPECT_CALL(*mock, AsyncModifyAckDeadline(_, _, _)) .WillRepeatedly(Return(ByMove(make_ready_future(Status{})))); AsyncSequencer aseq; @@ -104,8 +105,8 @@ TEST(PullAckHandlerTest, TracingEnabled) { Property(&AcknowledgeRequest::subscription, subscription.FullName())); EXPECT_CALL(*mock, AsyncAcknowledge(_, _, request_matcher)) .WillOnce(Return(ByMove(make_ready_future(Status{})))); - // Since the lease manager is started in the constructor of the ack handler, we - // need to match the lease manager calls. + // Since the lease manager is started in the constructor of the ack handler, + // we need to match the lease manager calls. EXPECT_CALL(*mock, AsyncModifyAckDeadline(_, _, _)) .WillRepeatedly(Return(ByMove(make_ready_future(Status{})))); AsyncSequencer aseq; @@ -132,8 +133,8 @@ TEST(PullAckHandlerTest, TracingDisabled) { Property(&AcknowledgeRequest::subscription, subscription.FullName())); EXPECT_CALL(*mock, AsyncAcknowledge(_, _, request_matcher)) .WillOnce(Return(ByMove(make_ready_future(Status{})))); - // Since the lease manager is started in the constructor of the ack handler, we - // need to match the lease manager calls. + // Since the lease manager is started in the constructor of the ack handler, + // we need to match the lease manager calls. EXPECT_CALL(*mock, AsyncModifyAckDeadline(_, _, _)) .WillRepeatedly(Return(ByMove(make_ready_future(Status{})))); AsyncSequencer aseq; From 586a3c518ccc428d068ac32df9dce8b486b97d99 Mon Sep 17 00:00:00 2001 From: Anna Levenberg Date: Wed, 17 Jan 2024 16:11:30 -0500 Subject: [PATCH 08/12] remove ByMove, it was invalidating the future return and causing the no-ex build to fail --- .../pubsub/internal/pull_ack_handler_factory_test.cc | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/google/cloud/pubsub/internal/pull_ack_handler_factory_test.cc b/google/cloud/pubsub/internal/pull_ack_handler_factory_test.cc index 0eb11238f7535..f41c62494969b 100644 --- a/google/cloud/pubsub/internal/pull_ack_handler_factory_test.cc +++ b/google/cloud/pubsub/internal/pull_ack_handler_factory_test.cc @@ -71,10 +71,9 @@ TEST(PullAckHandlerTest, AckSimple) { EXPECT_CALL(*mock, AsyncAcknowledge(_, _, request_matcher)) .WillOnce(Return(ByMove(make_ready_future(Status{})))); // Since the lease manager is started in the constructor of the ack handler, - // we - // need to match the lease manager calls. + // we need to match the lease manager calls. EXPECT_CALL(*mock, AsyncModifyAckDeadline(_, _, _)) - .WillRepeatedly(Return(ByMove(make_ready_future(Status{})))); + .WillRepeatedly(Return(make_ready_future(Status{}))); AsyncSequencer aseq; auto cq = MakeMockCompletionQueue(aseq); auto handler = @@ -108,7 +107,7 @@ TEST(PullAckHandlerTest, TracingEnabled) { // Since the lease manager is started in the constructor of the ack handler, // we need to match the lease manager calls. EXPECT_CALL(*mock, AsyncModifyAckDeadline(_, _, _)) - .WillRepeatedly(Return(ByMove(make_ready_future(Status{})))); + .WillRepeatedly(Return(make_ready_future(Status{}))); AsyncSequencer aseq; auto cq = MakeMockCompletionQueue(aseq); auto handler = @@ -136,7 +135,7 @@ TEST(PullAckHandlerTest, TracingDisabled) { // Since the lease manager is started in the constructor of the ack handler, // we need to match the lease manager calls. EXPECT_CALL(*mock, AsyncModifyAckDeadline(_, _, _)) - .WillRepeatedly(Return(ByMove(make_ready_future(Status{})))); + .WillRepeatedly(Return(make_ready_future(Status{}))); AsyncSequencer aseq; auto cq = MakeMockCompletionQueue(aseq); auto handler = From 956212bdcd1570c4aef1509f3dd2442cd9cd37ad Mon Sep 17 00:00:00 2001 From: Anna Levenberg Date: Thu, 18 Jan 2024 11:08:22 -0500 Subject: [PATCH 09/12] til: InvokeWithoutArgs exists --- .../pubsub/internal/pull_ack_handler_factory_test.cc | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/google/cloud/pubsub/internal/pull_ack_handler_factory_test.cc b/google/cloud/pubsub/internal/pull_ack_handler_factory_test.cc index f41c62494969b..5433e5b24064e 100644 --- a/google/cloud/pubsub/internal/pull_ack_handler_factory_test.cc +++ b/google/cloud/pubsub/internal/pull_ack_handler_factory_test.cc @@ -38,6 +38,7 @@ using ::testing::AllOf; using ::testing::ByMove; using ::testing::Contains; using ::testing::ElementsAre; +using ::testing::InvokeWithoutArgs; using ::testing::IsEmpty; using ::testing::Property; using ::testing::Return; @@ -73,7 +74,8 @@ TEST(PullAckHandlerTest, AckSimple) { // Since the lease manager is started in the constructor of the ack handler, // we need to match the lease manager calls. EXPECT_CALL(*mock, AsyncModifyAckDeadline(_, _, _)) - .WillRepeatedly(Return(make_ready_future(Status{}))); + .WillRepeatedly( + InvokeWithoutArgs([]() { return make_ready_future(Status{}); })); AsyncSequencer aseq; auto cq = MakeMockCompletionQueue(aseq); auto handler = @@ -107,7 +109,8 @@ TEST(PullAckHandlerTest, TracingEnabled) { // Since the lease manager is started in the constructor of the ack handler, // we need to match the lease manager calls. EXPECT_CALL(*mock, AsyncModifyAckDeadline(_, _, _)) - .WillRepeatedly(Return(make_ready_future(Status{}))); + .WillRepeatedly( + InvokeWithoutArgs([]() { return make_ready_future(Status{}); })); AsyncSequencer aseq; auto cq = MakeMockCompletionQueue(aseq); auto handler = @@ -135,7 +138,9 @@ TEST(PullAckHandlerTest, TracingDisabled) { // Since the lease manager is started in the constructor of the ack handler, // we need to match the lease manager calls. EXPECT_CALL(*mock, AsyncModifyAckDeadline(_, _, _)) - .WillRepeatedly(Return(make_ready_future(Status{}))); + .WillRepeatedly( + InvokeWithoutArgs([]() { return make_ready_future(Status{}); })); + AsyncSequencer aseq; auto cq = MakeMockCompletionQueue(aseq); auto handler = From a407240d8df2463728f6fb8957f3496bb1bd0859 Mon Sep 17 00:00:00 2001 From: Anna Levenberg Date: Thu, 18 Jan 2024 11:14:51 -0500 Subject: [PATCH 10/12] darren's suggestion --- .../pubsub/internal/pull_ack_handler_factory_test.cc | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/google/cloud/pubsub/internal/pull_ack_handler_factory_test.cc b/google/cloud/pubsub/internal/pull_ack_handler_factory_test.cc index 5433e5b24064e..7cad97659e4f8 100644 --- a/google/cloud/pubsub/internal/pull_ack_handler_factory_test.cc +++ b/google/cloud/pubsub/internal/pull_ack_handler_factory_test.cc @@ -38,7 +38,6 @@ using ::testing::AllOf; using ::testing::ByMove; using ::testing::Contains; using ::testing::ElementsAre; -using ::testing::InvokeWithoutArgs; using ::testing::IsEmpty; using ::testing::Property; using ::testing::Return; @@ -74,8 +73,7 @@ TEST(PullAckHandlerTest, AckSimple) { // Since the lease manager is started in the constructor of the ack handler, // we need to match the lease manager calls. EXPECT_CALL(*mock, AsyncModifyAckDeadline(_, _, _)) - .WillRepeatedly( - InvokeWithoutArgs([]() { return make_ready_future(Status{}); })); + .WillRepeatedly([]() { return make_ready_future(Status{}); }); AsyncSequencer aseq; auto cq = MakeMockCompletionQueue(aseq); auto handler = @@ -109,8 +107,7 @@ TEST(PullAckHandlerTest, TracingEnabled) { // Since the lease manager is started in the constructor of the ack handler, // we need to match the lease manager calls. EXPECT_CALL(*mock, AsyncModifyAckDeadline(_, _, _)) - .WillRepeatedly( - InvokeWithoutArgs([]() { return make_ready_future(Status{}); })); + .WillRepeatedly([]() { return make_ready_future(Status{}); }); AsyncSequencer aseq; auto cq = MakeMockCompletionQueue(aseq); auto handler = @@ -138,9 +135,7 @@ TEST(PullAckHandlerTest, TracingDisabled) { // Since the lease manager is started in the constructor of the ack handler, // we need to match the lease manager calls. EXPECT_CALL(*mock, AsyncModifyAckDeadline(_, _, _)) - .WillRepeatedly( - InvokeWithoutArgs([]() { return make_ready_future(Status{}); })); - + .WillRepeatedly([]() { return make_ready_future(Status{}); }); AsyncSequencer aseq; auto cq = MakeMockCompletionQueue(aseq); auto handler = From 6fefbdfbb7742cec87ac475d2875306659dbc5b1 Mon Sep 17 00:00:00 2001 From: Anna Levenberg Date: Thu, 18 Jan 2024 11:42:15 -0500 Subject: [PATCH 11/12] checkers --- .../internal/pull_ack_handler_factory_test.cc | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/google/cloud/pubsub/internal/pull_ack_handler_factory_test.cc b/google/cloud/pubsub/internal/pull_ack_handler_factory_test.cc index 7cad97659e4f8..3ec7ad6a8f130 100644 --- a/google/cloud/pubsub/internal/pull_ack_handler_factory_test.cc +++ b/google/cloud/pubsub/internal/pull_ack_handler_factory_test.cc @@ -72,8 +72,9 @@ TEST(PullAckHandlerTest, AckSimple) { .WillOnce(Return(ByMove(make_ready_future(Status{})))); // Since the lease manager is started in the constructor of the ack handler, // we need to match the lease manager calls. - EXPECT_CALL(*mock, AsyncModifyAckDeadline(_, _, _)) - .WillRepeatedly([]() { return make_ready_future(Status{}); }); + EXPECT_CALL(*mock, AsyncModifyAckDeadline(_, _, _)).WillRepeatedly([]() { + return make_ready_future(Status{}); + }); AsyncSequencer aseq; auto cq = MakeMockCompletionQueue(aseq); auto handler = @@ -106,8 +107,9 @@ TEST(PullAckHandlerTest, TracingEnabled) { .WillOnce(Return(ByMove(make_ready_future(Status{})))); // Since the lease manager is started in the constructor of the ack handler, // we need to match the lease manager calls. - EXPECT_CALL(*mock, AsyncModifyAckDeadline(_, _, _)) - .WillRepeatedly([]() { return make_ready_future(Status{}); }); + EXPECT_CALL(*mock, AsyncModifyAckDeadline(_, _, _)).WillRepeatedly([]() { + return make_ready_future(Status{}); + }); AsyncSequencer aseq; auto cq = MakeMockCompletionQueue(aseq); auto handler = @@ -134,8 +136,9 @@ TEST(PullAckHandlerTest, TracingDisabled) { .WillOnce(Return(ByMove(make_ready_future(Status{})))); // Since the lease manager is started in the constructor of the ack handler, // we need to match the lease manager calls. - EXPECT_CALL(*mock, AsyncModifyAckDeadline(_, _, _)) - .WillRepeatedly([]() { return make_ready_future(Status{}); }); + EXPECT_CALL(*mock, AsyncModifyAckDeadline(_, _, _)).WillRepeatedly([]() { + return make_ready_future(Status{}); + }); AsyncSequencer aseq; auto cq = MakeMockCompletionQueue(aseq); auto handler = From 98e0cb871f26802ebf9cbf54c0d6eebe290e8188 Mon Sep 17 00:00:00 2001 From: Anna Levenberg Date: Thu, 25 Jan 2024 15:02:48 -0500 Subject: [PATCH 12/12] fix no-ex build --- .../internal/pull_ack_handler_factory_test.cc | 36 ++++++++++++++++--- 1 file changed, 31 insertions(+), 5 deletions(-) diff --git a/google/cloud/pubsub/internal/pull_ack_handler_factory_test.cc b/google/cloud/pubsub/internal/pull_ack_handler_factory_test.cc index 3ec7ad6a8f130..f9adc30c6a5b8 100644 --- a/google/cloud/pubsub/internal/pull_ack_handler_factory_test.cc +++ b/google/cloud/pubsub/internal/pull_ack_handler_factory_test.cc @@ -68,10 +68,9 @@ TEST(PullAckHandlerTest, AckSimple) { auto request_matcher = AllOf( Property(&AcknowledgeRequest::ack_ids, ElementsAre("test-ack-id")), Property(&AcknowledgeRequest::subscription, subscription.FullName())); - EXPECT_CALL(*mock, AsyncAcknowledge(_, _, request_matcher)) - .WillOnce(Return(ByMove(make_ready_future(Status{})))); - // Since the lease manager is started in the constructor of the ack handler, - // we need to match the lease manager calls. + EXPECT_CALL(*mock, AsyncAcknowledge(_, _, _)).WillRepeatedly([]() { + return make_ready_future(Status{}); + }); EXPECT_CALL(*mock, AsyncModifyAckDeadline(_, _, _)).WillRepeatedly([]() { return make_ready_future(Status{}); }); @@ -80,8 +79,17 @@ TEST(PullAckHandlerTest, AckSimple) { auto handler = MakePullAckHandler(std::move(cq), std::move(mock), subscription, "test-ack-id", 42, MakeTestOptions()); - + auto pending = aseq.PopFrontWithName(); + EXPECT_EQ(pending.second, "MakeRelativeTimer"); + pending.first.set_value(true); + pending = aseq.PopFrontWithName(); + EXPECT_EQ(pending.second, "MakeRelativeTimer"); EXPECT_THAT(std::move(handler).ack().get(), StatusIs(StatusCode::kOk)); + + // Terminate the loop. With exceptions disabled abandoning a future with a + // continuation results in a crash. In non-test programs, the completion queue + // does this automatically as part of its shutdown. + pending.first.set_value(false); } #ifdef GOOGLE_CLOUD_CPP_HAVE_OPENTELEMETRY @@ -115,9 +123,18 @@ TEST(PullAckHandlerTest, TracingEnabled) { auto handler = MakePullAckHandler(std::move(cq), std::move(mock), subscription, "test-ack-id", 42, EnableTracing(MakeTestOptions())); + auto pending = aseq.PopFrontWithName(); + EXPECT_EQ(pending.second, "MakeRelativeTimer"); + pending.first.set_value(true); + pending = aseq.PopFrontWithName(); + EXPECT_EQ(pending.second, "MakeRelativeTimer"); EXPECT_THAT(std::move(handler).ack().get(), StatusIs(StatusCode::kOk)); + // Terminate the loop. With exceptions disabled abandoning a future with a + // continuation results in a crash. In non-test programs, the completion queue + // does this automatically as part of its shutdown. + pending.first.set_value(false); auto spans = span_catcher->GetSpans(); EXPECT_THAT(spans, Contains(AllOf( SpanHasInstrumentationScope(), SpanKindIsClient(), @@ -144,8 +161,17 @@ TEST(PullAckHandlerTest, TracingDisabled) { auto handler = MakePullAckHandler(std::move(cq), std::move(mock), subscription, "test-ack-id", 42, DisableTracing(MakeTestOptions())); + auto pending = aseq.PopFrontWithName(); + EXPECT_EQ(pending.second, "MakeRelativeTimer"); + pending.first.set_value(true); + pending = aseq.PopFrontWithName(); + EXPECT_EQ(pending.second, "MakeRelativeTimer"); EXPECT_THAT(std::move(handler).ack().get(), StatusIs(StatusCode::kOk)); + // Terminate the loop. With exceptions disabled abandoning a future with a + // continuation results in a crash. In non-test programs, the completion queue + // does this automatically as part of its shutdown. + pending.first.set_value(false); EXPECT_THAT(span_catcher->GetSpans(), IsEmpty()); }