github-actions[bot] commented on code in PR #65584:
URL: https://github.com/apache/doris/pull/65584#discussion_r3631340465
##########
be/test/runtime/load_stream_test.cpp:
##########
@@ -1357,4 +1376,52 @@ TEST_F(LoadStreamMgrTest,
two_client_one_close_before_the_other_open) {
}
}
+// Reproduce the split-lock close race with 3 incremental CLOSE_LOADs.
+// The debug point delays the first 2 (non-last) streams while the 3rd
+// (all_closed) drains+clears _closing_stream_ids first → the delayed
+// streams are orphaned → LoadStream is never cleaned up.
+TEST_F(LoadStreamMgrTest, incremental_close_race_orhpans_streams) {
+ MockSinkClient client;
+ auto st = client.connect_stream(NORMAL_SENDER_ID, 3);
+ ASSERT_TRUE(st.ok());
+ reset_response_stat();
+
+ // one tablet, no segments (pure close test)
+ PTabletID tablet;
+ tablet.set_partition_id(NORMAL_PARTITION_ID);
+ tablet.set_index_id(NORMAL_INDEX_ID);
+ tablet.set_tablet_id(NORMAL_TABLET_ID);
+
+ // Enable the debug point: non-last incremental streams sleep 3s.
+ auto debug_point_name =
std::string("LoadStream.close_load.delay_incremental_register");
+ auto* debug_points = DebugPoints::instance();
+ debug_points->add(debug_point_name);
Review Comment:
`DebugPoints::add()` only registers the point; `DBUG_EXECUTE_IF` is also
gated by `config::enable_debug_points`, whose default is `false`. This test
never enables that config, so the three-second delay is normally skipped and
the unconditional four-second sleep does not exercise the intended
interleaving. Please scope `config::enable_debug_points = true` with
restoration/point cleanup (ideally using a callback or latch to prove the
handlers reached the barrier rather than fixed sleeps).
##########
be/test/runtime/load_stream_test.cpp:
##########
@@ -1357,4 +1376,52 @@ TEST_F(LoadStreamMgrTest,
two_client_one_close_before_the_other_open) {
}
}
+// Reproduce the split-lock close race with 3 incremental CLOSE_LOADs.
+// The debug point delays the first 2 (non-last) streams while the 3rd
+// (all_closed) drains+clears _closing_stream_ids first → the delayed
+// streams are orphaned → LoadStream is never cleaned up.
+TEST_F(LoadStreamMgrTest, incremental_close_race_orhpans_streams) {
+ MockSinkClient client;
+ auto st = client.connect_stream(NORMAL_SENDER_ID, 3);
+ ASSERT_TRUE(st.ok());
+ reset_response_stat();
+
+ // one tablet, no segments (pure close test)
+ PTabletID tablet;
+ tablet.set_partition_id(NORMAL_PARTITION_ID);
+ tablet.set_index_id(NORMAL_INDEX_ID);
+ tablet.set_tablet_id(NORMAL_TABLET_ID);
+
+ // Enable the debug point: non-last incremental streams sleep 3s.
+ auto debug_point_name =
std::string("LoadStream.close_load.delay_incremental_register");
+ auto* debug_points = DebugPoints::instance();
+ debug_points->add(debug_point_name);
+
+ // Send 3 incremental CLOSE_LOADs. The third one is the last
+ // (all_closed); the first two will be delayed and orphaned.
+ for (int i = 0; i < 3; i++) {
+ close_load_incremental(client, {tablet}, NORMAL_SENDER_ID, 1);
+ }
+
+ // All 3 _report_result calls fired before the delay.
+ wait_for_ack(3);
+ EXPECT_EQ(g_response_stat.num, 3);
+
+ // Let the delayed streams wake up (3s sleep + small margin).
+ bthread_usleep(4 * 1000 * 1000);
+
+ // Orphan detected: LoadStream not destroyed because on_closed
+ // never fired for the 2 delayed streams.
+ auto remaining = _load_stream_mgr->get_load_stream_num();
+ EXPECT_GT(remaining, 0)
Review Comment:
This test opens only one physical stream while advertising three:
`connect_stream(..., 3)` issues one open RPC/`add_source()`, so after the first
CLOSE_LOAD erases the sole source, the next two messages are rejected as `no
open stream` (although the response counter still counts them). `remaining > 0`
therefore passes on the fixed code because two declared streams never existed,
and it is also the opposite of the fix's postcondition. Please open three
distinct `MockSinkClient` streams, send one CLOSE_LOAD on each so handlers can
overlap, validate successful EOS responses, and assert that
`get_load_stream_num()` reaches zero after the delayed handlers resume.
--
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]
