From 6e4e68852a99601c1c4c3d0cc3243facb14aaa5a Mon Sep 17 00:00:00 2001 From: Yura Yaroshevich Date: Mon, 28 Oct 2019 16:52:03 +0300 Subject: [PATCH] Fixed MSAN issue with usrsctp reliability test. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There is still an TSAN issues, but they are inside usrsctp library, that's why tests are still disabled by default. Bug: None Change-Id: I55f7c66b4d9a5feccd2121e2dd3b131cf1564804 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/158522 Commit-Queue: Yura Yaroshevich Reviewed-by: Patrik Höglund Cr-Commit-Position: refs/heads/master@{#29640} --- .../sctp_transport_reliability_unittest.cc | 52 +++++++++++-------- 1 file changed, 31 insertions(+), 21 deletions(-) diff --git a/media/sctp/sctp_transport_reliability_unittest.cc b/media/sctp/sctp_transport_reliability_unittest.cc index 25fdead7af..b9d8d7b3d5 100644 --- a/media/sctp/sctp_transport_reliability_unittest.cc +++ b/media/sctp/sctp_transport_reliability_unittest.cc @@ -49,10 +49,11 @@ class SimulatedPacketTransport final : public rtc::PacketTransportInternal { RTC_DCHECK_RUN_ON(transport_thread_); } - ~SimulatedPacketTransport() { + ~SimulatedPacketTransport() override { RTC_DCHECK_RUN_ON(transport_thread_); - if (destination_ != nullptr) { - invoker_.Flush(destination_->transport_thread_); + auto destination = destination_.load(); + if (destination != nullptr) { + invoker_.Flush(destination->transport_thread_); } invoker_.Flush(transport_thread_); destination_ = nullptr; @@ -70,7 +71,8 @@ class SimulatedPacketTransport final : public rtc::PacketTransportInternal { const rtc::PacketOptions& options, int flags = 0) { RTC_DCHECK_RUN_ON(transport_thread_); - if (destination_ == nullptr) { + auto destination = destination_.load(); + if (destination == nullptr) { return -1; } if (random_.Rand(100) < packet_loss_percents_) { @@ -79,11 +81,12 @@ class SimulatedPacketTransport final : public rtc::PacketTransportInternal { } rtc::CopyOnWriteBuffer buffer(data, len); auto send_job = [this, flags, buffer = std::move(buffer)] { - if (destination_ == nullptr) { + auto destination = destination_.load(); + if (destination == nullptr) { return; } - destination_->SignalReadPacket( - destination_, reinterpret_cast(buffer.data()), + destination->SignalReadPacket( + destination, reinterpret_cast(buffer.data()), buffer.size(), rtc::Time(), flags); }; // Introduce random send delay in range [0 .. 2 * avg_send_delay_millis_] @@ -96,10 +99,10 @@ class SimulatedPacketTransport final : public rtc::PacketTransportInternal { if (actual_send_delay > 0) { invoker_.AsyncInvokeDelayed(RTC_FROM_HERE, - destination_->transport_thread_, + destination->transport_thread_, std::move(send_job), actual_send_delay); } else { - invoker_.AsyncInvoke(RTC_FROM_HERE, destination_->transport_thread_, + invoker_.AsyncInvoke(RTC_FROM_HERE, destination->transport_thread_, std::move(send_job)); } return 0; @@ -129,7 +132,7 @@ class SimulatedPacketTransport final : public rtc::PacketTransportInternal { rtc::Thread* const transport_thread_; const uint8_t packet_loss_percents_; const uint16_t avg_send_delay_millis_; - SimulatedPacketTransport* destination_; + std::atomic destination_ ATOMIC_VAR_INIT(nullptr); rtc::AsyncInvoker invoker_; webrtc::Random random_; RTC_DISALLOW_COPY_AND_ASSIGN(SimulatedPacketTransport); @@ -601,8 +604,6 @@ namespace cricket { * usrsctp might misbehave in concurrent environment * under load on lossy networks: deadlocks and memory corruption * issues might happen in non-basic usage scenarios. - * The test set is disabled by default because it takes - * long time to run. * It's recommended to run this test whenever usrsctp version * used is updated to verify it properly works in stress * conditions under higher than usual load. @@ -610,16 +611,19 @@ namespace cricket { * are executed, so whenever memory bug is happen inside usrsctp, * it will be easier to understand what went wrong with ASAN * provided diagnostics information. + * The tests cases currently disabled by default due to + * long execution time and due to unresolved issue inside + * `usrsctp` library detected by try-bots with ThreadSanitizer. */ -class DISABLED_UsrSctpReliabilityTest : public ::testing::Test {}; +class UsrSctpReliabilityTest : public ::testing::Test {}; /** * A simple test which send multiple messages over reliable * connection, usefull to verify test infrastructure works. * Execution time is less than 1 second. */ -TEST_F(DISABLED_UsrSctpReliabilityTest, - AllMessagesAreDeliveredOverReliableConnection) { +TEST_F(UsrSctpReliabilityTest, + DISABLED_AllMessagesAreDeliveredOverReliableConnection) { auto thread1 = rtc::Thread::Create(); auto thread2 = rtc::Thread::Create(); thread1->Start(); @@ -649,10 +653,12 @@ TEST_F(DISABLED_UsrSctpReliabilityTest, * A test to verify that multiple messages can be reliably delivered * over lossy network when usrsctp configured to guarantee reliably * and in order delivery. + * The test case is disabled by default because it takes + * long time to run. * Execution time is about 2.5 minutes. */ -TEST_F(DISABLED_UsrSctpReliabilityTest, - AllMessagesAreDeliveredOverLossyConnectionReliableAndInOrder) { +TEST_F(UsrSctpReliabilityTest, + DISABLED_AllMessagesAreDeliveredOverLossyConnectionReliableAndInOrder) { auto thread1 = rtc::Thread::Create(); auto thread2 = rtc::Thread::Create(); thread1->Start(); @@ -682,10 +688,12 @@ TEST_F(DISABLED_UsrSctpReliabilityTest, * A test to verify that multiple messages can be reliably delivered * over lossy network when usrsctp configured to retransmit lost * packets. + * The test case is disabled by default because it takes + * long time to run. * Execution time is about 2.5 minutes. */ -TEST_F(DISABLED_UsrSctpReliabilityTest, - AllMessagesAreDeliveredOverLossyConnectionWithRetries) { +TEST_F(UsrSctpReliabilityTest, + DISABLED_AllMessagesAreDeliveredOverLossyConnectionWithRetries) { auto thread1 = rtc::Thread::Create(); auto thread2 = rtc::Thread::Create(); thread1->Start(); @@ -723,10 +731,12 @@ TEST_F(DISABLED_UsrSctpReliabilityTest, * It is recoomended to run this test whenever usrsctp version * used by WebRTC is updated. * + * The test case is disabled by default because it takes + * long time to run. * Execution time of this test is about 1-2 hours. */ -TEST_F(DISABLED_UsrSctpReliabilityTest, - AllMessagesAreDeliveredOverLossyConnectionConcurrentTests) { +TEST_F(UsrSctpReliabilityTest, + DISABLED_AllMessagesAreDeliveredOverLossyConnectionConcurrentTests) { ThreadPool pool(16); cricket::SendDataParams send_params;