BGQ99 commented on code in PR #3626:
URL: https://github.com/apache/brpc/pull/3626#discussion_r4334080563
##########
test/brpc_urma_unittest.cpp:
##########
@@ -41,6 +43,91 @@ extern butil::atomic<bool> g_urma_available;
} // namespace urma
} // namespace brpc
+class UrmaSendCompletionTest : public ::testing::Test {
+protected:
+ void SetUp() override {
+ SocketOptions options;
+ SocketId id;
+ ASSERT_EQ(0, Socket::Create(options, &id));
+ ASSERT_EQ(0, Socket::Address(id, &_socket));
+ }
+
+ void TearDown() override {
+ if (_socket) {
+ _socket->SetFailed();
+ }
+ }
+
+ static urma_cr_t SendCompletion() {
+ urma_cr_t cr{};
+ cr.status = URMA_CR_SUCCESS;
+ cr.user_ctx = 1;
+ return cr;
+ }
+
+ SocketUniquePtr _socket;
+};
+
+TEST_F(UrmaSendCompletionTest, reclaim_buffer_before_publishing_credit) {
+ char payload = 'x';
+ bool released = false;
+ urma::UrmaEndpoint endpoint(_socket.get());
+ endpoint._sbuf.resize(endpoint._sq_size -
+ urma::UrmaEndpoint::RESERVED_WR_NUM);
+ endpoint._local_window_capacity = endpoint._sbuf.size();
+ endpoint._sq_window_size.store(0, butil::memory_order_relaxed);
+ ASSERT_EQ(0, endpoint._sbuf[0].append_user_data(
+ &payload, sizeof(payload), [&](void*) {
+ released = true;
+ // Observe the credit at the exact point the old buffer is freed.
+ // Publishing first lets the writer reuse a slot still being
cleared.
+ EXPECT_EQ(0, endpoint._sq_window_size.load(
+ butil::memory_order_relaxed));
+ }));
Review Comment:
These tests use the existing test build’s -fno-access-control flag, without
changing production member visibility. Direct access lets the ordering test
observe the SQ credit from the buffer deleter at the exact point of
reclamation. Adding FRIEND_TEST would make access explicit, but would not
remove the tests’ dependence on these internals. I’ve kept the existing test
convention for this focused regression fix.
##########
test/brpc_urma_unittest.cpp:
##########
@@ -41,6 +43,91 @@ extern butil::atomic<bool> g_urma_available;
} // namespace urma
} // namespace brpc
+class UrmaSendCompletionTest : public ::testing::Test {
+protected:
+ void SetUp() override {
+ SocketOptions options;
+ SocketId id;
+ ASSERT_EQ(0, Socket::Create(options, &id));
+ ASSERT_EQ(0, Socket::Address(id, &_socket));
+ }
+
+ void TearDown() override {
+ if (_socket) {
+ _socket->SetFailed();
+ }
+ }
+
+ static urma_cr_t SendCompletion() {
+ urma_cr_t cr{};
+ cr.status = URMA_CR_SUCCESS;
+ cr.user_ctx = 1;
+ return cr;
+ }
+
+ SocketUniquePtr _socket;
+};
+
+TEST_F(UrmaSendCompletionTest, reclaim_buffer_before_publishing_credit) {
+ char payload = 'x';
+ bool released = false;
+ urma::UrmaEndpoint endpoint(_socket.get());
+ endpoint._sbuf.resize(endpoint._sq_size -
+ urma::UrmaEndpoint::RESERVED_WR_NUM);
+ endpoint._local_window_capacity = endpoint._sbuf.size();
+ endpoint._sq_window_size.store(0, butil::memory_order_relaxed);
+ ASSERT_EQ(0, endpoint._sbuf[0].append_user_data(
+ &payload, sizeof(payload), [&](void*) {
+ released = true;
+ // Observe the credit at the exact point the old buffer is freed.
+ // Publishing first lets the writer reuse a slot still being
cleared.
+ EXPECT_EQ(0, endpoint._sq_window_size.load(
+ butil::memory_order_relaxed));
+ }));
+
+ ASSERT_EQ(0, endpoint.HandleCompletion(SendCompletion()));
+ EXPECT_TRUE(released);
+ EXPECT_TRUE(endpoint._sbuf[0].empty());
+ EXPECT_EQ(1, endpoint._sq_sent);
Review Comment:
Same rationale as above; this duplicates the other comment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
