diff --git a/net/dcsctp/socket/stream_reset_handler.cc b/net/dcsctp/socket/stream_reset_handler.cc index 9d86953f44..c81b34b626 100644 --- a/net/dcsctp/socket/stream_reset_handler.cc +++ b/net/dcsctp/socket/stream_reset_handler.cc @@ -134,11 +134,16 @@ bool StreamResetHandler::ValidateReqSeqNbr( ReconfigRequestSN req_seq_nbr, std::vector& responses) { if (req_seq_nbr == last_processed_req_seq_nbr_) { - // This has already been performed previously. + // https://www.rfc-editor.org/rfc/rfc6525.html#section-5.2.1 "If the + // received RE-CONFIG chunk contains at least one request and based on the + // analysis of the Re-configuration Request Sequence Numbers this is the + // last received RE-CONFIG chunk (i.e., a retransmission), the same + // RE-CONFIG chunk MUST to be sent back in response, as it was earlier." RTC_DLOG(LS_VERBOSE) << log_prefix_ << "req=" << *req_seq_nbr - << " already processed"; + << " already processed, returning result=" + << ToString(last_processed_req_result_); responses.push_back(ReconfigurationResponseParameter( - req_seq_nbr, ResponseResult::kSuccessNothingToDo)); + req_seq_nbr, last_processed_req_result_)); return false; } @@ -170,20 +175,18 @@ void StreamResetHandler::HandleResetOutgoing( } if (ValidateReqSeqNbr(req->request_sequence_number(), responses)) { - ResponseResult result; - RTC_DLOG(LS_VERBOSE) << log_prefix_ << "Reset outgoing streams with req_seq_nbr=" << *req->request_sequence_number(); last_processed_req_seq_nbr_ = req->request_sequence_number(); - result = reassembly_queue_->ResetStreams( + last_processed_req_result_ = reassembly_queue_->ResetStreams( *req, data_tracker_->last_cumulative_acked_tsn()); - if (result == ResponseResult::kSuccessPerformed) { + if (last_processed_req_result_ == ResponseResult::kSuccessPerformed) { ctx_->callbacks().OnIncomingStreamsReset(req->stream_ids()); } responses.push_back(ReconfigurationResponseParameter( - req->request_sequence_number(), result)); + req->request_sequence_number(), last_processed_req_result_)); } } diff --git a/net/dcsctp/socket/stream_reset_handler.h b/net/dcsctp/socket/stream_reset_handler.h index 6e49665538..fa32e5fcc9 100644 --- a/net/dcsctp/socket/stream_reset_handler.h +++ b/net/dcsctp/socket/stream_reset_handler.h @@ -88,8 +88,9 @@ class StreamResetHandler { last_processed_req_seq_nbr_( handover_state ? ReconfigRequestSN( handover_state->rx.last_completed_reset_req_sn) - : ReconfigRequestSN(*ctx_->peer_initial_tsn() - 1)) { - } + : ReconfigRequestSN(*ctx_->peer_initial_tsn() - 1)), + last_processed_req_result_( + ReconfigurationResponseParameter::Result::kSuccessNothingToDo) {} // Initiates reset of the provided streams. While there can only be one // ongoing stream reset request at any time, this method can be called at any @@ -224,6 +225,8 @@ class StreamResetHandler { // For incoming requests - last processed request sequence number. ReconfigRequestSN last_processed_req_seq_nbr_; + // The result from last processed incoming request + ReconfigurationResponseParameter::Result last_processed_req_result_; }; } // namespace dcsctp diff --git a/net/dcsctp/socket/stream_reset_handler_test.cc b/net/dcsctp/socket/stream_reset_handler_test.cc index 493b4c4bf7..d3fa98752c 100644 --- a/net/dcsctp/socket/stream_reset_handler_test.cc +++ b/net/dcsctp/socket/stream_reset_handler_test.cc @@ -586,36 +586,20 @@ TEST_F(StreamResetHandlerTest, SendIncomingResetJustReturnsNothingPerformed) { EXPECT_THAT(responses[0].result(), ResponseResult::kSuccessNothingToDo); } -TEST_F(StreamResetHandlerTest, SendSameRequestTwiceReturnsNothingToDo) { - reasm_->Add(kPeerInitialTsn, gen_.Ordered({1, 2, 3, 4}, "BE")); - reasm_->Add(AddTo(kPeerInitialTsn, 1), gen_.Ordered({1, 2, 3, 4}, "BE")); +TEST_F(StreamResetHandlerTest, SendSameRequestTwiceIsIdempotent) { + // Simulate that receiving the same chunk twice (due to network issues, + // or retransmissions, causing a RECONFIG to be re-received) is idempotent. + for (int i = 0; i < 2; ++i) { + Parameters::Builder builder; + builder.Add(OutgoingSSNResetRequestParameter( + kPeerInitialReqSn, ReconfigRequestSN(3), AddTo(kPeerInitialTsn, 1), + {StreamID(1)})); - data_tracker_->Observe(kPeerInitialTsn); - data_tracker_->Observe(AddTo(kPeerInitialTsn, 1)); - EXPECT_THAT(reasm_->FlushMessages(), - UnorderedElementsAre( - SctpMessageIs(StreamID(1), PPID(53), kShortPayload), - SctpMessageIs(StreamID(1), PPID(53), kShortPayload))); - - Parameters::Builder builder1; - builder1.Add(OutgoingSSNResetRequestParameter( - kPeerInitialReqSn, ReconfigRequestSN(3), AddTo(kPeerInitialTsn, 1), - {StreamID(1)})); - - std::vector responses1 = - HandleAndCatchResponse(ReConfigChunk(builder1.Build())); - EXPECT_THAT(responses1, SizeIs(1)); - EXPECT_EQ(responses1[0].result(), ResponseResult::kSuccessPerformed); - - Parameters::Builder builder2; - builder2.Add(OutgoingSSNResetRequestParameter( - kPeerInitialReqSn, ReconfigRequestSN(3), AddTo(kPeerInitialTsn, 1), - {StreamID(1)})); - - std::vector responses2 = - HandleAndCatchResponse(ReConfigChunk(builder2.Build())); - EXPECT_THAT(responses2, SizeIs(1)); - EXPECT_EQ(responses2[0].result(), ResponseResult::kSuccessNothingToDo); + std::vector responses1 = + HandleAndCatchResponse(ReConfigChunk(builder.Build())); + EXPECT_THAT(responses1, SizeIs(1)); + EXPECT_EQ(responses1[0].result(), ResponseResult::kInProgress); + } } TEST_F(StreamResetHandlerTest,