Skip to content

Commit 3f58381

Browse files
sazonovkirillmeta-codesync[bot]
authored andcommitted
Fix flaky DuplicateStreamIdThrows tests
Summary: The DuplicateStreamIdThrows tests in SinkServiceTest and StreamServiceTest were flaky due to a race condition in the server's connection close flow. When the server detects a duplicate stream ID, it calls close() which sends a connection-level error frame and sets the state to CLOSING. However, closeIfNeeded() won't fully close the connection while there are inflight requests. The handler for the first (valid) frame can still complete and send a response before the connection fully closes. If the client receives this response before the connection-level error frame, co_await completes successfully instead of throwing TTransportException. Fixed both tests to handle both race outcomes: 1. If co_await throws TTransportException - pass (duplicate detected before response arrived) 2. If co_await succeeds - verify the connection is broken by asserting a follow-up request throws TTransportException Both outcomes correctly verify the server handles duplicate stream IDs. Reviewed By: evanjzou Differential Revision: D94856980 fbshipit-source-id: f8e463d3fcbf319d94c0cb6ff0e8921b1ea8650e
1 parent 1ef7083 commit 3f58381

2 files changed

Lines changed: 24 additions & 4 deletions

File tree

third-party/thrift/src/thrift/lib/cpp2/async/tests/SinkServiceTest.cpp

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -403,8 +403,18 @@ TEST_F(SinkServiceTest, DuplicateStreamIdThrows) {
403403
[](Client<TestSinkService>& client) -> folly::coro::Task<void> {
404404
// dummy request to send setup frame
405405
co_await client.co_test();
406-
// sink request frame will now be sent twice with the same stream id
407-
EXPECT_THROW(co_await client.co_range(0, 100), TTransportException);
406+
// sink request frame will now be sent twice with the same stream id.
407+
// The server detects the duplicate and closes the connection, but due
408+
// to a race the initial response may arrive before the connection-level
409+
// error. In either case the connection should be broken afterwards.
410+
try {
411+
co_await client.co_range(0, 100);
412+
} catch (const TTransportException&) {
413+
co_return;
414+
}
415+
// Initial response arrived before connection error; verify the
416+
// connection is now broken.
417+
EXPECT_THROW(co_await client.co_test(), TTransportException);
408418
});
409419
}
410420

third-party/thrift/src/thrift/lib/cpp2/async/tests/StreamServiceTest.cpp

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -143,8 +143,18 @@ TYPED_TEST(StreamServiceTest, DuplicateStreamIdThrows) {
143143
[](Client<TestStreamService>& client) -> folly::coro::Task<void> {
144144
// dummy request to send setup frame
145145
co_await client.co_test();
146-
// sink request frame will now be sent twice with the same stream id
147-
EXPECT_THROW(co_await client.co_range(0, 100), TTransportException);
146+
// stream request frame will now be sent twice with the same stream id.
147+
// The server detects the duplicate and closes the connection, but due
148+
// to a race the initial response may arrive before the connection-level
149+
// error. In either case the connection should be broken afterwards.
150+
try {
151+
co_await client.co_range(0, 100);
152+
} catch (const TTransportException&) {
153+
co_return;
154+
}
155+
// Initial response arrived before connection error; verify the
156+
// connection is now broken.
157+
EXPECT_THROW(co_await client.co_test(), TTransportException);
148158
});
149159
}
150160

0 commit comments

Comments
 (0)